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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/25/24 2:33 PM, Eric Dumazet wrote:
On Sat, Mar 23, 2024 at 4:02 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
On Fri, Mar 22, 2024 at 7:10 AM <patchwork-bot+netdevbpf@xxxxxxxxxx> wrote:

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@xxxxxxxxxxxxx>:

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)






[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux