netdev - Re: [PATCH v10 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver

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] [day] [month] [year] [list]

Message-ID: <20210120125217.6394e6a4@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 20 Jan 2021 12:52:17 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Lukasz Stelmach <l.stelmach@...sung.com>
Cc:     Andrew Lunn <andrew@...n.ch>, jim.cromie@...il.com,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Kukjin Kim <kgene@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org,
        Bartłomiej Żolnierkiewicz 
        <b.zolnierkie@...sung.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH v10 3/3] net: ax88796c: ASIX AX88796C SPI Ethernet
 Adapter Driver

On Wed, 20 Jan 2021 20:30:14 +0100 Lukasz Stelmach wrote:
> > You need to use 64 bit stats, like struct rtnl_link_stats64.
> > On a 32bit system at 100Mbps ulong can wrap in minutes.
> >  
> 
> Let me see. At first glance
> 
> git grep -l ndo_get_stats\\\> drivers/net/ethernet/  | xargs grep -li SPEED_100\\\>
> 
> quite a number of Fast Ethernet drivers use net_device_stats. Let me
> calculate.
> 
> - bytes
>   100Mbps is ~10MiB/s
>   sending 4GiB at 10MiB/s takes 27 minutes
> 
> - packets
>   minimum frame size is 84 bytes (840 bits on the wire) on 100Mbps means
>   119048 pps at this speed it takse 10 hours to transmit 2^32 packets
> 
> Anyway, I switched to rtnl_link_stats64. Tell me, is it OK to just
> memcpy() in .ndo_get_stats64?

Yup, you can just memcpy() your local copy over the one you get as an
argument of ndo_get_stats64

> >> +	struct work_struct	ax_work;  
> >
> > I don't see you ever canceling / flushing this work.
> > You should do that at least on driver remove if not close.  
> 
> Done.
> 
> Does it mean most drivers do it wrong?
> 
>     git grep INIT_WORK drivers/net/ethernet/ | \
>     sed -e 's/\(^[^:]*\):[^>]*->\([^,]*\),.*/\1        \2/' | \
>     while read file var; do \
>         grep -H $var $file;
>     done | grep INIT_WORK\\\|cancel_work

Some may use flush, but I wouldn't be surprised if there were bugs like
this out there.