[PATCH net v2] usbnet: fix EVENT_RX_PAUSED race in usbnet_skb_return()

[PATCH net v2] usbnet: fix EVENT_RX_PAUSED race in usbnet_skb_return()

[Ömer Mete Kaya]

usbnet_resume_rx() disables local BHs before clearing EVENT_RX_PAUSED,
but this does not synchronize with usbnet_skb_return() running on
another CPU. On SMP, usbnet_skb_return() on CPU0 can pass the test_bit()
check while CPU1 runs usbnet_resume_rx(), clears the flag and drains the
queue, leaving the skb stranded in rxq_pause until the next MTU change
or device stop.

Fix with a double-check pattern: move clear_bit() in usbnet_resume_rx()
under rxq_pause.lock; in usbnet_skb_return() take the lock and recheck
the flag only on the slow path (unlikely paused), keeping the normal RX
fast path lock-free.

Fixes: 43daa96b166c ("usbnet: Stop RX Q on MTU change")
Signed-off-by: Ömer Mete Kaya <omermetekaya0@gmail.com>

---
v2: Squash usbnet_resume_rx() fix into same patch; previous version
    only changed usbnet_skb_return() and was missing the clear_bit()
    change.

 drivers/net/usb/usbnet.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 84f97f448b2d..5eba2cef8bac 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -329,9 +329,14 @@ void usbnet_skb_return(struct usbnet *dev, struct sk_buff *skb)
 	unsigned long flags;
 	int	status;

-	if (test_bit(EVENT_RX_PAUSED, &dev->flags)) {
-		skb_queue_tail(&dev->rxq_pause, skb);
-		return;
+	if (unlikely(test_bit(EVENT_RX_PAUSED, &dev->flags))) {
+		spin_lock_bh(&dev->rxq_pause.lock);
+		if (test_bit(EVENT_RX_PAUSED, &dev->flags)) {
+			__skb_queue_tail(&dev->rxq_pause, skb);
+			spin_unlock_bh(&dev->rxq_pause.lock);
+			return;
+		}
+		spin_unlock_bh(&dev->rxq_pause.lock);
 	}

 	/* only update if unset to allow minidriver rx_fixup override */
@@ -701,8 +706,9 @@ void usbnet_resume_rx(struct usbnet *dev)
 	struct sk_buff *skb;
 	int num = 0;

-	local_bh_disable();
+	spin_lock_bh(&dev->rxq_pause.lock);
 	clear_bit(EVENT_RX_PAUSED, &dev->flags);
+	spin_unlock_bh(&dev->rxq_pause.lock);

 	while ((skb = skb_dequeue(&dev->rxq_pause)) != NULL) {
 		usbnet_skb_return(dev, skb);
@@ -710,7 +716,6 @@ void usbnet_resume_rx(struct usbnet *dev)
 	}

 	queue_work(system_bh_wq, &dev->bh_work);
-	local_bh_enable();

 	netif_dbg(dev, rx_status, dev->net,
 		  "paused rx queue disabled, %d skbs requeued\n", num);
--
2.55.0

Re: [PATCH net v2] usbnet: fix EVENT_RX_PAUSED race in usbnet_skb_return()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v2] usbnet: fix EVENT_RX_PAUSED race in usbnet_skb_return()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usbnet: missing BH disable in usbnet_resume_rx() corrupts per-CPU stats
--

commit 36ab26ed669164df5f63bb5c4faccea95151ecd0
Author: Ömer Mete Kaya <omermetekaya0@gmail.com>

usbnet: fix EVENT_RX_PAUSED race in usbnet_skb_return()

This patch fixes a race condition where usbnet_skb_return() could pass the
EVENT_RX_PAUSED check while usbnet_resume_rx() clears the flag and drains the
queue. The fix implements a double-check pattern under the rxq_pause.lock on
the slow path to prevent skbs from being stranded.

> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index a19ecf718f36a..dc340ea248c6b 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
[ ... ]
> @@ -699,8 +704,9 @@ void usbnet_resume_rx(struct usbnet *dev)
>  	struct sk_buff *skb;
>  	int num = 0;
>  
> -	local_bh_disable();
> +	spin_lock_bh(&dev->rxq_pause.lock);
>  	clear_bit(EVENT_RX_PAUSED, &dev->flags);
> +	spin_unlock_bh(&dev->rxq_pause.lock);
>  
>  	while ((skb = skb_dequeue(&dev->rxq_pause)) != NULL) {
>  		usbnet_skb_return(dev, skb);
> @@ -708,7 +714,6 @@ void usbnet_resume_rx(struct usbnet *dev)
>  	}
>  
>  	queue_work(system_bh_wq, &dev->bh_work);
> -	local_bh_enable();

[Severity: High]
Does removing local_bh_disable() cause usbnet_skb_return() to execute with
preemption enabled here?

usbnet_resume_rx() is invoked by usbnet_change_mtu() during an MTU change,
which runs in process context with preemption enabled (holding the RTNL lock).
By removing the local_bh_disable() wrapper, usbnet_skb_return() is now called
with preemption enabled.

This leads to issues inside usbnet_skb_return(), such as evaluating a per-CPU
pointer in preemptible context, which triggers a kernel warning:

	struct pcpu_sw_netstats *stats64 = this_cpu_ptr(dev->net->tstats);

Additionally, on 64-bit systems, u64_stats_update_begin_irqsave() is a no-op
regarding preemption/interrupts. Without BH disabled, the update lacks mutual
exclusion against the network softirq on the same CPU, causing data races and
lost statistic increments:

	flags = u64_stats_update_begin_irqsave(&stats64->syncp);
	u64_stats_inc(&stats64->rx_packets);
	u64_stats_add(&stats64->rx_bytes, skb->len);
	u64_stats_update_end_irqrestore(&stats64->syncp, flags);

On 32-bit systems, if the thread migrates to another CPU after this_cpu_ptr()
but before taking the IRQ lock, it will concurrently modify a different CPU's
stats while that CPU processes softirqs. This race can corrupt the syncp
seqcount, causing stat readers (like ndo_get_stats64) to spin infinitely and
lock up the system.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006120825.342164-1-omermetekaya0@gmail.com?part=1

Re: [PATCH net v2] usbnet: fix EVENT_RX_PAUSED race in usbnet_skb_return()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This patch brings back the syzbot 'BUG: using smp_processor_id()
  in preemptible code' bug that commit 327cd4b68b43 ('usbnet: Fix using…

--

usbnet: fix EVENT_RX_PAUSED race in usbnet_skb_return()

This patch targets a race between usbnet_skb_return() and
usbnet_resume_rx(). It moves clear_bit() in usbnet_resume_rx() under
rxq_pause.lock, and usbnet_skb_return() now takes that lock to recheck
EVENT_RX_PAUSED on the slow path. As part of this, the
local_bh_disable()/local_bh_enable() pair in usbnet_resume_rx() is
replaced by a spin_lock_bh() section that covers only clear_bit().

> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index a19ecf718f36a..dc340ea248c6b 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c

[ ... ]

> @@ -699,8 +704,9 @@ void usbnet_resume_rx(struct usbnet *dev)
>  	struct sk_buff *skb;
>  	int num = 0;
>  
> -	local_bh_disable();
> +	spin_lock_bh(&dev->rxq_pause.lock);
>  	clear_bit(EVENT_RX_PAUSED, &dev->flags);
> +	spin_unlock_bh(&dev->rxq_pause.lock);
>  
>  	while ((skb = skb_dequeue(&dev->rxq_pause)) != NULL) {
>  		usbnet_skb_return(dev, skb);
> @@ -708,7 +714,6 @@ void usbnet_resume_rx(struct usbnet *dev)
>  	}
>  
>  	queue_work(system_bh_wq, &dev->bh_work);
> -	local_bh_enable();

[Severity: High]
Does this bring back the syzbot warning that was fixed by commit
327cd4b68b43 ("usbnet: Fix using smp_processor_id() in preemptible code
warnings")?

That commit added the local_bh_disable()/local_bh_enable() pair around
the drain loop and queue_work(). With this patch, BHs come back on right
after clear_bit(). The drain loop then runs in preemptible process
context when it is reached from the MTU change path:

dev_ioctl()
  dev_set_mtu()
    usbnet_change_mtu()
      usbnet_resume_rx()
        usbnet_skb_return()

usbnet_skb_return() starts with:

	struct pcpu_sw_netstats *stats64 = this_cpu_ptr(dev->net->tstats);

With CONFIG_DEBUG_PREEMPT, this should produce the same "BUG: using
smp_processor_id() in preemptible" splat that 327cd4b68b43 quotes.

Without debug options, the task could migrate after this_cpu_ptr(). It
would then update another CPU's pcpu_sw_netstats while that CPU's own BH
RX path (usbnet_bh()->rx_process()->usbnet_skb_return()) writes the same
counters. Can RX stats updates be lost this way?

On 64-bit, u64_stats_inc() and u64_stats_add() are local64 operations
and are not safe across CPUs. On 32-bit, two writers on one syncp could
corrupt the seqcount. Readers in dev_fetch_sw_netstats() could then keep
retrying.

The commit message doesn't mention that the BH protection around the
drain loop is removed. The patch also carries Fixes: 43daa96b166c, so it
could bring this back into stable trees that already have 327cd4b68b43.

Could the local_bh_disable()/local_bh_enable() pair stay around the
whole drain loop and queue_work()? Inside that region, rxq_pause.lock
could then be taken with spin_lock()/spin_unlock() just around
clear_bit().

>  
>  	netif_dbg(dev, rx_status, dev->net,
>  		  "paused rx queue disabled, %d skbs requeued\n", num);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261006120825.342164-1-omermetekaya0%40gmail.com