On Tue, Mar 26, 2024 at 6:57 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 3/26/24 2:38 PM, Eric Dumazet wrote:
> > On Tue, Mar 26, 2024 at 2:37 PM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> >> On Tue, Mar 26, 2024 at 1:46 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> >>> 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)) {
> >>
> >> Unfortunately this will not prevent frames with skb->len == 1 to reach
> >> an Ethernet driver ndo_start_xmit()
> >>
> >> At ndo_start_xmit(), we do not look where the MAC header supposedly
> >> starts in the skb, we only use skb->data
> >>
> >> I have a syzbot repro using team driver, so I added the following part in team :
> >>
> >> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> >> index 0a44bbdcfb7b9f30a0c27b700246501c5eba322f..75e5ef585a8f05b35cfddbae0bfc377864e6e38c
> >> 100644
> >> --- a/drivers/net/team/team.c
> >> +++ b/drivers/net/team/team.c
> >> @@ -1714,6 +1714,11 @@ static netdev_tx_t team_xmit(struct sk_buff
> >> *skb, struct net_device *dev)
> >> bool tx_success;
> >> unsigned int len = skb->len;
> >>
> >> + if (len < 14) {
> >> + pr_err_once("team_xmit(len=%u)\n", len);
> >> + DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false);
> >> + WARN_ON_ONCE(1);
> >> + }
> >> tx_success = team_queue_override_transmit(team, skb);
> >> if (!tx_success)
> >> tx_success = team->ops.transmit(team, skb);
> >>
> >>
> >> And I get (with your suggestion instead of skb->len)
> >
> > Missing part in my copy/paste :
> >
> > [ 41.123829] team_xmit(len=1)
> > [ 41.124335] skb len=1 headroom=78 headlen=1 tailroom=113
> >
> >> mac=(78,0) net=(78,-1) trans=-1
>
> Interesting.
>
> Could you also dump dev->type and/or dev->min_header_len? I suspect
> this case may not be ARPHRD_ETHER in team.
>
> Above says mac=(78,0), so mac len is 0 and the check against the
> dev->min_header_len should have dropped it if it went that branch.
mac header is reset in __dev_queue_xmit() :
skb_reset_mac_header(skb);
So when the bpf code ran, skb_mac_header_len(skb) was 14,
but later the MAC header was set (to skb->data)
>
> I wonder, is team driver missing sth like :
>
> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> index 0a44bbdcfb7b..6256f0d2f565 100644
> --- a/drivers/net/team/team.c
> +++ b/drivers/net/team/team.c
> @@ -2124,6 +2124,7 @@ static void team_setup_by_port(struct net_device *dev,
> dev->type = port_dev->type;
> dev->hard_header_len = port_dev->hard_header_len;
> dev->needed_headroom = port_dev->needed_headroom;
> + dev->min_header_len = port_dev->min_header_len;
> dev->addr_len = port_dev->addr_len;
> dev->mtu = port_dev->mtu;
> memcpy(dev->broadcast, port_dev->broadcast, port_dev->addr_len);
>
I have confirmed that team min_header_len is 14, nothing seems to be
missing I think.
team_xmit(dev team0, skb->len=1, dev->min_header_len=14)
