On Fri, Aug 26, 2022 at 12:09 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> 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.
I'll keep it as is for v5.
