linux-kernel - Re: [PATCH net v2] net: usbnet: Fix the wrong netif_carrier

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]

Message-ID: <aJKaHfLteSF842IY@linux.gnuweeb.org>
Date: Wed, 6 Aug 2025 06:56:13 +0700
From: Ammar Faizi <ammarfaizi2@...weeb.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Simon Horman <horms@...nel.org>, Oliver Neukum <oneukum@...e.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Linux Netdev Mailing List <netdev@...r.kernel.org>,
	Linux USB Mailing List <linux-usb@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Armando Budianto <sprite@...weeb.org>, gwml@...r.gnuweeb.org,
	stable@...r.kernel.org, John Ernberg <john.ernberg@...ia.se>
Subject: Re: [PATCH net v2] net: usbnet: Fix the wrong netif_carrier_on()
 call placement

On Wed, Aug 06, 2025 at 01:40:37AM +0300, Linus Torvalds wrote:
> In particular, the whole *rest* of the code in that
> 
>         if (!netif_carrier_ok(dev->net)) {
> 
> no longer makes sense after we've turned the link on with that
> 
>                 if (test_and_clear_bit(EVENT_LINK_CARRIER_ON, &dev->flags))
>                         netif_carrier_on(dev->net);
> 
> sequence.
> 
> Put another way - once we've turned the carrier on, now that whole
> 
>                 /* kill URBs for reading packets to save bus bandwidth */
>                 unlink_urbs(dev, &dev->rxq);
> 
>                 /*
>                  * tx_timeout will unlink URBs for sending packets and
>                  * tx queue is stopped by netcore after link becomes off
>                  */
> 
> thing makes no sense.

After taking a look further, I agree with you. I git-blamed the
unlink_urbs()'s line and it's indeed expected to be called after link
becomes off. So yes, it makes no sense to call that when we're turning
the link on.

    commit 4b49f58fff00e6e9b24eaa31d4c6324393d76b0a
    Author: Ming Lei <ming.lei@...onical.com>
    Date:   Thu Apr 11 04:40:40 2013 +0000

        usbnet: handle link change
    
        The link change is detected via the interrupt pipe, and bulk
        pipes are responsible for transfering packets, so it is reasonable
        to stop bulk transfer after link is reported as off.
 
Even though my patch works on my machine. Something may go wrong.

> So my gut feel is that the
> 
>                 if (test_and_clear_bit(EVENT_LINK_CARRIER_ON, &dev->flags))
>                         netif_carrier_on(dev->net);
> 
> should actually be done outside that if-statement entirely, because it
> literally ends up changing the thing that if-statement is testing.

Apart from moving it outside that if-statement, unlink_urbs() call
should probably also be guarded as we agreed it makes no sense to call
it when we're turning the link on.

-- 
Ammar Faizi