Message-ID: <d9531955-06ad-ccdd-d3d0-4779400090ba@iogearbox.net>
Date: Tue, 26 Mar 2024 13:46:22 +0100
From: Daniel Borkmann <daniel@...earbox.net>
To: Stanislav Fomichev <sdf@...gle.com>,
 Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>, Guillaume Nault <gnault@...hat.com>,
 patchwork-bot+netdevbpf@...nel.org, "David S. Miller" <davem@...emloft.net>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Alexei Starovoitov <ast@...nel.org>, Martin KaFai Lau
 <martin.lau@...ux.dev>, Andrii Nakryiko <andrii@...nel.org>,
 Network Development <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
 Eric Dumazet <eric.dumazet@...il.com>,
 syzbot+9e27778c0edc62cb97d8@...kaller.appspotmail.com,
 Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net] bpf: Don't redirect too small packets

On 3/25/24 5:28 PM, Stanislav Fomichev wrote:
> On 03/25, Alexei Starovoitov wrote:
>> On Mon, Mar 25, 2024 at 6:33 AM Eric Dumazet <edumazet@...gle.com> wrote:
>>>
>>> On Sat, Mar 23, 2024 at 4:02 AM Alexei Starovoitov
>>> <alexei.starovoitov@...il.com> wrote:
>>>>
>>>> On Fri, Mar 22, 2024 at 7:10 AM <patchwork-bot+netdevbpf@...nel.org> wrote:
>>>>>
>>>>> Hello:
>>>>>
>>>>> This patch was applied to bpf/bpf.git (master)
>>>>> by Daniel Borkmann <daniel@...earbox.net>:
>>>>>
>>>>> On Fri, 22 Mar 2024 12:24:07 +0000 you wrote:
>>>>>> Some drivers ndo_start_xmit() expect a minimal size, as shown
>>>>>> by various syzbot reports [1].
>>>>>>
>>>>>> Willem added in commit 217e6fa24ce2 ("net: introduce device min_header_len")
>>>>>> the missing attribute that can be used by upper layers.
>>>>>>
>>>>>> We need to use it in __bpf_redirect_common().
>>>>
>>>> This patch broke empty_skb test:
>>>> $ test_progs -t empty_skb
>>>>
>>>> test_empty_skb:FAIL:ret: veth ETH_HLEN+1 packet ingress
>>>> [redirect_ingress] unexpected ret: veth ETH_HLEN+1 packet ingress
>>>> [redirect_ingress]: actual -34 != expected 0
>>>> test_empty_skb:PASS:err: veth ETH_HLEN+1 packet ingress [redirect_egress] 0 nsec
>>>> test_empty_skb:FAIL:ret: veth ETH_HLEN+1 packet ingress
>>>> [redirect_egress] unexpected ret: veth ETH_HLEN+1 packet ingress
>>>> [redirect_egress]: actual -34 != expected 1
>>>>
>>>> And looking at the test I think it's not a test issue.
>>>> This check
>>>> if (unlikely(skb->len < dev->min_header_len))
>>>> is rejecting more than it should.
>>>>
>>>> So I reverted this patch for now.
>>>
>>> OK, it seems I missed __bpf_rx_skb() vs __bpf_tx_skb(), but even if I
>>> move my sanity test in __bpf_tx_skb(),
>>> the bpf test program still fails, I am suspecting the test needs to be adjusted.
>>>
>>>
>>>
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 745697c08acb3a74721d26ee93389efa81e973a0..e9c0e2087a08f1d8afd2c3e8e7871ddc9231b76d
>>> 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -2128,6 +2128,12 @@ static inline int __bpf_tx_skb(struct
>>> net_device *dev, struct sk_buff *skb)
>>>                  return -ENETDOWN;
>>>          }
>>>
>>> +       if (unlikely(skb->len < dev->min_header_len)) {
>>> +               pr_err_once("__bpf_tx_skb skb->len=%u <
>>> dev(%s)->min_header_len(%u)\n", skb->len, dev->name,
>>> dev->min_header_len);
>>> +               DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false);
>>> +               kfree_skb(skb);
>>> +               return -ERANGE;
>>> +       } // Note: this is before we change skb->dev
>>>          skb->dev = dev;
>>>          skb_set_redirected_noclear(skb, skb_at_tc_ingress(skb));
>>>          skb_clear_tstamp(skb);
>>>
>>>
>>> -->
>>>
>>>
>>> test_empty_skb:FAIL:ret: veth ETH_HLEN+1 packet ingress
>>> [redirect_egress] unexpected ret: veth ETH_HLEN+1 packet ingress
>>> [redirect_egress]: actual -34 != expected 1
>>>
>>> [   58.382051] __bpf_tx_skb skb->len=1 < dev(veth0)->min_header_len(14)
>>> [   58.382778] skb len=1 headroom=78 headlen=1 tailroom=113
>>>                 mac=(64,14) net=(78,-1) trans=-1
>>>                 shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
>>>                 csum(0x0 ip_summed=0 complete_sw=0 valid=0 level=0)
>>>                 hash(0x0 sw=0 l4=0) proto=0x7f00 pkttype=0 iif=0
>>
>> Hmm. Something is off.
>> That test creates 15 byte skb.
>> It's not obvious to me how it got reduced to 1.
>> Something stripped L2 header and the prog is trying to redirect
>> such skb into veth that expects skb with L2 ?
>>
>> Stan,
>> please take a look.
>> Since you wrote that test.
> 
> Sure. Daniel wants to take a look on a separate thread, so we can sync
> up. Tentatively, seems like the failure is in the lwt path that does
> indeed drop the l2.

If we'd change the test into the below, the tc and empty_skb tests pass.
run_lwt_bpf() calls into skb_do_redirect() which has L2 stripped, and thus
skb->len is 1 in this test. We do use skb_mac_header_len() also in other
tc BPF helpers, so perhaps s/skb->len/skb_mac_header_len(skb)/ is the best
way forward..

static int __bpf_redirect_common(struct sk_buff *skb, struct net_device *dev,
                                  u32 flags)
{
         /* Verify that a link layer header is carried */
         if (unlikely(skb->mac_header >= skb->network_header || skb->len == 0)) {
                 kfree_skb(skb);
                 return -ERANGE;
         }

         if (unlikely(skb_mac_header_len(skb) < dev->min_header_len)) {
                 kfree_skb(skb);
                 return -ERANGE;
         }

         bpf_push_mac_rcsum(skb);
         return flags & BPF_F_INGRESS ?
                __bpf_rx_skb(dev, skb) : __bpf_tx_skb(dev, skb);
}

Thanks,
Daniel

Powered by blists - more mailing lists