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

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

[Ömer Mete Kaya]

On SMP, usbnet_skb_return() can observe EVENT_RX_PAUSED as set on CPU0
while usbnet_resume_rx() clears the flag and drains rxq_pause on CPU1.
The skb can then be queued after the drain has completed and remain
stranded until the next MTU change or device stop.

Synchronize the EVENT_RX_PAUSED state transition and paused-queue
dequeue operations with rxq_pause.lock, dropping the lock around each
usbnet_skb_return() call to avoid lock inversion. In usbnet_skb_return()
use a double-check pattern under rxq_pause.lock on the slow path,
keeping the normal RX fast path lock-free. Use spin_lock_irqsave() to
match the lockdep class shared with other sk_buff_head locks taken from
hardirq context in defer_bh().

Discovered by Sashiko, not triggered, not runtime-tested.

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

---
v4: Use spin_lock_irqsave()/spin_unlock_irqrestore() to match lockdep
    class shared with other sk_buff_head locks taken from hardirq
    context in defer_bh(); restore local_bh_disable()/local_bh_enable()
    around drain loop to preserve BH context.

 drivers/net/usb/usbnet.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 84f97f448b2d..9c4a23d8cb86 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_irqsave(&dev->rxq_pause.lock, flags);
+		if (test_bit(EVENT_RX_PAUSED, &dev->flags)) {
+			__skb_queue_tail(&dev->rxq_pause, skb);
+			spin_unlock_irqrestore(&dev->rxq_pause.lock, flags);
+			return;
+		}
+		spin_unlock_irqrestore(&dev->rxq_pause.lock, flags);
 	}

 	/* only update if unset to allow minidriver rx_fixup override */
@@ -700,15 +705,18 @@ void usbnet_resume_rx(struct usbnet *dev)
 {
 	struct sk_buff *skb;
 	int num = 0;
+	unsigned long flags;

 	local_bh_disable();
+	spin_lock_irqsave(&dev->rxq_pause.lock, flags);
 	clear_bit(EVENT_RX_PAUSED, &dev->flags);
-
-	while ((skb = skb_dequeue(&dev->rxq_pause)) != NULL) {
+	while ((skb = __skb_dequeue(&dev->rxq_pause)) != NULL) {
+		spin_unlock_irqrestore(&dev->rxq_pause.lock, flags);
 		usbnet_skb_return(dev, skb);
+		spin_lock_irqsave(&dev->rxq_pause.lock, flags);
 		num++;
 	}
-
+	spin_unlock_irqrestore(&dev->rxq_pause.lock, flags);
 	queue_work(system_bh_wq, &dev->bh_work);
 	local_bh_enable();

--
2.56.0

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

[sashiko-bot]

> On SMP, usbnet_skb_return() can observe EVENT_RX_PAUSED as set on CPU0
> while usbnet_resume_rx() clears the flag and drains rxq_pause on CPU1.
> The skb can then be queued after the drain has completed and remain
> stranded until the next MTU change or device stop.
> 
> Synchronize the EVENT_RX_PAUSED state transition and paused-queue
> dequeue operations with rxq_pause.lock, dropping the lock around each
> usbnet_skb_return() call to avoid lock inversion. In usbnet_skb_return()
> use a double-check pattern under rxq_pause.lock on the slow path,
> keeping the normal RX fast path lock-free. Use spin_lock_irqsave() to
> match the lockdep class shared with other sk_buff_head locks taken from
> hardirq context in defer_bh().
> 
> Discovered by Sashiko, not triggered, not runtime-tested.
> 
> Fixes: 43daa96b166c ("usbnet: Stop RX Q on MTU change")
> Signed-off-by: Ömer Mete Kaya <omermetekaya0@gmail.com>

Sashiko has reviewed this patch and found no issues.

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