On 02.10.2020 13:48, Eric Dumazet wrote:
> On Fri, Oct 2, 2020 at 1:09 PM Heiner Kallweit <hkallwe...@gmail.com> wrote:
>>
>> On 02.10.2020 10:46, Eric Dumazet wrote:
>>> On Fri, Oct 2, 2020 at 10:32 AM Eric Dumazet <eric.duma...@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/2/20 10:26 AM, Eric Dumazet wrote:
>>>>> On Thu, Oct 1, 2020 at 10:34 PM Heiner Kallweit <hkallwe...@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);
+
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.
>
