Re: [PATCH bpf-next v1 1/3] bpf: Add skb dynptrs

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 5 Aug 2022 00:58:01 +0200 Kumar Kartikeya Dwivedi wrote:
> When I worked on [0], I actually did it a bit like you described in
> the original discussion under the xdp multi-buff thread, but I left
> the other case (where data to be read resides across frag boundaries)
> up to the user to handle, instead of automatically passing in pointer
> to stack and doing the copy for them, so in my case
> xdp_load_bytes/xdp_store_bytes is the fallback if you can't get a
> bpf_packet_pointer for a ctx, offset, len which you can directly
> access. But this was only for XDP, not for skb.
> 
> The advantage with a dynptr is that len for the slice from
> bpf_packet_pointer style helper doesn't have to be a constant, it can
> be a runtime value and since it is checked at runtime anyway, the
> helper's code is the same but access can be done for slices whose
> length is unknown to the verifier in a safe manner. The dynptr is very
> useful as the return value of such a helper.

I see.

> The suggested usage was like this:
> 
>     int err = 0;
>     char buf[N];
> 
>     off &= 0xffff;
>     ptr = bpf_packet_pointer(ctx, off, sizeof(buf), &err);
>     if (unlikely(!ptr)) {
>         if (err < 0)
>             return XDP_ABORTED;
>         err = bpf_xdp_load_bytes(ctx, off, buf, sizeof(buf));
>         if (err < 0)
>             return XDP_ABORTED;
>         ptr = buf;
>     }
>     ...
>     // Do some stores and loads in [ptr, ptr + N) region
>     ...
>     if (unlikely(ptr == buf)) {
>         err = bpf_xdp_store_bytes(ctx, off, buf, sizeof(buf));
>         if (err < 0)
>             return XDP_ABORTED;
>     }
> 
> So the idea was the same, there is still a "flush" (in that unlikely
> branch), but it is done explicitly by the user (which I found less
> confusing than it being done automagically or a by a new flush helper
> which will do the same thing we do here, but YMMV).

Ack, the flush is awkward to create an API for. I presume that's 
the main reason the xdp mbuf discussion wasn't fruitful.