On 10/11/2022 18.02, John Fastabend wrote:
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.
What kind of access does the BPF-prog obtain with these different
proposals, e.g. read-only access to xdp_buff or also write access?
--Jesper
