Date:   Wed, 24 Aug 2022 11:10:54 -0700
From:   Joanne Koong <joannelkoong@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     Kumar Kartikeya Dwivedi <memxor@...il.com>, bpf@...r.kernel.org,
        andrii@...nel.org, daniel@...earbox.net, ast@...nel.org,
        kafai@...com, 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 Wed, Aug 24, 2022 at 3:39 AM Toke Høiland-Jørgensen <toke@...hat.com> wrote:
>
> Joanne Koong <joannelkoong@...il.com> writes:
>
> > On Mon, Aug 22, 2022 at 7:31 PM Kumar Kartikeya Dwivedi
> > <memxor@...il.com> wrote:
> >>
> >> +Cc XDP folks
> >>
> >> On Tue, 23 Aug 2022 at 02:12, Joanne Koong <joannelkoong@...il.com> wrote:
> >> >
> >> > Add xdp dynptrs, which are dynptrs whose underlying pointer points
> >> > to a xdp_buff. The dynptr acts on xdp data. xdp dynptrs have two main
> >> > benefits. One is that they allow operations on sizes that are not
> >> > statically known at compile-time (eg variable-sized accesses).
> >> > Another is that parsing the packet data through dynptrs (instead of
> >> > through direct access of xdp->data and xdp->data_end) can be more
> >> > ergonomic and less brittle (eg does not need manual if checking for
> >> > being within bounds of data_end).
> >> >
> >> > For reads and writes on the dynptr, this includes reading/writing
> >> > from/to and across fragments. For data slices, direct access to
> >>
> >> It's a bit awkward to have such a difference between xdp and skb
> >> dynptr's read/write. I understand why it is the way it is, but it
> >> still doesn't feel right. I'm not sure if we can reconcile the
> >> differences, but it makes writing common code for both xdp and tc
> >> harder as it needs to be aware of the differences (and then the flags
> >> for dynptr_write would differ too). So we're 90% there but not the
> >> whole way...
> >
> > Yeah, it'd be great if the behavior for skb/xdp progs could be the
> > same, but I'm not seeing a better solution here (unless we invalidate
> > data slices on writes in xdp progs, just to make it match more :P).
> >
> > Regarding having 2 different interfaces bpf_dynptr_from_{skb/xdp}, I'm
> > not convinced this is much of a problem - xdp and skb programs already
> > have different interfaces for doing things (eg
> > bpf_{skb/xdp}_{store/load}_bytes).
>
> This is true, but it's quite possible to paper over these differences
> and write BPF code that works for both TC and XDP. Subtle semantic
> differences in otherwise identical functions makes this harder.
>
> Today you can write a function like:
>
> static inline int parse_pkt(void *data, void* data_end)
> {
>         /* parse data */
> }
>
> And call it like:
>
> SEC("xdp")
> int parse_xdp(struct xdp_md *ctx)
> {
>         return parse_pkt(ctx->data, ctx->data_end);
> }
>
> SEC("tc")
> int parse_tc(struct __sk_buff *skb)
> {
>         return parse_pkt(skb->data, skb->data_end);
> }
>
>
> IMO the goal should be to be able to do the equivalent for dynptrs, like:
>
> static inline int parse_pkt(struct bpf_dynptr *ptr)
> {
>         __u64 *data;
>
>         data = bpf_dynptr_data(ptr, 0, sizeof(*data));
>         if (!data)
>                 return 0;
>         /* parse data */
> }
>
> SEC("xdp")
> int parse_xdp(struct xdp_md *ctx)
> {
>         struct bpf_dynptr ptr;
>
>         bpf_dynptr_from_xdp(ctx, 0, &ptr);
>         return parse_pkt(&ptr);
> }
>
> SEC("tc")
> int parse_tc(struct __sk_buff *skb)
> {
>         struct bpf_dynptr ptr;
>
>         bpf_dynptr_from_skb(skb, 0, &ptr);
>         return parse_pkt(&ptr);
> }
>

To clarify, this is already possible when using data slices, since the
behavior for data slices is equivalent between xdp and tc programs for
non-fragmented accesses. From looking through the selftests, I
anticipate that data slices will be the main way programs interact
with dynptrs. For the cases where the program may write into frags,
then bpf_dynptr_write will be needed (which is where functionality
between xdp and tc start differing) - today, we're not able to write
common code that writes into the frags since tc uses
bpf_skb_store_bytes and xdp uses bpf_xdp_store_bytes.

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


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

This will unite the functionality for skb and xdp programs across
bpf_dyntpr_data, bpf_dynptr_write, and bpf_dynptr_read. As for whether
we should unite bpf_dynptr_from_skb and bpf_dynptr_from_xdp into one
common bpf_dynptr_from_packet as Jakub brought in [0], I'm leaning
towards no because 1) if in the future there's some irreconcilable
aspect between skb and xdp that gets added, that'll be hard to support
since the expectation is that there is just one overall "packet
dynptr" 2) the "packet dynptr" view is not completely accurate (eg
bpf_dynptr_write accepts flags from skb progs and not xdp progs) 3)
this adds some additional hardcoding in the verifier since there's no
organic mapping between prog type -> prog ctx


[0] https://lore.kernel.org/bpf/20220726184706.954822-1-joannelkoong@gmail.com/T/#m1438f89152b1d0e539fe60a9376482bbc9de7b6e

>
> If the dynptr-based parse_pkt() function has to take special care to
> figure out where the dynptr comes from, it makes it a lot more difficult
> to write reusable packet parsing functions. So I'd be in favour of
> restricting the dynptr interface to the lowest common denominator of the
> skb and xdp interfaces even if that makes things slightly more awkward
> in the specialised cases...
>
> -Toke
>

Powered by blists - more mailing lists