netdev - Re: [PATCH net] net: natsemi: fix `rx_dropped` double accounting on `netif

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: <DCQXWATX22EF.1AWF6AGWZ639S@gmail.com>
Date: Sat, 13 Sep 2025 00:50:37 +0900
From: "Yeounsu Moon" <yyyynoom@...il.com>
To: "Eric Dumazet" <edumazet@...gle.com>
Cc: "Andrew Lunn" <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, "Jakub Kicinski" <kuba@...nel.org>, "Paolo Abeni"
 <pabeni@...hat.com>, <netdev@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net: natsemi: fix `rx_dropped` double accounting on
 `netif_rx()` failure

On Fri Sep 12, 2025 at 11:19 PM KST, Eric Dumazet wrote:
>
> I do not think this Fixes: is correct.
>
> I think core networking got this accounting in netif_rx() in 2010
>
> commit caf586e5f23c (" net: add a core netdev->rx_dropped counter")
>
I hadn't considered that the Fixes: tag can refer to code outside of the
changes being made. Thank you for pointing this out. I also noticed your
earlier work from 2010.

I'll update the Fixes: tag as you suggested.

>> Signed-off-by: Yeounsu Moon <yyyynoom@...il.com>
>> ---
>>  drivers/net/ethernet/natsemi/ns83820.c | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
>> index 56d5464222d9..cdbf82affa7b 100644
>> --- a/drivers/net/ethernet/natsemi/ns83820.c
>> +++ b/drivers/net/ethernet/natsemi/ns83820.c
>> @@ -820,7 +820,7 @@ static void rx_irq(struct net_device *ndev)
>>         struct ns83820 *dev = PRIV(ndev);
>>         struct rx_info *info = &dev->rx_info;
>>         unsigned next_rx;
>> -       int rx_rc, len;
>> +       int len;
>>         u32 cmdsts;
>>         __le32 *desc;
>>         unsigned long flags;
>> @@ -881,8 +881,10 @@ static void rx_irq(struct net_device *ndev)
>>                 if (likely(CMDSTS_OK & cmdsts)) {
>>  #endif
>>                         skb_put(skb, len);
>> -                       if (unlikely(!skb))
>
> I doubt this driver is used.
>
I also honestly doubt that this driver is still in use.

I came across it while analyzing the `netif_rx()` and `rx_dropped` code
paths, and I noticed that there are quite a few unmanaged drivers using
this kind of code. So I started to fix that.

But If patches like this only burden busy maintainers and reviewers,
I'll stop sending them. That said, I do think leaving unmanaged drivers
as they are is also problematic.

As a newcomer sending patches to netdev, I realized that there are quite
a few such drivers. I don't necessarily believe they all must be actively
maintained, but it feels like some action is needed.

> Notice that this test  about skb being NULL or not happens after
> skb_put(skb, len)
> which would have crashed anyway if skb was NULL.
>
I think I wrote the commit message incorrectly.
The main point was not about `skb_put()`, but rather about the `if`
statement that checks `skb`.
That said, after your comment I realized that `skb_put()` itself also
looks problematic.

Thank you for the detailed review!

	Yeounsu Moon