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.
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)
