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

[sashiko-bot@kernel.org]

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