Re: [PATCH bpf-next v1 3/3] selftests/bpf: tests for using dynptrs to parse skb and xdp buffers

[Joanne Koong <joannelkoong@gmail.com>]

On Tue, Aug 2, 2022 at 3:21 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Mon, Aug 1, 2022 at 12:12 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Jul 26, 2022 at 11:48 AM Joanne Koong <joannelkoong@gmail.com> wrote:
> > >
> > >
> > > -static __always_inline int handle_ipv4(struct xdp_md *xdp)
> > > +static __always_inline int handle_ipv4(struct xdp_md *xdp, struct bpf_dynptr *xdp_ptr)
> > >  {
> > > -       void *data_end = (void *)(long)xdp->data_end;
> > > -       void *data = (void *)(long)xdp->data;
> > > +       struct bpf_dynptr new_xdp_ptr;
> > >         struct iptnl_info *tnl;
> > >         struct ethhdr *new_eth;
> > >         struct ethhdr *old_eth;
> > > -       struct iphdr *iph = data + sizeof(struct ethhdr);
> > > +       struct iphdr *iph;
> > >         __u16 *next_iph;
> > >         __u16 payload_len;
> > >         struct vip vip = {};
> > > @@ -90,10 +90,12 @@ static __always_inline int handle_ipv4(struct xdp_md *xdp)
> > >         __u32 csum = 0;
> > >         int i;
> > >
> > > -       if (iph + 1 > data_end)
> > > +       iph = bpf_dynptr_data(xdp_ptr, ethhdr_sz,
> > > +                             iphdr_sz + (tcphdr_sz > udphdr_sz ? tcphdr_sz : udphdr_sz));
> > > +       if (!iph)
> > >                 return XDP_DROP;
> >
> > dynptr based xdp/skb access looks neat.
> > Maybe in addition to bpf_dynptr_data() we can add helper(s)
> > that return skb/xdp_md from dynptr?
> > This way the code will be passing dynptr only and there will
> > be no need to pass around 'struct xdp_md *xdp' (like this function).
>
> Great idea! I'll add this to v2.

Thinking about this some more, I don't think the extra helpers will be
that useful. We'd have to add 2 custom helpers (bpf_dynptr_get_xdp +
bpf_dynptr_get_skb) and calling them would always require a null check
(since we'd return NULL if the dyntpr is invalid/null). I think it'd
be faster / easier for the program to just pass in the ctx as an extra
arg in the special cases where it needs that.

>
> >
> > Separately please keep the existing tests instead of converting them.
> > Either ifdef data/data_end vs dynptr style or copy paste
> > the whole test into a new .c file. Whichever is cleaner.
>
> Will do for v2.