Date:   Mon, 8 Mar 2021 23:14:38 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        john.fastabend@...il.com,
        Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        Marek Majtyka <alardam@...il.com>
Subject: Re: [PATCH bpf-next V2 1/2] bpf: BPF-helper for MTU checking add
 length input

On 2/27/21 11:37 AM, Jesper Dangaard Brouer wrote:
> On Sat, 27 Feb 2021 00:36:02 +0100
> Daniel Borkmann <daniel@...earbox.net> wrote:
>> On 2/18/21 12:49 PM, Jesper Dangaard Brouer wrote:
>>> The FIB lookup example[1] show how the IP-header field tot_len
>>> (iph->tot_len) is used as input to perform the MTU check.
>>>
>>> This patch extend the BPF-helper bpf_check_mtu() with the same ability
>>> to provide the length as user parameter input, via mtu_len parameter.
>>>
>>> [1] samples/bpf/xdp_fwd_kern.c
>>>
>>> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
>>> Acked-by: John Fastabend <john.fastabend@...il.com>
[...]
>> Btw, one more note on the whole bpf_*_check_mtu() helper... Last week I implemented
>> PMTU discovery support for clients for Cilium's XDP stand-alone LB in DSR mode, so I
>> was briefly considering whether to use the bpf_xdp_check_mtu() helper for retrieving
>> the device MTU, but then I thought to myself why having an unnecessary per-packet cost
>> for an extra helper call if I could just pass it in via constant instead. So I went
>> with the latter instead of the helper with the tradeoff to restart the Cilium agent
>> if someone actually changes MTU in prod which is a rare event anyway.
>>
>> Looking at what bpf_xdp_check_mtu() for example really offers is retrieval of dev->mtu
>> as well as dev->hard_header_len and the rest can all be done inside the BPF prog itself
>> w/o the helper overhead. Why am I mentioning this.. because the above change is a similar
>> case of what could have been done /inside/ the BPF prog anyway (especially on XDP where
>> extra overhead should be cut where possible).
> 
> The XDP case looks super simple now, but I thinking ahead.  When
> Lorenzo adds multi-buff support, then we can/must update this helper to
> use another XDP length value, based on the multi-buff jumbo-frame len.
> 
> Maybe we need another helper or what you propose below. BUT we could
> also allow this helper (via flag?) to ALSO check if dev support
> multi-buff XDP transmit (besides MTU limit with multi-buff len).  Then
> the BPF-programmer can know this packet cannot be redirected to the
> device.

Whether a XDP program is running on a device with multi-buff support or without
it should be transparent to this helper, in other words, the helper would have
to figure this out internally so that programs wouldn't have to be changed (in
the ideal case).

Overall, I still think that especially for the XDP case where performance matters
most, all this could have been done inside the program itself w/o the overhead of
the helper call as outlined earlier with struct bpf_dev ; adding more flags like
querying if a device supports multi-buff XDP transmit has not much to do with the
original purpose of bpf_xdp_check_mtu() anymore (aka do one thing/do it well mantra),
maybe the API should have been named bpf_xdp_check_forwardable() instead if it goes
beyond MTU .. But similarly here, struct bpf_dev property could also solve this
case if the prog developer needs more sanity checks when it is not clear whether
both devs support it, which can then also be /compiled out/ for the situation when
it /is/ known a-priori.

>> I think it got lost somewhere in the many versions of the original set where it was
>> mentioned before, but allowing to retrieve the dev object into BPF context and then
>> exposing it similarly to how we handle the case of struct bpf_tcp_sock would have been
>> much cleaner approach, e.g. the prog from XDP and tc context would be able to do:
>>
>>     struct bpf_dev *dev = ctx->dev;
>>
>> And we expose initially, for example:
>>
>>     struct bpf_dev {
>>       __u32 mtu;
>>       __u32 hard_header_len;
>>       __u32 ifindex;
>>       __u32 rx_queues;
>>       __u32 tx_queues;
>>     };
>>
>> And we could also have a BPF helper for XDP and tc that would fetch a /different/ dev
>> given we're under RCU context anyway, like ...
>>
>> BPF_CALL_2(bpf_get_dev, struct xdp_buff *, xdp, u32, ifindex)
>> {
>> 	return dev_get_by_index_rcu(dev_net(xdp->rxq->dev), index);
>> }
>>
>> ... returning a new dev_or_null type. With this flexibility everything else can be done
>> inside the prog, and later on it easily allows to expose more from dev side. Actually,
>> I'm inclined to code it up ...
> 
> I love the idea to retrieve the dev object into BPF context.  It is
> orthogonal, and doesn't replace the MTU helpers as the packet ctx
> objects (SKB and xdp_buff) are more complex, and the helper allows us
> to extend them without users have to update their BPF-code (as desc
> above).
> 
> I do think it makes a lot of sense to expose/retrieve dev object into
> BPF context.  As I hinted about, when we implement XDP multi-buff, then
> the bpf_redirect BPF-helper cannot check if the remote device support
> multi-buff transmit (as it don't have packet ctx).  If we have the dev
> object, the we could expose XDP features that allow us (BPF-programmer)
> to check this prior to doing the redirect.

Yep, that would be cleaner.

Thanks,
Daniel

Powered by blists - more mailing lists