On Fri, 26 Aug 2022 at 08:37, Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> On Thu, Aug 25, 2022 at 01:04:16AM +0200, Kumar Kartikeya Dwivedi wrote:
> > On Wed, 24 Aug 2022 at 20:11, Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
> > > I'm more and more liking the idea of limiting xdp to match the
> > > constraints of skb given that both you, Kumar, and Jakub have
> > > mentioned that portability between xdp and skb would be useful for
> > > users :)
> > >
> > > What are your thoughts on this API:
> > >
> > > 1) bpf_dynptr_data()
> > >
> > > Before:
> > > for skb-type progs:
> > > - data slices in fragments is not supported
> > >
> > > for xdp-type progs:
> > > - data slices in fragments is supported as long as it is in a
> > > contiguous frag (eg not across frags)
> > >
> > > Now:
> > > for skb + xdp type progs:
> > > - data slices in fragments is not supported
> I don't think it is necessary (or help) to restrict xdp slice from getting
> a fragment. In any case, the xdp prog has to deal with the case
> that bpf_dynptr_data(xdp_dynptr, offset, len) is across two fragments.
> Although unlikely, it still need to handle it (dynptr_data returns NULL)
> properly by using bpf_dynptr_read(). The same that the skb case
> also needs to handle dynptr_data returning NULL.
>
> Something like Kumar's sample in [0] should work for both
> xdp and skb dynptr but replace the helpers with
> bpf_dynptr_{data,read,write}().
>
> [0]: https://lore.kernel.org/bpf/20220726184706.954822-1-joannelkoong@xxxxxxxxx/T/#mf082a11403bc76fa56fde4de79a25c660680285c
>
> > >
> > >
> > > 2) bpf_dynptr_write()
> > >
> > > Before:
> > > for skb-type progs:
> > > - all data slices are invalidated after a write
> > >
> > > for xdp-type progs:
> > > - nothing
> > >
> > > Now:
> > > for skb + xdp type progs:
> > > - all data slices are invalidated after a write
> I wonder if the 'Before' behavior can be kept as is.
>
> The bpf prog that runs in both xdp and bpf should be
> the one always expecting the data-slice will be invalidated and
> it has to call the bpf_dynptr_data() again after writing.
> Yes, it is unnecessary for xdp but the bpf prog needs to the
> same anyway if the verifier was the one enforcing it for
> both skb and xdp dynptr type.
>
> If the bpf prog is written for xdp alone, then it has
> no need to re-get the bpf_dynptr_data() after writing.
>
Yeah, compared to the alternative, I guess it's better how it is right
now. It doesn't seem possible to meaningfully hide the differences
without penalizing either case. It also wouldn't be good to make it
less useful for XDP, since the whole point of this and the previous
effort was exposing bpf_xdp_pointer to the user.
