Re: [Bug 209423] WARN_ON_ONCE() at rtl8169_tso_csum_v2()

[Eric Dumazet <edumazet@google.com>]

On Thu, Oct 8, 2020 at 6:37 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 02.10.2020 13:48, Eric Dumazet wrote:
> > On Fri, Oct 2, 2020 at 1:09 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
> >>
> >> On 02.10.2020 10:46, Eric Dumazet wrote:
> >>> On Fri, Oct 2, 2020 at 10:32 AM Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 10/2/20 10:26 AM, Eric Dumazet wrote:
> >>>>> On Thu, Oct 1, 2020 at 10:34 PM Heiner Kallweit <hkallweit1@gmail.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);
> >
>
> We did the following for debugging:
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 62b06523b..4c943b774 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3491,6 +3491,9 @@ static netdev_features_t gso_features_check(const struct sk_buff *skb,
>  {
>         u16 gso_segs = skb_shinfo(skb)->gso_segs;
>
> +       if (!skb_shinfo(skb)->gso_type)
> +               skb_warn_bad_offload(skb);

You also want to get a stack trace here, to give us the call graph.


> +
>         if (gso_segs > dev->gso_max_segs)
>                 return features & ~NETIF_F_GSO_MASK;
>
> Following skb then triggered the skb_warn_bad_offload. Not sure whether this helps
> to find out where in the network stack something goes wrong.
>
>
> [236222.967236] skb len=134 headroom=778 headlen=134 tailroom=31536
>                 mac=(778,14) net=(792,20) trans=812
>                 shinfo(txflags=0 nr_frags=0 gso(size=568 type=0 segs=1))
>                 csum(0x0 ip_summed=1 complete_sw=0 valid=0 level=0)
>                 hash(0x0 sw=0 l4=0) proto=0x0800 pkttype=0 iif=4
> [236222.967297] dev name=enp1s0 feat=0x0x00000100000041b2
> [236222.967392] skb linear:   00000000: 00 13 3b a0 01 e8 7c d3 0a 2d 1b 3b 08 00 45 00
> [236222.967404] skb linear:   00000010: 00 78 e2 e6 00 00 7b 06 52 e1 d8 3a d0 ce c0 a8
> [236222.967415] skb linear:   00000020: a0 06 01 bb 8b c6 53 91 be 5e 6e 60 bd e2 80 18
> [236222.967426] skb linear:   00000030: 01 13 5c f6 00 00 01 01 08 0a 3d d6 6a a3 63 ea
> [236222.967437] skb linear:   00000040: 5c d9 17 03 03 00 3f af 00 01 84 45 e2 36 e4 6a
> [236222.967454] skb linear:   00000050: 3d 76 a8 7f d7 12 fa 72 4b d1 d0 74 0d c1 49 77
> [236222.967466] skb linear:   00000060: 8b a4 bb 04 e5 aa 03 61 d3 e6 1f c9 0d 3e 46 c8
> [236222.967477] skb linear:   00000070: cd 1f 7d ce e8 a7 84 84 01 5d 1f b4 ee 4f 27 63
> [236222.967488] skb linear:   00000080: d2 a1 ab 1f 26 1d
>
>
>
> >
> > 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.
> >
>