Re: [1/2 bpf-next] bpf: expose net_device from xdp for metadata

[John Fastabend <john.fastabend@gmail.com>]

Yonghong Song wrote:
> 
> 
> On 11/9/22 6:17 PM, John Fastabend wrote:
> > Yonghong Song wrote:
> >>
> >>
> >> On 11/9/22 1:52 PM, John Fastabend wrote:
> >>> Allow xdp progs to read the net_device structure. Its useful to extract
> >>> info from the dev itself. Currently, our tracing tooling uses kprobes
> >>> to capture statistics and information about running net devices. We use
> >>> kprobes instead of other hooks tc/xdp because we need to collect
> >>> information about the interface not exposed through the xdp_md structures.
> >>> This has some down sides that we want to avoid by moving these into the
> >>> XDP hook itself. First, placing the kprobes in a generic function in
> >>> the kernel is after XDP so we miss redirects and such done by the
> >>> XDP networking program. And its needless overhead because we are
> >>> already paying the cost for calling the XDP program, calling yet
> >>> another prog is a waste. Better to do everything in one hook from
> >>> performance side.
> >>>
> >>> Of course we could one-off each one of these fields, but that would
> >>> explode the xdp_md struct and then require writing convert_ctx_access
> >>> writers for each field. By using BTF we avoid writing field specific
> >>> convertion logic, BTF just knows how to read the fields, we don't
> >>> have to add many fields to xdp_md, and I don't have to get every
> >>> field we will use in the future correct.
> >>>
> >>> For reference current examples in our code base use the ifindex,
> >>> ifname, qdisc stats, net_ns fields, among others. With this
> >>> patch we can now do the following,
> >>>
> >>>           dev = ctx->rx_dev;
> >>>           net = dev->nd_net.net;
> >>>
> >>> 	uid.ifindex = dev->ifindex;
> >>> 	memcpy(uid.ifname, dev->ifname, NAME);
> >>>           if (net)
> >>> 		uid.inum = net->ns.inum;
> >>>
> >>> to report the name, index and ns.inum which identifies an
> >>> interface in our system.
> >>
> >> In
> >> https://lore.kernel.org/bpf/ad15b398-9069-4a0e-48cb-4bb651ec3088@meta.com/
> >> Namhyung Kim wanted to access new perf data with a helper.
> >> I proposed a helper bpf_get_kern_ctx() which will get
> >> the kernel ctx struct from which the actual perf data
> >> can be retrieved. The interface looks like
> >> 	void *bpf_get_kern_ctx(void *)
> >> the input parameter needs to be a PTR_TO_CTX and
> >> the verifer is able to return the corresponding kernel
> >> ctx struct based on program type.
> >>
> >> The following is really hacked demonstration with
> >> some of change coming from my bpf_rcu_read_lock()
> >> patch set https://lore.kernel.org/bpf/20221109211944.3213817-1-yhs@fb.com/
> >>
> >> I modified your test to utilize the
> >> bpf_get_kern_ctx() helper in your test_xdp_md.c.
> >>
> >> With this single helper, we can cover the above perf
> >> data use case and your use case and maybe others
> >> to avoid new UAPI changes.
> > 
> > hmm I like the idea of just accessing the xdp_buff directly
> > instead of adding more fields. I'm less convinced of the
> > kfunc approach. What about a terminating field *self in the
> > xdp_md. Then we can use existing convert_ctx_access to make
> > it BPF inlined and no verifier changes needed.
> > 
> > Something like this quickly typed up and not compiled, but
> > I think shows what I'm thinking.
> > 
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 94659f6b3395..10ebd90d6677 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -6123,6 +6123,10 @@ struct xdp_md {
> >          __u32 rx_queue_index;  /* rxq->queue_index  */
> >   
> >          __u32 egress_ifindex;  /* txq->dev->ifindex */
> > +       /* Last xdp_md entry, for new types add directly to xdp_buff and use
> > +        * BTF access. Reading this gives BTF access to xdp_buff.
> > +        */
> > +       __bpf_md_ptr(struct xdp_buff *, self);
> >   };
> 
> This would be the first instance to have a kernel internal struct
> in a uapi struct. Not sure whether this is a good idea or not.

We can use probe_read from some of the socket progs already but
sure.

> 
> >   
> >   /* DEVMAP map-value layout
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index bb0136e7a8e4..547e9576a918 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -9808,6 +9808,11 @@ static u32 xdp_convert_ctx_access(enum bpf_access_type type,
> >                  *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg,
> >                                        offsetof(struct net_device, ifindex));
> >                  break;
> > +       case offsetof(struct xdp_md, self):
> > +               *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct xdp_buff, self),
> > +                                     si->dst_reg, si->src_reg,
> > +                                     offsetof(struct xdp_buff, 0));
> > +               break;
> >          }
> >   
> >          return insn - insn_buf;
> > 
> > Actually even that single insn conversion is a bit unnessary because
> > should be enough to just change the type to the correct BTF_ID in the
> > verifier and omit any instructions. But it wwould be a bit confusing
> > for C side. Might be a good use for passing 'cast' info through to
> > the verifier as an annotation so it could just do the BTF_ID cast for
> > us without any insns.
> 
> We cannot change the context type to BTF_ID style which will be a
> uapi violation.

I don't think it would be uapi violation if user asks for it
by annotating the cast.

> 
> The helper I proposed can be rewritten by verifier as
> 	r0 = r1
> so we should not have overhead for this.

Agree other than reading the bpf asm where its a bit odd.

> It cover all program types with known uapi ctx -> kern ctx
> conversions. So there is no need to change existing uapi structs.
> Also I except that most people probably won't use this kfunc.
> The existing uapi fields might already serve most needs.

Maybe not sure missing some things we need.

> 
> Internally we have another use case to access some 'struct sock' fields
> but the uapi struct only has struct bpf_sock. Currently it is advised
> to use bpf_probe_read_kernel(...) to get the needed information.
> The proposed helper should help that too without uapi change.

Yep.

I'm fine doing it with bpf_get_kern_ctx() did you want me to code it
the rest of the way up and test it?

.John