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@xxxxxxxxxx> 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.
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
