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:   Thu, 8 Oct 2020 18:37:07 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Eric Dumazet <edumazet@...gle.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 02.10.2020 13:48, Eric Dumazet wrote:
> 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);
> 

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);
+
 	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.
>