Message-ID: <8c569ff6-dadc-18a2-69f2-8450c818dafc@meta.com>
Date:   Fri, 11 Nov 2022 07:15:27 -0800
From:   Yonghong Song <yhs@...a.com>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Toke Høiland-Jørgensen <toke@...nel.org>,
        hawk@...nel.org, daniel@...earbox.net, kuba@...nel.org,
        davem@...emloft.net, ast@...nel.org
Cc:     brouer@...hat.com, 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



On 11/11/22 2:51 AM, Jesper Dangaard Brouer wrote:
> 
> 
> On 10/11/2022 18.02, John Fastabend wrote:
>> 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.
>>
> 
> 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?

read-only access.

> 
> --Jesper
> 

Powered by blists - more mailing lists