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