[PATCH 0/2] usbip: stability changes

[PATCH 0/2] usbip: stability changes

[Michael Grzeschik]

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.

Signed-off-by: Michael Grzeschik <grzeschik@puzzle.ch>
---
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 <grzeschik@puzzle.ch>

[PATCH 1/2] usbip: stub: reference count struct stub_device

[Michael Grzeschik]

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.

Signed-off-by: Michael Grzeschik <grzeschik@puzzle.ch>
---
 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(-)

diff --git a/drivers/usb/usbip/stub.h b/drivers/usb/usbip/stub.h
index d11270560c24..fa40f349d640 100644
--- a/drivers/usb/usbip/stub.h
+++ b/drivers/usb/usbip/stub.h
@@ -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);
diff --git a/drivers/usb/usbip/stub_dev.c b/drivers/usb/usbip/stub_dev.c
index abfa11d6bde7..27d1e9c8db17 100644
--- a/drivers/usb/usbip/stub_dev.c
+++ b/drivers/usb/usbip/stub_dev.c
@@ -11,6 +11,42 @@
 #include "usbip_common.h"
 #include "stub.h"
 
+/*
+ * A stub_device is referenced by its binding to the USB device (from probe
+ * until disconnect), by the rx and tx threads while they run, and by the
+ * event handler while an event for it is queued or being handled. It must
+ * not be freed before all of them are done with it: stopping the threads
+ * relies on the event handler, which can miss a shutdown request or still
+ * have an event queued when disconnect wants to free the device.
+ */
+static void stub_device_release(struct kref *kref)
+{
+	struct stub_device *sdev = container_of(kref, struct stub_device, kref);
+
+	usb_put_dev(sdev->udev);
+	kfree(sdev);
+}
+
+static void stub_device_get(struct stub_device *sdev)
+{
+	kref_get(&sdev->kref);
+}
+
+void stub_device_put(struct stub_device *sdev)
+{
+	kref_put(&sdev->kref, stub_device_release);
+}
+
+static void stub_eh_get(struct usbip_device *ud)
+{
+	stub_device_get(container_of(ud, struct stub_device, ud));
+}
+
+static void stub_eh_put(struct usbip_device *ud)
+{
+	stub_device_put(container_of(ud, struct stub_device, ud));
+}
+
 /*
  * usbip_status shows the status of usbip-host as long as this driver is bound
  * to the target device.
@@ -101,6 +137,10 @@ static ssize_t usbip_sockfd_store(struct device *dev, struct device_attribute *a
 		get_task_struct(tcp_rx);
 		get_task_struct(tcp_tx);
 
+		/* one reference per thread, dropped when the thread ends */
+		stub_device_get(sdev);
+		stub_device_get(sdev);
+
 		/* lock and update sdev->ud state */
 		spin_lock_irq(&sdev->ud.lock);
 		sdev->ud.tcp_socket = socket;
@@ -165,13 +205,18 @@ static void stub_shutdown_connection(struct usbip_device *ud)
 		kernel_sock_shutdown(ud->tcp_socket, SHUT_RDWR);
 	}
 
-	/* 1. stop threads */
+	/*
+	 * 1. stop threads. A thread that was stopped before it ran
+	 * (-EINTR) did not drop its reference itself.
+	 */
 	if (ud->tcp_rx) {
-		kthread_stop_put(ud->tcp_rx);
+		if (kthread_stop_put(ud->tcp_rx) == -EINTR)
+			stub_device_put(sdev);
 		ud->tcp_rx = NULL;
 	}
 	if (ud->tcp_tx) {
-		kthread_stop_put(ud->tcp_tx);
+		if (kthread_stop_put(ud->tcp_tx) == -EINTR)
+			stub_device_put(sdev);
 		ud->tcp_tx = NULL;
 	}
 
@@ -267,7 +312,8 @@ static struct stub_device *stub_device_alloc(struct usb_device *udev)
 	if (!sdev)
 		return NULL;
 
-	sdev->udev = udev;
+	kref_init(&sdev->kref);
+	sdev->udev = usb_get_dev(udev);
 
 	/*
 	 * devid is defined with devnum when this driver is first allocated.
@@ -294,6 +340,8 @@ static struct stub_device *stub_device_alloc(struct usb_device *udev)
 	sdev->ud.eh_ops.shutdown = stub_shutdown_connection;
 	sdev->ud.eh_ops.reset    = stub_device_reset;
 	sdev->ud.eh_ops.unusable = stub_device_unusable;
+	sdev->ud.eh_ops.get      = stub_eh_get;
+	sdev->ud.eh_ops.put      = stub_eh_put;
 
 	usbip_start_eh(&sdev->ud);
 
@@ -302,11 +350,6 @@ static struct stub_device *stub_device_alloc(struct usb_device *udev)
 	return sdev;
 }
 
-static void stub_device_free(struct stub_device *sdev)
-{
-	kfree(sdev);
-}
-
 static int stub_probe(struct usb_device *udev)
 {
 	struct stub_device *sdev = NULL;
@@ -409,7 +452,7 @@ static int stub_probe(struct usb_device *udev)
 	put_busid_priv(busid_priv);
 
 sdev_free:
-	stub_device_free(sdev);
+	stub_device_put(sdev);
 
 	return rc;
 }
@@ -489,15 +532,15 @@ static void stub_disconnect(struct usb_device *udev)
 
 	/* we already have busid_priv, just lock busid_lock */
 	spin_lock(&busid_priv->busid_lock);
-	/* free sdev */
 	busid_priv->sdev = NULL;
-	stub_device_free(sdev);
 
 	if (busid_priv->status == STUB_BUSID_ALLOC)
 		busid_priv->status = STUB_BUSID_ADDED;
 	/* release busid_lock */
 	spin_unlock(&busid_priv->busid_lock);
-	return;
+
+	/* drop the reference of the binding, threads may still hold theirs */
+	stub_device_put(sdev);
 }
 
 #ifdef CONFIG_PM
diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 1e9ae578810d..63571b690f14 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -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;
 }
diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
index 4ffd342250b5..44b3e870456c 100644
--- a/drivers/usb/usbip/stub_tx.c
+++ b/drivers/usb/usbip/stub_tx.c
@@ -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;
 }
diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
index be4c5e65a7f8..dfacd8530db8 100644
--- a/drivers/usb/usbip/usbip_common.h
+++ b/drivers/usb/usbip/usbip_common.h
@@ -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;
diff --git a/drivers/usb/usbip/usbip_event.c b/drivers/usb/usbip/usbip_event.c
index 0e00c2d000f8..eea9a71f0eac 100644
--- a/drivers/usb/usbip/usbip_event.c
+++ b/drivers/usb/usbip/usbip_event.c
@@ -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

[PATCH 2/2] usbip: identify the event handler with current_work()

[Michael Grzeschik]

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.

Signed-off-by: Michael Grzeschik <grzeschik@puzzle.ch>
---
 drivers/usb/usbip/usbip_event.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/usbip/usbip_event.c b/drivers/usb/usbip/usbip_event.c
index eea9a71f0eac..e00bc535a822 100644
--- a/drivers/usb/usbip/usbip_event.c
+++ b/drivers/usb/usbip/usbip_event.c
@@ -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

Re: [PATCH 2/2] usbip: identify the event handler with current_work()

[John Ernberg]

Hi Michael,

On 11/11/2026 12:34, Michael Grzeschik wrote:
> 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.
> 
> Signed-off-by: Michael Grzeschik <grzeschik@puzzle.ch>

We sent a similar patch back in August that we forgot to follow up on. [1]

Reviewed-by: John Ernberg <john.ernberg@actia.se>

[1]: 
https://lore.kernel.org/linux-usb/20260803104342.2790105-1-john.ernberg@actia.se/

Thanks! // John Ernberg

Re: [PATCH 2/2] usbip: identify the event handler with current_work()

[Michael Grzeschik]

[-- Attachment #1.1: Type: text/plain, Size: 1214 bytes --]

Hi John

On 2026-11-11 4:35 PM, John Ernberg wrote:
> Hi Michael,
> 
> On 11/11/2026 12:34, Michael Grzeschik wrote:
>> 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.
>>
>> Signed-off-by: Michael Grzeschik <grzeschik@puzzle.ch>
> 
> We sent a similar patch back in August that we forgot to follow up on. [1]
> 
> Reviewed-by: John Ernberg <john.ernberg@actia.se>
> 
> [1]: https://lore.kernel.org/linux-usb/20260803104342.2790105-1-john.ernberg@actia.se/

Damn, I really should have checked the usb list better before sending my change.
Please lets use your change then. I will just send my Reviewed-by and push for your contribution.

So please ignore this mail.

Thanks,
Michael

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]