Re: [PATCH net] bpf: Don't redirect too small packets

[Daniel Borkmann <daniel@iogearbox.net>]

On 3/25/24 2:33 PM, Eric Dumazet wrote:
> On Sat, Mar 23, 2024 at 4:02 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>> On Fri, Mar 22, 2024 at 7:10 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>>>
>>> Hello:
>>>
>>> This patch was applied to bpf/bpf.git (master)
>>> by Daniel Borkmann <daniel@iogearbox.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.

Let me take a look, I do think so too that we'd need to adjust the test.
I'll see to have a patch for the latter so that we can reapply the fix
as-is along with it given it's correct.

> 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
> 
> Note that veth driver is one of the few 'Ethernet' drivers that make
> sure to get at least 14 bytes in the skb at ndo_start_xmit()
> after commit 726e2c5929de841fdcef4e2bf995680688ae1b87 ("veth: Ensure
> eth header is in skb's linear part")
> 
> BTW this last patch (changing veth) should have been done generically
> from act_mirred
> 
> (We do not want to patch ~400 ethernet drivers in the tree)
>