Date:   Wed, 09 Nov 2022 18:17:05 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     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

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);
 };
 
 /* 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.

Powered by blists - more mailing lists