usbip: stability changes

While using usbip on kirkwood embedded machine, I came accross some stability issues. This series is addressing the reference counting on and usage of the event handler itself.

---
Michael Grzeschik (2):
usbip: stub: reference count struct stub_device
usbip: identify the event handler with current_work()

 drivers/usb/usbip/stub.h         |  3 ++
 drivers/usb/usbip/stub_dev.c     | 69 ++++++++++++++++++++++++++++++++--------
 drivers/usb/usbip/stub_rx.c      |  3 ++
 drivers/usb/usbip/stub_tx.c      |  3 ++
 drivers/usb/usbip/usbip_common.h |  6 ++++
 drivers/usb/usbip/usbip_event.c  | 17 +++++-----
 6 files changed, 79 insertions(+), 22 deletions(-)

---
base-commit: 067c5c24cef23d913b67eaaf71ae67e7169a6620
change-id: 20261111-usbip-fcb6d02254a7

Best regards,

Michael Grzeschik <[email protected]>

A stub_device is freed by stub_disconnect() right after usbip_stop_eh() reported that the event handler is done with it. The rx and tx threads and the event handler can still use it at that point:

- The event handler clears an event bit after it ran the handler for it. A SHUTDOWN requested while a previous shutdown is running is lost, so threads started after that shutdown are not stopped before the stub_device is freed.
- An event queued for the device while another one is handled leaves an entry in the event list. Once the event bits are clear, usbip_stop_eh() returns, the stub_device is freed, and the handler then locks the sysfs_lock of the freed device.
- The stub_device no longer holds a reference to the usb_device, so a thread outliving the disconnect touches a freed usb_device too.

This was seen exporting the serial port of an ST-Link from an STM32MP1-DK2 on a Kirkwood (ARMv5) host. After repeated "endpoint 5 is stalled" / usb_clear_halt cycles while a client opened and closed the port, stub_rx crashed in dev_err() with the device pointer computed from a NULL sdev->udev of freed memory:

  Unable to handle kernel NULL pointer dereference at virtual address 000000b8 when read
  PC is at dev_driver_string+0x0/0x38
  LR is at __dev_printk+0x20/0x6c
  Call trace:
   dev_driver_string from __dev_printk+0x20/0x6c
   __dev_printk from _dev_err+0x40/0x74
   _dev_err from stub_rx_loop+0x5d8/0xd68 [usbip_host]
   stub_rx_loop [usbip_host] from kthread+0x100/0x120

Reference count the stub_device instead of relying on the event handler for its lifetime:

- the binding (probe until disconnect) holds the initial reference, stub_disconnect() drops it instead of freeing the device
- the rx and tx thread hold one each while they run, taken before they are woken and dropped when they end. If kthread_stop() reports that a thread never ran, stub_shutdown_connection() drops it for the thread.
- the event handler holds one per queued event until it is completely done with the device, through new optional get/put event handler operations, which vhci and vudc do not set
- the stub_device holds a reference to its usb_device again, dropped with the last reference

Threads that miss their shutdown now end when the connection goes away instead of using freed memory.

---
 drivers/usb/usbip/stub.h         |  3 ++
 drivers/usb/usbip/stub_dev.c     | 69 ++++++++++++++++++++++++++++++++--------
 drivers/usb/usbip/stub_rx.c      |  3 ++
 drivers/usb/usbip/stub_tx.c      |  3 ++
 drivers/usb/usbip/usbip_common.h |  6 ++++
 drivers/usb/usbip/usbip_event.c  |  6 ++++
 6 files changed, 77 insertions(+), 13 deletions(-)

index d11270560c24..fa40f349d640 100644
@@ -6,6 +6,7 @@
 #ifndef __USBIP_STUB_H
 #define __USBIP_STUB_H
 
+#include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>

@@ -20,6 +21,7 @@
 
 struct stub_device {
 	struct usb_device *udev;
+	struct kref kref;
 
 	struct usbip_device ud;
 	__u32 devid;

@@ -85,6 +87,7 @@ extern struct kmem_cache *stub_priv_cache;
 
 /* stub_dev.c */
 extern struct usb_device_driver stub_driver;
+void stub_device_put(struct stub_device *sdev);
 
 /* stub_main.c */
 struct bus_id_priv *get_busid_priv(const char *busid);

index 1e9ae578810d..63571b690f14 100644
@@ -707,5 +707,8 @@ int stub_rx_loop(void *data)
 		stub_rx_pdu(ud);
 	}
 
+	/* drop the reference taken in usbip_sockfd_store() */
+	stub_device_put(container_of(ud, struct stub_device, ud));
+
 	return 0;
 }

index 4ffd342250b5..44b3e870456c 100644
@@ -446,5 +446,8 @@ int stub_tx_loop(void *data)
 					  kthread_should_stop()));
 	}
 
+	/* drop the reference taken in usbip_sockfd_store() */
+	stub_device_put(sdev);
+
 	return 0;
 }

index be4c5e65a7f8..dfacd8530db8 100644
@@ -280,6 +280,12 @@ struct usbip_device {
 		void (*shutdown)(struct usbip_device *);
 		void (*reset)(struct usbip_device *);
 		void (*unusable)(struct usbip_device *);
+		/*
+		 * Optional: hold a reference while an event for this device is
+		 * queued for or being handled by the event handler.
+		 */
+		void (*get)(struct usbip_device *ud);
+		void (*put)(struct usbip_device *ud);
 	} eh_ops;
 
 	struct kcov_common_handle_id kcov_handle;

index 0e00c2d000f8..eea9a71f0eac 100644
@@ -94,6 +94,10 @@ static void event_handler(struct work_struct *work)
 		mutex_unlock(&ud->sysfs_lock);
 
 		wake_up(&ud->eh_waitq);
+
+		/* drop the reference taken by usbip_event_add() */
+		if (ud->eh_ops.put)
+			ud->eh_ops.put(ud);
 	}
 }
 

@@ -163,6 +167,8 @@ void usbip_event_add(struct usbip_device *ud, unsigned long event)
 		goto out;
 
 	ue->ud = ud;
+	if (ud->eh_ops.get)
+		ud->eh_ops.get(ud);
 
 	list_add_tail(&ue->node, &event_list);
 	queue_work(usbip_queue, &usbip_work);

2.53.0

usbip_in_eh() compares the task with worker_context, the first worker that ever ran the event handler. Workers of the unbound usbip_event workqueue are reaped when idle and new ones run the handler later, so the check fails inside the event handler, and the remembered task_struct may be reused by any other task, which then wrongly counts as the event handler. stub_disconnect() relies on it to skip the shutdown of a device reset from the event handler.

Ask the workqueue which work item the current task executes instead.

---
 drivers/usb/usbip/usbip_event.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

index eea9a71f0eac..e00bc535a822 100644
@@ -57,16 +57,11 @@ static struct usbip_device *get_event(void)
 	return ud;
 }
 
-static struct task_struct *worker_context;
 
 static void event_handler(struct work_struct *work)
 {
 	struct usbip_device *ud;
 
-	if (worker_context == NULL) {
-		worker_context = current;
-	}
-
 	while ((ud = get_event()) != NULL) {
 		usbip_dbg_eh("pending event %lx\n", ud->event);
 

@@ -194,9 +189,7 @@ EXPORT_SYMBOL_GPL(usbip_event_happened);
 
 int usbip_in_eh(struct task_struct *task)
 {
-	if (task == worker_context)
-		return 1;
-
-	return 0;
+	/* only the worker running usbip_work is the event handler */
+	return task == current && current_work() == &usbip_work;
 }
 EXPORT_SYMBOL_GPL(usbip_in_eh);

2.53.0