Re: __sk_buff.data_end

[Daniel Borkmann <daniel@iogearbox.net>]

On 04/20/2017 04:17 PM, Johannes Berg wrote:
> On Thu, 2017-04-20 at 16:10 +0200, Daniel Borkmann wrote:
>>
>> I think this would be a rather more complex operation on the BPF
>> side, it would need changes from LLVM (which assumes initial ctx sits
>> in r1), verifier for tracking this ctx2, all the way down to JITs
>> plus some way to handle 1 and 2 argument program calls generically.
>> Much easier to pass additional meta data for the program via cb[],
>> for example.
>
> Yeah, it did seem very complex :)
>
>>> Alternatively I can clear another pointer (u64) in the CB, store a
>>> pointer there, and always emit code following that pointer - should
>>> be possible right?
>>
>> What kind of pointer? If it's something like data_end as read-only,
>> then this needs to be tracked in the verifier in addition, of course.
>> Other option you could do (depending on what you want to achieve) is
>> to have a bpf_probe_read() version as a helper for your prog type
>> that would further walk that pointer/struct (similar to tracing)
>> where this comes w/o any backward compat guarantees, though.
>
> I meant something like this
>
> struct wifi_cb {
> 	struct wifi_data *wifi_data;
> 	...
> 	void *data_end; // with BUILD_BUG_ON to the right offset
> };
>
> Then struct wifi_data can contain extra data that doesn't fit into
> wifi_cb, like the stuff I evicted for *data_end and *wifi_data. Let's
> say one of those fields is "u64 boottime_ns;" (as I did in my patch
> now), so we have
>
> struct wifi_data {
> 	u64 boottime_ns;
> };
>
> then I can still have
>
> struct __wifi_sk_buff {
> 	u32 len;
> 	u32 data;
> 	u32 data_end;
> 	u32 boottime_ns; // this is strange but
> 			 // seems to be done this way?
> };
>
> And then when boottime_ns is accessed, I can have:
>
>          case offsetof(struct __wifi_sk_buff, boottime_ns):
>                  off  = si->off;
>                  off -= offsetof(struct __wifi_sk_buff, boottime_ns);
>                  off += offsetof(struct sk_buff, cb);
>                  off += offsetof(struct wifi_cb, wifi_data);
>                  *insn++ = BPF_LDX_MEM(BPF_SIZEOF(void *), si->dst_reg,
>                                        si->src_reg, off);
> 		off = offsetof(struct wifi_data, boottime_ns);
> 		*isns++ = BPF_LDX_MEM(BPF_SIZEOF(u64), si->dst_reg,
> 				      si->src_reg, off);
>                  break;
>
> no?
>
> It seems to me this should work, and essentially emit code to follow
> the pointer to inside struct wifi_data. Assuming

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;
[...]

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.