[PATCH net v3] net: usb: Remove disruptive netif_wake_queue in rtl8150_set_multicast

[PATCH net v3] net: usb: Remove disruptive netif_wake_queue in rtl8150_set_multicast

[I Viswanath]

syzbot reported WARNING in rtl8150_start_xmit/usb_submit_urb.
This is the sequence of events that leads to the warning:

rtl8150_start_xmit() {
	netif_stop_queue();
	usb_submit_urb(dev->tx_urb);
}

rtl8150_set_multicast() {
	netif_stop_queue();
	netif_wake_queue();		<-- wakes up TX queue before URB is done
}

rtl8150_start_xmit() {
	netif_stop_queue();
	usb_submit_urb(dev->tx_urb);	<-- double submission
}

rtl8150_set_multicast being the ndo_set_rx_mode callback should not be 
calling netif_stop_queue and notif_start_queue as these handle 
TX queue synchronization.

The net core function dev_set_rx_mode handles the synchronization
for rtl8150_set_multicast making it safe to remove these locks.

Reported-and-tested-by: syzbot+78cae3f37c62ad092caa@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=78cae3f37c62ad092caa
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Tested-by: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: I Viswanath <viswanathiyyappan@gmail.com>
---
v1: 
Link: https://lore.kernel.org/netdev/20250920045059.48400-1-viswanathiyyappan@gmail.com/
 
v2:
- Add explanation why netif_stop_queue/netif_wake_queue can be safely removed
- Add the net prefix to the patch, designating it to the net tree
Link: https://lore.kernel.org/netdev/20250920181852.18164-1-viswanathiyyappan@gmail.com/
 
v3:
- Simplified the event sequence that lead to the warning
- Added Tested-by tag

 drivers/net/usb/rtl8150.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index ddff6f19ff98..92add3daadbb 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -664,7 +664,6 @@ static void rtl8150_set_multicast(struct net_device *netdev)
 	rtl8150_t *dev = netdev_priv(netdev);
 	u16 rx_creg = 0x9e;
 
-	netif_stop_queue(netdev);
 	if (netdev->flags & IFF_PROMISC) {
 		rx_creg |= 0x0001;
 		dev_info(&netdev->dev, "%s: promiscuous mode\n", netdev->name);
@@ -678,7 +677,6 @@ static void rtl8150_set_multicast(struct net_device *netdev)
 		rx_creg &= 0x00fc;
 	}
 	async_set_registers(dev, RCR, sizeof(rx_creg), rx_creg);
-	netif_wake_queue(netdev);
 }
 
 static netdev_tx_t rtl8150_start_xmit(struct sk_buff *skb,
-- 
2.47.3

Re: [PATCH net v3] net: usb: Remove disruptive netif_wake_queue in rtl8150_set_multicast

[Petko Manolov]

On 25-09-24 19:13:50, I Viswanath wrote:
> syzbot reported WARNING in rtl8150_start_xmit/usb_submit_urb.
> This is the sequence of events that leads to the warning:
> 
> rtl8150_start_xmit() {
> 	netif_stop_queue();
> 	usb_submit_urb(dev->tx_urb);
> }
> 
> rtl8150_set_multicast() {
> 	netif_stop_queue();
> 	netif_wake_queue();		<-- wakes up TX queue before URB is done
> }
> 
> rtl8150_start_xmit() {
> 	netif_stop_queue();
> 	usb_submit_urb(dev->tx_urb);	<-- double submission
> }
> 
> rtl8150_set_multicast being the ndo_set_rx_mode callback should not be calling
> netif_stop_queue and notif_start_queue as these handle TX queue
> synchronization.

netif_[stop|wake]_queue() should have been removed from rtl8150_set_multicast()
long time ago, but somehow it has slipped under the radar.  As far as i can tell
this is the only change needed.


		Petko


> The net core function dev_set_rx_mode handles the synchronization
> for rtl8150_set_multicast making it safe to remove these locks.
> 
> Reported-and-tested-by: syzbot+78cae3f37c62ad092caa@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=78cae3f37c62ad092caa
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Tested-by: Michal Pecio <michal.pecio@gmail.com>
> Signed-off-by: I Viswanath <viswanathiyyappan@gmail.com>
> ---
> v1: 
> Link: https://lore.kernel.org/netdev/20250920045059.48400-1-viswanathiyyappan@gmail.com/
>  
> v2:
> - Add explanation why netif_stop_queue/netif_wake_queue can be safely removed
> - Add the net prefix to the patch, designating it to the net tree
> Link: https://lore.kernel.org/netdev/20250920181852.18164-1-viswanathiyyappan@gmail.com/
>  
> v3:
> - Simplified the event sequence that lead to the warning
> - Added Tested-by tag
> 
>  drivers/net/usb/rtl8150.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index ddff6f19ff98..92add3daadbb 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -664,7 +664,6 @@ static void rtl8150_set_multicast(struct net_device *netdev)
>  	rtl8150_t *dev = netdev_priv(netdev);
>  	u16 rx_creg = 0x9e;
>  
> -	netif_stop_queue(netdev);
>  	if (netdev->flags & IFF_PROMISC) {
>  		rx_creg |= 0x0001;
>  		dev_info(&netdev->dev, "%s: promiscuous mode\n", netdev->name);
> @@ -678,7 +677,6 @@ static void rtl8150_set_multicast(struct net_device *netdev)
>  		rx_creg &= 0x00fc;
>  	}
>  	async_set_registers(dev, RCR, sizeof(rx_creg), rx_creg);
> -	netif_wake_queue(netdev);
>  }
>  
>  static netdev_tx_t rtl8150_start_xmit(struct sk_buff *skb,
> -- 
> 2.47.3
> 

Re: [PATCH net v3] net: usb: Remove disruptive netif_wake_queue in rtl8150_set_multicast

[Michal Pecio]

On Wed, 24 Sep 2025 16:58:14 +0300, Petko Manolov wrote:
> netif_[stop|wake]_queue() should have been removed from rtl8150_set_multicast()
> long time ago, but somehow it has slipped under the radar.  As far as i can tell
> this is the only change needed.

Hi,

Glad to see that you are still around.

Do you happen to remember what was the reason for padding all TX frames
to at least 60 bytes?

This was apparently added in version "v0.5.0 (2002/03/28)".

I'm yet to test the exact effect of this hack (will the HW really send
frames with trailing garbage?) and what happens if it's removed (maybe
nothing bad? or was there a HW bug?), but this part caught my attention
because I think nowadays some people could consider it "information
leak" ;) And it looks like a waste of bandwidth at least.

Regards,
Michal

Re: [PATCH net v3] net: usb: Remove disruptive netif_wake_queue in rtl8150_set_multicast

[Michal Pecio]

On Wed, 24 Sep 2025 19:50:55 +0200, Michal Pecio wrote:
> Do you happen to remember what was the reason for padding all TX frames
> to at least 60 bytes?
> 
> This was apparently added in version "v0.5.0 (2002/03/28)".
> 
> I'm yet to test the exact effect of this hack (will the HW really send
> frames with trailing garbage?) and what happens if it's removed (maybe
> nothing bad? or was there a HW bug?), but this part caught my attention
> because I think nowadays some people could consider it "information
> leak" ;) And it looks like a waste of bandwidth at least.

Sorry, stupid question, such frames are illegal.

That being said, I see that other drivers pad them with zeros or
other fixed pattern ('skb_padto(skb, ETH_ZLEN)' seems to be common)
rather than just DMA beyond the specified length.

Re: [PATCH net v3] net: usb: Remove disruptive netif_wake_queue in rtl8150_set_multicast

[Jakub Kicinski]

On Wed, 24 Sep 2025 19:13:50 +0530 I Viswanath wrote:
> Signed-off-by: I Viswanath <viswanathiyyappan@gmail.com>

Sorry, one more nit - please spell out your first name, not just
the initial (in Author/From and SoB).
-- 
pw-bot: cr

Re: [PATCH net v3] net: usb: Remove disruptive netif_wake_queue in rtl8150_set_multicast

[viswanath]

On Thu, 25 Sept 2025 at 04:48, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 24 Sep 2025 19:13:50 +0530 I Viswanath wrote:
> > Signed-off-by: I Viswanath <viswanathiyyappan@gmail.com>
>
> Sorry, one more nit - please spell out your first name, not just
> the initial (in Author/From and SoB).
> --
> pw-bot: cr

I know it's uncommon but "I Viswanath" is actually my legal name

Thanks
Viswanath

Re: [PATCH net v3] net: usb: Remove disruptive netif_wake_queue in rtl8150_set_multicast

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 24 Sep 2025 19:13:50 +0530 you wrote:
> syzbot reported WARNING in rtl8150_start_xmit/usb_submit_urb.
> This is the sequence of events that leads to the warning:
> 
> rtl8150_start_xmit() {
> 	netif_stop_queue();
> 	usb_submit_urb(dev->tx_urb);
> }
> 
> [...]

Here is the summary with links:
  - [net,v3] net: usb: Remove disruptive netif_wake_queue in rtl8150_set_multicast
    https://git.kernel.org/netdev/net/c/958baf5eaee3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html