Re: [PATCH bpf-next V15 2/7] bpf: fix bpf_fib_lookup helper MTU check for SKB ctx

From: Daniel Borkmann <daniel@iogearbox.net>
To: Jesper Dangaard Brouer <brouer@redhat.com>
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org,
	Daniel Borkmann <borkmann@iogearbox.net>,
	Alexei Starovoitov <alexei.starovoitov@gmail.com>,
	maze@google.com, lmb@cloudflare.com, shaun@tigera.io,
	Lorenzo Bianconi <lorenzo@kernel.org>,
	marek@cloudflare.com, John Fastabend <john.fastabend@gmail.com>,
	Jakub Kicinski <kuba@kernel.org>,
	eyal.birger@gmail.com, colrack@gmail.com,
	David Ahern <dsahern@kernel.org>
Subject: Re: [PATCH bpf-next V15 2/7] bpf: fix bpf_fib_lookup helper MTU check for SKB ctx
Date: Mon, 8 Feb 2021 16:41:24 +0100	[thread overview]
Message-ID: <547131a3-5125-d419-8e61-0fc675d663a8@iogearbox.net> (raw)
In-Reply-To: <20210208162056.44b0236e@carbon>

On 2/8/21 4:20 PM, Jesper Dangaard Brouer wrote:
> On Mon, 8 Feb 2021 14:57:13 +0100
> Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>> On Fri, 5 Feb 2021 01:06:35 +0100
>> Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> On 2/2/21 5:26 PM, Jesper Dangaard Brouer wrote:
>>>> BPF end-user on Cilium slack-channel (Carlo Carraro) wants to use
>>>> bpf_fib_lookup for doing MTU-check, but *prior* to extending packet size,
>>>> by adjusting fib_params 'tot_len' with the packet length plus the expected
>>>> encap size. (Just like the bpf_check_mtu helper supports). He discovered
>>>> that for SKB ctx the param->tot_len was not used, instead skb->len was used
>>>> (via MTU check in is_skb_forwardable() that checks against netdev MTU).
>>>>
>>>> Fix this by using fib_params 'tot_len' for MTU check. If not provided (e.g.
>>>> zero) then keep existing TC behaviour intact. Notice that 'tot_len' for MTU
>>>> check is done like XDP code-path, which checks against FIB-dst MTU.
[...]
>>>> -	if (!rc) {
>>>> -		struct net_device *dev;
>>>> -
>>>> -		dev = dev_get_by_index_rcu(net, params->ifindex);
>>>> +	if (rc == BPF_FIB_LKUP_RET_SUCCESS && !check_mtu) {
>>>> +		/* When tot_len isn't provided by user,
>>>> +		 * check skb against net_device MTU
>>>> +		 */
>>>>    		if (!is_skb_forwardable(dev, skb))
>>>>    			rc = BPF_FIB_LKUP_RET_FRAG_NEEDED;
>>>
>>> ... so using old cached dev from above will result in wrong MTU check &
>>> subsequent passing of wrong params->mtu_result = dev->mtu this way.
>>
>> Yes, you are right, params->ifindex have a chance to change in the calls.
>> So, our attempt to save an ifindex lookup (dev_get_by_index_rcu) is not
>> correct.
>>
>>> So one
>>> way to fix is that we would need to pass &dev to bpf_ipv{4,6}_fib_lookup().
>>
>> Ok, I will try to code it up, and see how ugly it looks, but I'm no
>> longer sure that it is worth saving this ifindex lookup, as it will
>> only happen if BPF-prog didn't specify params->tot_len.
> 
> I guess we can still do this as an "optimization", but the dev/ifindex
> will very likely be another at this point.

I would say for sake of progress, lets ship your series w/o this optimization so
it can land, and we revisit this later on independent from here. Actually DavidA
back then acked the old poc patch I posted, so maybe that's worth a revisit as
well but needs more testing first.

Thanks,
Daniel