net: mctp: usb: fix race between urb completion and rx_retry cancellation

It's possible that sequencing between setting ->stopped and cancelling
the rx_retry work (in ndo_stop) could leave us with an urb queued:

    T1: ndo_stop                  T2: rx_retry_work
    ------------                  ----------------
                                  LD: ->stopped => false
    ST: ->stopped <= true
    usb_kill_urb()
                                  mctp_usb_rx_queue()
                                    usb_submit_urb()
    cancel_delayed_work_sync()

That urb completion can then re-schedule rx_retry_work.

Strenghen the sequencing between the stop (preventing another requeue)
and the cancel by updating both atomically under a new rx lock. After
setting ->rx_stopped, and cancelling pending work, we know that the
requeue cannot occur, so all that's left is killing any pending urb.

Fixes: 0791c0327a6e ("net: mctp: Add MCTP USB transport driver")
Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
Link: https://patch.msgid.link/20260608-dev-mctp-usb-rx-requeue-v2-1-29a3aa507609@codeconstruct.com.au
Signed-off-by: Paolo Abeni <pabeni@redhat.com>

diff --git a/drivers/net/mctp/mctp-usb.c b/drivers/net/mctp/mctp-usb.c
index 3b5dff144177470b039c0d5160a3698d2c5b6953..cf6f6a93a45112233bd38db0c875a4db8c8f77dd 100644 (file)
--- a/drivers/net/mctp/mctp-usb.c
+++ b/drivers/net/mctp/mctp-usb.c
@@ -22,7 +22,6 @@
 struct mctp_usb {
        struct usb_device *usbdev;
        struct usb_interface *intf;
-       bool stopped;
 
        struct net_device *netdev;
 
@@ -32,6 +31,9 @@ struct mctp_usb {
        struct urb *tx_urb;
        struct urb *rx_urb;
 
+       /* enforces atomic access to rx_stopped and requeuing the retry work */
+       spinlock_t rx_lock;
+       bool rx_stopped;
        struct delayed_work rx_retry_work;
 };
 
@@ -122,6 +124,7 @@ static const unsigned long RX_RETRY_DELAY = HZ / 4;
 
 static int mctp_usb_rx_queue(struct mctp_usb *mctp_usb, gfp_t gfp)
 {
+       unsigned long flags;
        struct sk_buff *skb;
        int rc;
 
@@ -147,7 +150,10 @@ static int mctp_usb_rx_queue(struct mctp_usb *mctp_usb, gfp_t gfp)
        return rc;
 
 err_retry:
-       schedule_delayed_work(&mctp_usb->rx_retry_work, RX_RETRY_DELAY);
+       spin_lock_irqsave(&mctp_usb->rx_lock, flags);
+       if (!mctp_usb->rx_stopped)
+               schedule_delayed_work(&mctp_usb->rx_retry_work, RX_RETRY_DELAY);
+       spin_unlock_irqrestore(&mctp_usb->rx_lock, flags);
        return rc;
 }
 
@@ -248,9 +254,6 @@ static void mctp_usb_rx_retry_work(struct work_struct *work)
        struct mctp_usb *mctp_usb = container_of(work, struct mctp_usb,
                                                 rx_retry_work.work);
 
-       if (READ_ONCE(mctp_usb->stopped))
-               return;
-
        mctp_usb_rx_queue(mctp_usb, GFP_KERNEL);
 }
 
@@ -258,7 +261,7 @@ static int mctp_usb_open(struct net_device *dev)
 {
        struct mctp_usb *mctp_usb = netdev_priv(dev);
 
-       WRITE_ONCE(mctp_usb->stopped, false);
+       WRITE_ONCE(mctp_usb->rx_stopped, false);
 
        netif_start_queue(dev);
 
@@ -268,17 +271,21 @@ static int mctp_usb_open(struct net_device *dev)
 static int mctp_usb_stop(struct net_device *dev)
 {
        struct mctp_usb *mctp_usb = netdev_priv(dev);
+       unsigned long flags;
 
        netif_stop_queue(dev);
 
        /* prevent RX submission retry */
-       WRITE_ONCE(mctp_usb->stopped, true);
+       spin_lock_irqsave(&mctp_usb->rx_lock, flags);
+       mctp_usb->rx_stopped = true;
+       cancel_delayed_work(&mctp_usb->rx_retry_work);
+       spin_unlock_irqrestore(&mctp_usb->rx_lock, flags);
+
+       flush_delayed_work(&mctp_usb->rx_retry_work);
 
        usb_kill_urb(mctp_usb->rx_urb);
        usb_kill_urb(mctp_usb->tx_urb);
 
-       cancel_delayed_work_sync(&mctp_usb->rx_retry_work);
-
        return 0;
 }
 
@@ -331,6 +338,7 @@ static int mctp_usb_probe(struct usb_interface *intf,
        dev->netdev = netdev;
        dev->usbdev = interface_to_usbdev(intf);
        dev->intf = intf;
+       spin_lock_init(&dev->rx_lock);
        usb_set_intfdata(intf, dev);
 
        dev->ep_in = ep_in->bEndpointAddress;