Re: [PATCH net] net: usb: lan78xx: fix possible skb leak

[Eric Dumazet <eric.dumazet@gmail.com>]

On 1/8/20 3:19 PM, RENARD Pierre-Francois wrote:
> OK
> 
> Before scp command ( and after a fresh reboot)
> ---------------------------------
> skbuff_ext_cache     378    378    192   21    1 : tunables 0    0    0 : slabdata     18     18      0
> skbuff_fclone_cache    112    112    512   16    2 : tunables 0    0    0 : slabdata      7      7      0
> skbuff_head_cache   1936   2160    256   16    1 : tunables 0    0    0 : slabdata    135    135      0
> ---------------------------------
> ---------------------------------
> 
> After the hang of scp (hanged at 203 MB)
> ---------------------------------
> skbuff_ext_cache     693    693    192   21    1 : tunables 0    0    0 : slabdata     33     33      0
> skbuff_fclone_cache    128    128    512   16    2 : tunables 0    0    0 : slabdata      8      8      0
> skbuff_head_cache   2032   2176    256   16    1 : tunables 0    0    0 : slabdata    136    136      0
> ---------------------------------
> TcpExtTCPSpuriousRtxHostQueues 120                0.0
> ---------------------------------
> 
> After CTRL-C of scp
> ---------------------------------
> skbuff_ext_cache     693    693    192   21    1 : tunables 0    0    0 : slabdata     33     33      0
> skbuff_fclone_cache    128    128    512   16    2 : tunables 0    0    0 : slabdata      8      8      0
> skbuff_head_cache   2112   2336    256   16    1 : tunables 0    0    0 : slabdata    146    146      0
> ---------------------------------
> TcpExtTCPSpuriousRtxHostQueues  124                0.0
> ---------------------------------
> 
> 
> 
> After the hang of a second attempt (hanged at 1214 MB)
> ---------------------------------
> skbuff_ext_cache     735    735    192   21    1 : tunables 0    0    0 : slabdata     35     35      0
> skbuff_fclone_cache    160    160    512   16    2 : tunables 0    0    0 : slabdata     10     10      0
> skbuff_head_cache   2096   2240    256   16    1 : tunables 0    0    0 : slabdata    140    140      0
> ---------------------------------
> TcpExtTCPSpuriousRtxHostQueues  248                0.0
> ---------------------------------
> 
> 
> After a third attempt (hanged at 55 MB)
> ---------------------------------
> skbuff_ext_cache     735    735    192   21    1 : tunables 0    0    0 : slabdata     35     35      0
> skbuff_fclone_cache    176    176    512   16    2 : tunables 0    0    0 : slabdata     11     11      0
> skbuff_head_cache   2000   2144    256   16    1 : tunables 0    0    0 : slabdata    134    134      0
> ---------------------------------
> TcpExtTCPSpuriousRtxHostQueues  365                0.0
> ---------------------------------
> 
> 


Thanks for testing.

This seems to suggest there is another bug in the driver, leading to some skb being never freed.

Since the driver seems to limit aggregation to 9000 bytes (MAX_SINGLE_PACKET_SIZE)
I wonder if gso skbs should also be limited to the same value.

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index d3239b49c3bb2803c874a2e8af332bcf03848e18..65dea9a94b90e27889c8f44294560ffeabda2eb9 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -3787,6 +3787,8 @@ static int lan78xx_probe(struct usb_interface *intf,
        if (ret < 0)
                goto out4;
 
+       netif_set_gso_max_size(netdev, MAX_SINGLE_PACKET_SIZE - MAX_HEADER);
+
        ret = register_netdev(netdev);
        if (ret != 0) {
                netif_err(dev, probe, netdev, "couldn't register the device\n");



> 
> 
> 
> 
> 
> 
> On 1/8/20 9:25 PM, Eric Dumazet wrote:
>> On Wed, Jan 8, 2020 at 11:13 AM RENARD Pierre-Francois
>> <pfrenard@gmail.com> wrote:
>>> I tried with last rawhide kernel 5.5.0-0.rc5.git0.1.local.fc32.aarch64
>>> I compiled it this night. (I check it includes the patch for lan78xx.c )
>>>
>>> Both tests (scp and nfs ) are failing the same way as before.
>>>
>>> Fox
>>>
>> Please report the output of " grep skb /proc/slabinfo"
>>
>> before and after your test.
>>
>> The symptoms (of retransmit being not attempted by TCP) match the fact
>> that skb(s) is(are) not freed by a driver (or some layer)
>>
>> When TCP detects this (function skb_still_in_host_queue()), one SNMP
>> counter is incremented
>>
>> nstat -a | grep TCPSpuriousRtxHostQueues
>>
>> Thanks.
>>
>>>
>>> On 1/7/20 7:57 PM, Eric Dumazet wrote:
>>>> If skb_linearize() fails, we need to free the skb.
>>>>
>>>> TSO makes skb bigger, and this bug might be the reason
>>>> Raspberry Pi 3B+ users had to disable TSO.
>>>>
>>>> Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
>>>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>>>> Reported-by: RENARD Pierre-Francois <pfrenard@gmail.com>
>>>> Cc: Stefan Wahren <stefan.wahren@i2se.com>
>>>> Cc: Woojung Huh <woojung.huh@microchip.com>
>>>> Cc: Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
>>>> ---
>>>>    drivers/net/usb/lan78xx.c | 9 +++------
>>>>    1 file changed, 3 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
>>>> index f940dc6485e56a7e8f905082ce920f5dd83232b0..fb4781080d6dec2af22f41c5e064350ea74130b3 100644
>>>> --- a/drivers/net/usb/lan78xx.c
>>>> +++ b/drivers/net/usb/lan78xx.c
>>>> @@ -2724,11 +2724,6 @@ static int lan78xx_stop(struct net_device *net)
>>>>        return 0;
>>>>    }
>>>>
>>>> -static int lan78xx_linearize(struct sk_buff *skb)
>>>> -{
>>>> -     return skb_linearize(skb);
>>>> -}
>>>> -
>>>>    static struct sk_buff *lan78xx_tx_prep(struct lan78xx_net *dev,
>>>>                                       struct sk_buff *skb, gfp_t flags)
>>>>    {
>>>> @@ -2740,8 +2735,10 @@ static struct sk_buff *lan78xx_tx_prep(struct lan78xx_net *dev,
>>>>                return NULL;
>>>>        }
>>>>
>>>> -     if (lan78xx_linearize(skb) < 0)
>>>> +     if (skb_linearize(skb)) {
>>>> +             dev_kfree_skb_any(skb);
>>>>                return NULL;
>>>> +     }
>>>>
>>>>        tx_cmd_a = (u32)(skb->len & TX_CMD_A_LEN_MASK_) | TX_CMD_A_FCS_;
>>>>
>>>
>