netdev - Re: [PATCH bpf-next V15 2/7] bpf: fix bpf_fib

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 8 Feb 2021 17:46:34 +0100
From:   Daniel Borkmann <borkmann@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        maze@...gle.com, lmb@...udflare.com, shaun@...era.io,
        Lorenzo Bianconi <lorenzo@...nel.org>, marek@...udflare.com,
        John Fastabend <john.fastabend@...il.com>,
        Jakub Kicinski <kuba@...nel.org>, eyal.birger@...il.com,
        colrack@...il.com, David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH bpf-next V15 2/7] bpf: fix bpf_fib_lookup helper MTU check
 for SKB ctx

On 2/8/21 5:27 PM, Jesper Dangaard Brouer wrote:
> On Mon, 8 Feb 2021 16:41:24 +0100
> Daniel Borkmann <daniel@...earbox.net> wrote:
>> On 2/8/21 4:20 PM, Jesper Dangaard Brouer wrote:
>>> On Mon, 8 Feb 2021 14:57:13 +0100
>>> Jesper Dangaard Brouer <brouer@...hat.com> wrote:
>>>> On Fri, 5 Feb 2021 01:06:35 +0100
>>>> Daniel Borkmann <daniel@...earbox.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.
> 
> I would really really like to make progress for this patchset.  I
> unfortunately finished coding this up (and tested with selftests)
> before I noticed this request (without optimizations).
> 
> I guess, I can revert my recent work by pulling in V12 of the patch.
> I'll do it tomorrow, as I want to have time to run my tests before
> re-sending patchset.

I'm okay either way - it was just a suggestion and not a request, of course.
If you already have it ready in the meantime, that is also not a problem, no
need to revert again.

Thanks,
Daniel