netdev - Re: [PATCH bpf-next v4 2/3] bpf: Add xdp dynptrs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220826063706.pufgtu4rff4urbzf@kafai-mbp.dhcp.thefacebook.com>
Date:   Thu, 25 Aug 2022 23:37:06 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Joanne Koong <joannelkoong@...il.com>
Cc:     Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        bpf@...r.kernel.org, andrii@...nel.org, daniel@...earbox.net,
        ast@...nel.org, kuba@...nel.org, netdev@...r.kernel.org,
        "brouer@...hat.com" <brouer@...hat.com>, lorenzo@...nel.org
Subject: Re: [PATCH bpf-next v4 2/3] bpf: Add xdp dynptrs

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@...il.com> 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@gmail.com/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.

> >
> 
> There is also the other option: failing to write until you pull skb,
> which looks a lot better to me if we are adding this helper anyway...