On Tue, Mar 26, 2024 at 7:08 PM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
>
> 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)
FYI, I am releasing today a syzbot report with the same problem.
