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

[Yonghong Song <yhs@meta.com>]

On 11/10/22 4:45 AM, Toke Høiland-Jørgensen wrote:
> John Fastabend <john.fastabend@gmail.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.
> 
> 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?

The kfunc execution can be replaced with a register move like
	r0 = r1  /* r1 is the ctx */
	/* r0 is the kctx */

> 
> -Toke