Toke Høiland-Jørgensen wrote: > John Fastabend <john.fastabend@xxxxxxxxx> 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@xxxxxxxx/ > >> 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@xxxxxx/ > >> > >> 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.
