Subject: Re: __sk_buff.data_end

From: Daniel Borkmann <[email-masked]>
Date: Thu, 20 Apr 2017 16:46:18 +0200

On 04/20/2017 04:32 PM, Johannes Berg wrote:
> On Thu, 2017-04-20 at 16:28 +0200, Daniel Borkmann wrote:
>>
>> I see what you mean now. Yes, that's fine. We already do something
>> similar essentially with skb->ifindex access already (skb->dev +
>> dev->ifindex), f.e.:
>>
>> [...]
>> 	case offsetof(struct __sk_buff, ifindex):
>> 		BUILD_BUG_ON(FIELD_SIZEOF(struct net_device, ifindex)
>> != 4);
>>
>> 		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff,
>> dev),
>> 				      si->dst_reg, si->src_reg,
>> 				      offsetof(struct sk_buff, dev));
>> 		*insn++ = BPF_JMP_IMM(BPF_JEQ, si->dst_reg, 0, 1);
>> 		*insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg,
>> 				      offsetof(struct net_device,
>> ifindex));
>> 		break;
>> [...]
>
> Oh, right, good point.
>
>> Which is not too different from the above. You'd probably need to
>> populate the struct wifi_data each time if you place it onto the
>> stack, but perhaps could be optimized by storing that somewhere
>> else (e.g. somewhere via netdev, etc) and walking the pointer from
>> there, which would also spare you the cb[] save/restore.
>
> Hmm. I don't see what "somewhere else" I could possibly have though,
> given that I want the (kernel-side) context to be "struct sk_buff *"
> to allow the skb helpers?

I have not enough context on the wireless side, perhaps could be
somewhere under skb->dev->ieee80211_ptr or so, iff suitable. But
it also really doesn't matter much since this is all transparently
handled in the kernel, meaning these kind of rewrites can still be
changed at a later point in time, f.e. if it's only 'u64 boottime_ns'
right now, that could live directly in the cb[] w/o extra pointer,
and should that grow to more members, then it could be moved behind
a pointer later on and it still works as-is from the program point
of view.