netdev - Re: [Bug 209423] WARN_ON_ONCE() at rtl8169_tso_csum

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 2 Oct 2020 13:48:47 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [Bug 209423] WARN_ON_ONCE() at rtl8169_tso_csum_v2()

On Fri, Oct 2, 2020 at 1:09 PM Heiner Kallweit <hkallweit1@...il.com> wrote:
>
> On 02.10.2020 10:46, Eric Dumazet wrote:
> > On Fri, Oct 2, 2020 at 10:32 AM Eric Dumazet <eric.dumazet@...il.com> wrote:
> >>
> >>
> >>
> >> On 10/2/20 10:26 AM, Eric Dumazet wrote:
> >>> On Thu, Oct 1, 2020 at 10:34 PM Heiner Kallweit <hkallweit1@...il.com> wrote:
> >>>>
> >>>> I have a problem with the following code in ndo_start_xmit() of
> >>>> the r8169 driver. A user reported the WARN being triggered due
> >>>> to gso_size > 0 and gso_type = 0. The chip supports TSO(6).
> >>>> The driver is widely used, therefore I'd expect much more such
> >>>> reports if it should be a common problem. Not sure what's special.
> >>>> My primary question: Is it a valid use case that gso_size is
> >>>> greater than 0, and no SKB_GSO_ flag is set?
> >>>> Any hint would be appreciated.
> >>>>
> >>>>
> >>>
> >>> Maybe this is not a TCP packet ? But in this case GSO should have taken place.
> >>>
> >>> You might add a
> >>> pr_err_once("gso_type=%x\n", shinfo->gso_type);
> >>>
> >
> >>
> >> Ah, sorry I see you already printed gso_type
> >>
> >> Must then be a bug somewhere :/
> >
> >
> > napi_reuse_skb() does :
> >
> > skb_shinfo(skb)->gso_type = 0;
> >
> > It does _not_ clear gso_size.
> >
> > I wonder if in some cases we could reuse an skb while gso_size is not zero.
> >
> > Normally, we set it only from dev_gro_receive() when the skb is queued
> > into GRO engine (status being GRO_HELD)
> >
> Thanks Eric. I'm no expert that deep in the network stack and just wonder
> why napi_reuse_skb() re-initializes less fields in shinfo than __alloc_skb().
> The latter one does a
> memset(shinfo, 0, offsetof(struct skb_shared_info, dataref));
>

memset() over the whole thing is more expensive.

Here we know the prior state of some fields, while __alloc_skb() just
got a piece of memory with random content.

> What I can do is letting the affected user test the following.
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 62b06523b..8e75399cc 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6088,6 +6088,7 @@ static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb)
>
>         skb->encapsulation = 0;
>         skb_shinfo(skb)->gso_type = 0;
> +       skb_shinfo(skb)->gso_size = 0;
>         skb->truesize = SKB_TRUESIZE(skb_end_offset(skb));
>         skb_ext_reset(skb);
>

As I hinted, this should not be needed.

For debugging purposes, I would rather do :

BUG_ON(skb_shinfo(skb)->gso_size);


Nothing in GRO stack will change gso_size, unless the packet is queued
by GRO layer (after this, napi_reuse_skb() wont be called)

napi_reuse_skb() is only used when a packet has been aggregated to
another, and at this point gso_size should be still 0.