On 01/27/2016 08:42 PM, Alexei Starovoitov wrote:

On Wed, Jan 27, 2016 at 11:26 AM, Daniel Borkmann <daniel@...> wrote:

On 01/27/2016 06:18 PM, Alexei Starovoitov wrote:

On Wed, Jan 27, 2016 at 7:03 AM, Daniel Borkmann <daniel@...>
wrote:

+static inline int skb_try_make_writable(struct sk_buff *skb, int offset,
+ int len)

I would keep single 'offset' or 'len' argument here.

Sure, that's fine, can do that.

Let the caller do the math, since it's faster and
better matches meaning of single arg as
'length up to which to write'.

+{
+ return skb_cloned(skb) && !skb_clone_writable(skb, offset + len)
&&
+ pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+}
+

..

@@ -1737,6 +1731,13 @@ bool bpf_helper_changes_skb_data(void *func)
return true;
if (func == bpf_skb_vlan_pop)
return true;
+ if (func == bpf_skb_store_bytes)
+ return true;
+ if (func == bpf_l3_csum_replace)
+ return true;
+ if (func == bpf_l4_csum_replace)
+ return true;

yep. was thinking to do the same, since
bpf_helper_changes_skb_data() landed.
That should be a nice addition!

Yeah, I think reloading pointers via JIT should not take too many cycles.

Thanks

Was still thinking if we better should extend this (rather slow-path)
test into handling it more gracefully:

!skb_shared(skb) && pskb_expand_head(skb, 0, 0, GFP_ATOMIC);

Shared skbs should be rather rare. But, there seem to be tricky things
with skb_get() or raw atomic_inc(&skb->users) that /could/ cause a BUG
when calling into pskb_expand_head() in our path. Taking pktgen aside,
I remember from an old netdev discussion, that with taps shared skbs
and pskb_expand_head() should cause issues. Going through the pf_packet

I don't think so. only pktgen does ugly things.
it's a requirement of the IP stack to have users == 1.
we had this discussion before. I will try to dig out my old email.

I believe from the old nft ingress discussion. ;) But maybe it was just
a different issue w/ other actions (or not up to date fact anymore).

btw, there is skb_make_writable() that used by netfilter,
but doing skb_cloned(skb) && !skb_clone_writable() && pskb_expand
is faster and probably cleaner.

There's also skb_ensure_writable(), but seems to be doing more than we
actually need.

Thanks,
Daniel