How to safely disconnect an usbnet device

Commit Message

Oliver Neukum June 27, 2022, 2:05 p.m. UTC

Hi,

looking at the race conditions with respect to works, timers and bottom
halves it looks to me like me it needs the following algorithm (in
pseudocode):

1. Set a flag
2. Cancel everything
3. Recancel everything

while checking for the flag while every time a work is scheduled,
a timer armed or something similar.
By that logic anything that escapes the first round will be caught
in the second round. What do you think of his patch?

	Regards
		Oliver

Patch

From 15c537de5e6e9499b964f8337b22ae183742cbb5 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <[e-mail]>
Date: Mon, 27 Jun 2022 16:03:37 +0200
Subject: [PATCH] usbnet: fix cyclical race on disconnect

There is a cyclical dependency that must be broken.

Signed-off-by: Oliver Neukum <[e-mail]>
---
 drivers/net/usb/usbnet.c   | 28 +++++++++++++++++++++++-----
 include/linux/usb/usbnet.h | 18 ++++++++++++++++++
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 7835890df6d3..fcf1a3b9db12 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -501,7 +501,8 @@  static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
 		skb = __netdev_alloc_skb_ip_align(dev->net, size, flags);
 	if (!skb) {
 		netif_dbg(dev, rx_err, dev->net, "no rx skb\n");
-		usbnet_defer_kevent (dev, EVENT_RX_MEMORY);
+		if (!usbnet_going_away(dev))
+			usbnet_defer_kevent (dev, EVENT_RX_MEMORY);
 		usb_free_urb (urb);
 		return -ENOMEM;
 	}
@@ -518,6 +519,7 @@  static int rx_submit (struct usbnet *dev, struct urb *urb, gfp_t flags)
 
 	if (netif_running (dev->net) &&
 	    netif_device_present (dev->net) &&
+	    !usbnet_going_away(dev) &&
 	    test_bit(EVENT_DEV_OPEN, &dev->flags) &&
 	    !test_bit (EVENT_RX_HALT, &dev->flags) &&
 	    !test_bit (EVENT_DEV_ASLEEP, &dev->flags)) {
@@ -854,6 +856,16 @@  int usbnet_stop (struct net_device *net)
 	del_timer_sync (&dev->delay);
 	tasklet_kill (&dev->bh);
 	cancel_work_sync(&dev->kevent);
+
+	/*
+	 * we have cyclic dependencies. Those calls are needed
+	 * to break a cycle. We cannot fall into the gaps because
+	 * we have a flag
+	 */
+	tasklet_kill (&dev->bh);
+	del_timer_sync (&dev->delay);
+	cancel_work_sync(&dev->kevent);
+
 	if (!pm)
 		usb_autopm_put_interface(dev->intf);
 
@@ -1179,7 +1191,8 @@  usbnet_deferred_kevent (struct work_struct *work)
 					   status);
 		} else {
 			clear_bit (EVENT_RX_HALT, &dev->flags);
-			tasklet_schedule (&dev->bh);
+			if (!usbnet_going_away(dev))
+				tasklet_schedule (&dev->bh);
 		}
 	}
 
@@ -1201,10 +1214,13 @@  usbnet_deferred_kevent (struct work_struct *work)
 			}
 			if (rx_submit (dev, urb, GFP_KERNEL) == -ENOLINK)
 				resched = 0;
-			usb_autopm_put_interface(dev->intf);
 fail_lowmem:
-			if (resched)
+			usb_autopm_put_interface(dev->intf);
+			if (resched && !usbnet_going_away(dev)) {
+				set_bit (EVENT_RX_MEMORY, &dev->flags);
+
 				tasklet_schedule (&dev->bh);
+			}
 		}
 	}
 
@@ -1217,13 +1233,13 @@  usbnet_deferred_kevent (struct work_struct *work)
 		if (status < 0)
 			goto skip_reset;
 		if(info->link_reset && (retval = info->link_reset(dev)) < 0) {
-			usb_autopm_put_interface(dev->intf);
 skip_reset:
 			netdev_info(dev->net, "link reset failed (%d) usbnet usb-%s-%s, %s\n",
 				    retval,
 				    dev->udev->bus->bus_name,
 				    dev->udev->devpath,
 				    info->description);
+			usb_autopm_put_interface(dev->intf);
 		} else {
 			usb_autopm_put_interface(dev->intf);
 		}
@@ -1560,6 +1576,7 @@  static void usbnet_bh (struct timer_list *t)
 	} else if (netif_running (dev->net) &&
 		   netif_device_present (dev->net) &&
 		   netif_carrier_ok(dev->net) &&
+		   !usbnet_going_away(dev) &&
 		   !timer_pending(&dev->delay) &&
 		   !test_bit(EVENT_RX_PAUSED, &dev->flags) &&
 		   !test_bit(EVENT_RX_HALT, &dev->flags)) {
@@ -1606,6 +1623,7 @@  void usbnet_disconnect (struct usb_interface *intf)
 	usb_set_intfdata(intf, NULL);
 	if (!dev)
 		return;
+	usbnet_mark_going_away(dev);
 
 	xdev = interface_to_usbdev (intf);
 
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 1b4d72d5e891..8b568ae65366 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -84,8 +84,26 @@  struct usbnet {
 #		define EVENT_LINK_CHANGE	11
 #		define EVENT_SET_RX_MODE	12
 #		define EVENT_NO_IP_ALIGN	13
+/*
+ * this one is special, as it indicates that the device is going away
+ * there are cyclic dependencies between tasklet, timer and bh
+ * that must be broken
+ */
+#		define EVENT_UNPLUG		31
 };
 
+static inline bool usbnet_going_away(struct usbnet *ubn)
+{
+	smp_mb__before_atomic();
+	return test_bit(EVENT_UNPLUG, &ubn->flags);
+}
+
+static inline void usbnet_mark_going_away(struct usbnet *ubn)
+{
+	set_bit(EVENT_UNPLUG, &ubn->flags);
+	smp_mb__after_atomic();
+}
+
 static inline struct usb_driver *driver_of(struct usb_interface *intf)
 {
 	return to_usb_driver(intf->dev.driver);
-- 
2.35.3