Message-ID: <636d2e8c5a010_145693208c8@john.notmuch>
Date:   Thu, 10 Nov 2022 09:02:04 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Toke Høiland-Jørgensen <toke@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Yonghong Song <yhs@...a.com>,
        John Fastabend <john.fastabend@...il.com>, hawk@...nel.org,
        daniel@...earbox.net, kuba@...nel.org, davem@...emloft.net,
        ast@...nel.org
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, sdf@...gle.com
Subject: Re: [1/2 bpf-next] bpf: expose net_device from xdp for metadata

Toke Høiland-Jørgensen wrote:
> John Fastabend <john.fastabend@...il.com> writes:
> 
> > 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);
> >  };
> 
> xdp_md is UAPI; I really don't think it's a good idea to add "unstable"
> BTF fields like this to it, that's just going to confuse people. Tying
> this to a kfunc for conversion is more consistent with the whole "kfunc
> and BTF are its own thing" expectation.

hmm from my side self here would be stable. Whats behind it is not,
but that seems fine to me.  Doing `ctx->self` feels more natural imo
then doing a call. A bunch more work but could do btf casts maybe
with annotations. I'm not sure its worth it though because only reason
I can think to do this would be for this self reference from ctx.

   struct xdp_buff *xdp = __btf (struct xdp_buff *)ctx;

C++ has 'this' as well but thats confusing from C side. Could have
a common syntax to do 'ctx->this' to get the pointer in BTF
format.

Maybe see what Yonghong thinks.

> 
> The kfunc doesn't actually have to execute any instructions either, it
> can just be collapsed into a type conversion to BTF inside the verifier,
> no?

Agree either implementation can be made that same underneath its just
a style question. I can probably do either but using the ctx keeps
the existing machinery to go through is_valid_access and so on.

Thanks.

Powered by blists - more mailing lists