[PATCH 0/6] usbip: vhci_hcd: fix URB giveback leaks and disconnect races

[PATCH 0/6] usbip: vhci_hcd: fix URB giveback leaks and disconnect races

[Raul E Rangel]

This series fixes a set of URB lifecycle, locking, and disconnect
teardown bugs in drivers/usb/usbip/vhci_hcd that can orphan pending URBs
and wedge usb_kill_urb() and usb_hub_wq in uninterruptible D state when
a virtual USB port disconnects under concurrent transfer/unlink load.

Patch breakdown:
  1. Protect urb->hcpriv accesses in vhci_urb_dequeue() with
     vdev->priv_lock inside vhci->lock (matching vhci_urb_enqueue() and
     the cleanup helpers), and add a !portnum || portnum > VHCI_HC_PORTS
     bounds guard to both vhci_urb_enqueue() and vhci_urb_dequeue().
  2. Track whether an URB has been moved from vdev->priv_tx to
     vdev->priv_rx via priv->submitted under vdev->priv_lock, and
     complete unsent (!priv->submitted) URBs locally in
     vhci_urb_dequeue() instead of queuing a CMD_UNLINK PDU. This
     prevents vhci_device_unlink_cleanup() from freeing the unlink entry
     and orphaning an unsent URB on priv_tx, and prevents CMD_UNLINK
     from being transmitted before CMD_SUBMIT on an active connection.
  3. Clear vdev->ud.tcp_socket under vhci->lock and vdev->ud.lock in
     vhci_shutdown_connection(), reject vhci_urb_enqueue() once
     USBIP_EH_SHUTDOWN is set or tcp_socket is NULL, and drain all
     remaining URBs on vdev->priv_tx and vdev->priv_rx in
     vhci_device_unlink_cleanup().
  4. Move the URB_DMA_MAP_SG clearing from vhci_recv_ret_submit() into
     pickup_urb_and_free_priv() and also clear URB_DMA_MAP_SG in
     vhci_urb_dequeue() and vhci_cleanup_priv_list() so USB core does
     not call dma_unmap_sg() on unmapped scatterlists on any giveback
     path.
  5. Allocate struct vhci_priv with mem_flags before acquiring spinlocks
     in vhci_urb_enqueue(), and return 0 (with VDEV_EVENT_ERROR_MALLOC
     triggered) if struct vhci_unlink allocation fails in
     vhci_urb_dequeue() after usb_hcd_check_unlink_urb() has already
     marked urb->unlinked.
  6. Serialize vdev->udev clearing in vhci_device_reset() under
     vhci->lock and ud->lock (dropping old usb_device and socket
     references outside the spinlocks in both vhci_device_reset() and
     vhci_urb_enqueue()), and reject URBs from a stale usb_device
     (including address-0 requests from an already-registered
     usb_device undergoing USBDEVFS_RESET) if the same rhport is
     re-attached before usb_hub_wq finishes disconnecting the old
     device.

Testing:
  - Built at every commit in the series with W=1 (0 warnings, 0 errors).
  - Verified under User-Mode Linux (ARCH=um, UP with CONFIG_PREEMPT=y,
    CONFIG_PROVE_LOCKING=y, CONFIG_KASAN=y, CONFIG_DEBUG_ATOMIC_SLEEP=y,
    CONFIG_USBIP_VHCI_HCD=y, CONFIG_USBIP_DEBUG=y):
    * Test 1: Abrupt server disconnect mid-transfer while client threads
      loop 32 KiB scatter-gather (num_sgs=2, URB_DMA_MAP_SG) SUBMITURB,
      DISCARDURB, USBDEVFS_CONTROL, and USBDEVFS_RESET (5/5 cycles,
      0 D-state tasks).
    * Test 2: 500 rapid SUBMITURB + DISCARDURB + REAPURB iterations on a
      live connection, plus deterministic verification with a stalled
      tx socket that unlinking 40 unsent URBs on priv_tx completes them
      locally with 0 CMD_UNLINK PDUs sent on the wire.
    * Test 3: Immediate detach + re-attach on the same rhport while a
      client thread submits URBs on the old usbfs fd, confirming all
      stale submits fail with -ENODEV and 0 stale CMD_SUBMIT packets
      reach the new server.
    * Test 3B: Deterministic reproduction of the field deadlock sequence
      (unsent URB queued on priv_tx while tcp_socket != NULL, followed
      by concurrent USBDEVFS_RESET and server socket close), completing
      with 0 D-state tasks.
    * Test 4: Lockdep (debug_locks = 1) and KASAN clean across all runs.
  - Built and soaked for 18.5+ hours (280 cycles) on an SMP gLinux host
    (7.1.6-1rodete1-amd64) where the original usb_disconnect() /
    usb_kill_urb() deadlock was captured, completing 125,000+ EP0
    control transfers, 21,000+ async URB unlinks, 8+ GiB of
    scatter-gather USB Mass Storage bulk reads, 1,000+ mid-transfer
    SIGKILL client aborts, and 850+ live USB/IP socket disconnect
    teardowns and port re-attachments under active load with 0 kernel
    warnings and 0 hung tasks.


Raul E Rangel (6):
  usbip: vhci_hcd: protect urb->hcpriv with priv_lock in
    vhci_urb_dequeue()
  usbip: vhci_hcd: complete unsent URBs locally in vhci_urb_dequeue()
  usbip: vhci_hcd: stop enqueue and drain all pending URBs on disconnect
  usbip: vhci_hcd: clear URB_DMA_MAP_SG on all giveback paths
  usbip: vhci_hcd: allocate vhci_priv outside lock and handle ENOMEM
  usbip: vhci_hcd: reject URBs from stale usb_device after port
    re-attach

 drivers/usb/usbip/vhci.h     |   1 +
 drivers/usb/usbip/vhci_hcd.c | 236 +++++++++++++++++++++++------------
 drivers/usb/usbip/vhci_rx.c  |   6 +-
 drivers/usb/usbip/vhci_tx.c  |   1 +
 4 files changed, 162 insertions(+), 82 deletions(-)

-- 
2.56.0.212.g129ede20d9-goog

[PATCH 1/6] usbip: vhci_hcd: protect urb->hcpriv with priv_lock in vhci_urb_dequeue()

[Raul E Rangel]

In vhci_urb_dequeue(), urb->hcpriv is read and priv->vdev is
dereferenced while holding only vhci->lock, before acquiring
vdev->priv_lock. However, pickup_urb_and_free_priv() in vhci_rx.c frees
priv and clears urb->hcpriv under vdev->priv_lock without holding
vhci->lock.

If vhci_urb_dequeue() races with pickup_urb_and_free_priv() on a
completing URB, vhci_urb_dequeue() can observe a non-NULL priv pointer
just before pickup_urb_and_free_priv() frees it, leading to a
use-after-free when dereferencing priv->vdev or priv->seqnum, or a
double-free of priv.

Resolve the race by deriving vdev from urb->dev->portnum (matching
vhci_urb_enqueue(), and guarding against portnum == 0 in both) and
acquiring vdev->priv_lock (nested inside vhci->lock) before reading
urb->hcpriv.

Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
Cc: stable@vger.kernel.org
Assisted-by: Gemini
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/usb/usbip/vhci_hcd.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 39e8faf4c18c..3196ea80e7c2 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -704,7 +704,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 	struct vhci_device *vdev;
 	unsigned long flags;
 
-	if (portnum > VHCI_HC_PORTS) {
+	if (!portnum || portnum > VHCI_HC_PORTS) {
 		dev_err(hcd_dev(hcd), "invalid port number %d\n", portnum);
 		return -ENODEV;
 	}
@@ -899,34 +899,35 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	struct vhci_priv *priv;
 	struct vhci_device *vdev;
 	unsigned long flags;
+	u8 portnum = urb->dev->portnum;
+	int ret;
+
+	if (!portnum || portnum > VHCI_HC_PORTS)
+		return -ENODEV;
+	vdev = &vhci_hcd->vdev[portnum - 1];
 
 	spin_lock_irqsave(&vhci->lock, flags);
+	spin_lock(&vdev->priv_lock);
 
 	priv = urb->hcpriv;
 	if (!priv) {
 		/* URB was never linked! or will be soon given back by
 		 * vhci_rx. */
+		spin_unlock(&vdev->priv_lock);
 		spin_unlock_irqrestore(&vhci->lock, flags);
 		return -EIDRM;
 	}
 
-	{
-		int ret = 0;
-
-		ret = usb_hcd_check_unlink_urb(hcd, urb, status);
-		if (ret) {
-			spin_unlock_irqrestore(&vhci->lock, flags);
-			return ret;
-		}
+	ret = usb_hcd_check_unlink_urb(hcd, urb, status);
+	if (ret) {
+		spin_unlock(&vdev->priv_lock);
+		spin_unlock_irqrestore(&vhci->lock, flags);
+		return ret;
 	}
 
 	 /* send unlink request here? */
-	vdev = priv->vdev;
-
 	if (!vdev->ud.tcp_socket) {
 		/* tcp connection is closed */
-		spin_lock(&vdev->priv_lock);
-
 		list_del(&priv->list);
 		kfree(priv);
 		urb->hcpriv = NULL;
@@ -948,8 +949,6 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 		/* tcp connection is alive */
 		struct vhci_unlink *unlink;
 
-		spin_lock(&vdev->priv_lock);
-
 		/* setup CMD_UNLINK pdu */
 		unlink = kzalloc_obj(struct vhci_unlink, GFP_ATOMIC);
 		if (!unlink) {
-- 
2.56.0.212.g129ede20d9-goog

[PATCH 2/6] usbip: vhci_hcd: complete unsent URBs locally in vhci_urb_dequeue()

[Raul E Rangel]

When an URB is enqueued via vhci_urb_enqueue(), its vhci_priv entry is
placed on vdev->priv_tx until vhci_tx_loop() moves it to vdev->priv_rx
in dequeue_from_priv_tx() and sends USBIP_CMD_SUBMIT.

If vhci_urb_dequeue() is called while the URB is still on vdev->priv_tx
and vdev->ud.tcp_socket is non-NULL, vhci_urb_dequeue() currently sets
urb->unlinked and queues a vhci_unlink entry on vdev->unlink_tx without
removing the URB from vdev->priv_tx. This causes two bugs:

1. If vhci_tx_loop() has already exited after an earlier send error or
   been stopped during connection shutdown before tcp_socket is cleared,
   the URB remains on vdev->priv_tx. When vhci_device_unlink_cleanup()
   later runs pickup_urb_and_free_priv(), which only searches
   vdev->priv_rx, the vhci_unlink entry is freed while the URB remains
   linked on ep->urb_list with urb->unlinked set. Subsequent calls to
   vhci_urb_dequeue() from usb_hcd_flush_endpoint() fail with -EBUSY at
   usb_hcd_check_unlink_urb(), leaking the URB giveback and hanging
   usb_kill_urb() in D state forever. (Stopping new enqueues on shutdown
   and draining non-unlinked URBs are addressed in the next patch.)

2. On an active connection, if an URB is enqueued and immediately
   dequeued while vhci_tx_loop() is in vhci_send_cmd_unlink(),
   USBIP_CMD_UNLINK can be transmitted before USBIP_CMD_SUBMIT for the
   same seqnum. The server replies USBIP_RET_UNLINK for the unknown
   seqnum while vhci_tx_loop() is concurrently building or sending
   USBIP_CMD_SUBMIT, racing on urb->hcpriv or leaving an orphaned
   USBIP_RET_SUBMIT that tears down the connection with
   VDEV_EVENT_ERROR_TCP.

Record whether an URB has been dequeued from vdev->priv_tx into
vdev->priv_rx via priv->submitted under vdev->priv_lock, and complete
unsent (!priv->submitted) URBs locally in vhci_urb_dequeue() without
queuing a CMD_UNLINK request. Because vhci_tx_loop() drains
vdev->priv_tx in vhci_send_cmd_submit() before entering
vhci_send_cmd_unlink(), any URB with priv->submitted set is guaranteed
to have its CMD_SUBMIT sent before its CMD_UNLINK.

Fixes: 258c81b341c8 ("usbip: give back URBs for unsent unlink requests during cleanup")
Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
Cc: stable@vger.kernel.org
Assisted-by: Gemini
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/usb/usbip/vhci.h     |  1 +
 drivers/usb/usbip/vhci_hcd.c | 14 ++++++--------
 drivers/usb/usbip/vhci_tx.c  |  1 +
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/usbip/vhci.h b/drivers/usb/usbip/vhci.h
index 5659dce1526e..4a62f89b89c7 100644
--- a/drivers/usb/usbip/vhci.h
+++ b/drivers/usb/usbip/vhci.h
@@ -55,6 +55,7 @@ struct vhci_priv {
 
 	struct vhci_device *vdev;
 	struct urb *urb;
+	bool submitted;
 };
 
 struct vhci_unlink {
diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 3196ea80e7c2..7fadb41c01fc 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -925,20 +925,18 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 		return ret;
 	}
 
-	 /* send unlink request here? */
-	if (!vdev->ud.tcp_socket) {
-		/* tcp connection is closed */
+	/*
+	 * Give back the URB locally if the TCP connection is closed or the URB
+	 * is still on priv_tx (not yet picked up by vhci_tx_loop), so no
+	 * CMD_UNLINK is needed.
+	 */
+	if (!vdev->ud.tcp_socket || !priv->submitted) {
 		list_del(&priv->list);
 		kfree(priv);
 		urb->hcpriv = NULL;
 
 		spin_unlock(&vdev->priv_lock);
 
-		/*
-		 * If tcp connection is alive, we have sent CMD_UNLINK.
-		 * vhci_rx will receive RET_UNLINK and give back the URB.
-		 * Otherwise, we give back it here.
-		 */
 		usb_hcd_unlink_urb_from_ep(hcd, urb);
 
 		spin_unlock_irqrestore(&vhci->lock, flags);
diff --git a/drivers/usb/usbip/vhci_tx.c b/drivers/usb/usbip/vhci_tx.c
index 32e6fabccf72..997db6cfa382 100644
--- a/drivers/usb/usbip/vhci_tx.c
+++ b/drivers/usb/usbip/vhci_tx.c
@@ -40,6 +40,7 @@ static struct vhci_priv *dequeue_from_priv_tx(struct vhci_device *vdev)
 
 	list_for_each_entry_safe(priv, tmp, &vdev->priv_tx, list) {
 		list_move_tail(&priv->list, &vdev->priv_rx);
+		priv->submitted = true;
 		spin_unlock_irqrestore(&vdev->priv_lock, flags);
 		return priv;
 	}
-- 
2.56.0.212.g129ede20d9-goog

[PATCH 3/6] usbip: vhci_hcd: stop enqueue and drain all pending URBs on disconnect

[Raul E Rangel]

Even with vhci_urb_dequeue() completing unsent URBs locally when
unlinked, several races in vhci_shutdown_connection() allow URBs to be
enqueued after shutdown begins or remain stranded on vdev->priv_tx and
vdev->priv_rx across disconnect and port reset:

1. vhci_urb_enqueue() only rejects URBs when vdev->ud.status is
   VDEV_ST_NULL or VDEV_ST_ERROR. Because ud.status remains VDEV_ST_USED
   after USBIP_EH_SHUTDOWN is set and until vhci_device_reset() runs
   after vhci_shutdown_connection(), vhci_urb_enqueue() continues
   accepting new URBs onto vdev->priv_tx even after tcp_tx has stopped.

2. vhci_shutdown_connection() clears vdev->ud.tcp_socket without holding
   vhci->lock or vdev->ud.lock, racing with vhci_urb_enqueue() and
   vhci_urb_dequeue().

3. vhci_device_unlink_cleanup() only processes vdev->unlink_tx and
   vdev->unlink_rx, leaving non-unlinked URBs on vdev->priv_tx and
   vdev->priv_rx until asynchronous rh_port_disconnect() work runs on
   usb_hub_wq. If a task holds udev->dev.mutex while waiting on an URB,
   usb_disconnect() blocks on udev->dev.mutex. Moreover, because
   vhci_device_reset() immediately sets ud.status to VDEV_ST_NULL, any
   entries left on vdev->priv_tx when the port is re-attached via
   attach_store() will be transmitted by the new connection's tcp_tx
   thread to the new server.

In a live deadlock captured with drgn on a hung host:
- rx thread logged "connection closed" and set USBIP_EH_SHUTDOWN, and
  vhci_shutdown_connection() stopped tcp_rx and tcp_tx (29.6519 s)
  before clearing tcp_socket (29.6716 s).
- In kernel memory (drgn), vdev->priv_tx still held never-transmitted
  URB seqnum 17193284 with urb->unlinked = -ESHUTDOWN and
  use_count = 1, while vdev->priv_rx held unanswered URB
  seqnum 17193282 with urb->unlinked = 0 and vdev->unlink_tx/unlink_rx
  were empty.
- A concurrent USBDEVFS_RESET ioctl holding udev->dev.mutex called
  usb_hcd_flush_endpoint() -> usb_kill_urb(), blocking in D state on
  seqnum 17193284 while usb_hub_wq blocked in usb_disconnect() waiting
  for udev->dev.mutex, freezing the vhci root hub.

Fix this to guarantee all pending URBs are given back when the
connection is torn down (note that while a connection remains up,
usb_kill_urb() still waits for RET_UNLINK or connection teardown):
- Clear vdev->ud.tcp_socket under vhci->lock and vdev->ud.lock in
  vhci_shutdown_connection(), and reject vhci_urb_enqueue() (with a
  rate-limited error) when USBIP_EH_SHUTDOWN is set or tcp_socket is
  NULL.
- Drain and give back all remaining URBs on vdev->priv_tx and
  vdev->priv_rx in vhci_device_unlink_cleanup() via
  vhci_cleanup_priv_list(), using list_first_entry() loops across lock
  drops.

Fixes: 258c81b341c8 ("usbip: give back URBs for unsent unlink requests during cleanup")
Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
Cc: stable@vger.kernel.org
Assisted-by: Gemini
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/usb/usbip/vhci_hcd.c | 115 ++++++++++++++++++++++++-----------
 1 file changed, 81 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 7fadb41c01fc..78ea1fc7906e 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -727,8 +727,11 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 	/* refuse enqueue for dead connection */
 	spin_lock(&vdev->ud.lock);
 	if (vdev->ud.status == VDEV_ST_NULL ||
-	    vdev->ud.status == VDEV_ST_ERROR) {
-		dev_err(dev, "enqueue for inactive port %d\n", vdev->rhport);
+	    vdev->ud.status == VDEV_ST_ERROR ||
+	    (vdev->ud.event & USBIP_EH_SHUTDOWN) ||
+	    !vdev->ud.tcp_socket) {
+		dev_err_ratelimited(dev, "enqueue for inactive port %d\n",
+				    vdev->rhport);
 		spin_unlock(&vdev->ud.lock);
 		spin_unlock_irqrestore(&vhci->lock, flags);
 		return -ENODEV;
@@ -982,18 +985,20 @@ static void vhci_cleanup_unlink_list(struct vhci_device *vdev,
 	struct vhci_hcd *vhci_hcd = vdev_to_vhci_hcd(vdev);
 	struct usb_hcd *hcd = vhci_hcd_to_hcd(vhci_hcd);
 	struct vhci *vhci = vhci_hcd->vhci;
-	struct vhci_unlink *unlink, *tmp;
+	struct vhci_unlink *unlink;
 	unsigned long flags;
 
 	spin_lock_irqsave(&vhci->lock, flags);
 	spin_lock(&vdev->priv_lock);
 
-	list_for_each_entry_safe(unlink, tmp, unlink_list, list) {
+	while (!list_empty(unlink_list)) {
 		struct urb *urb;
 
+		unlink = list_first_entry(unlink_list, struct vhci_unlink, list);
+		list_del(&unlink->list);
+
 		urb = pickup_urb_and_free_priv(vdev, unlink->unlink_seqnum);
 		if (!urb) {
-			list_del(&unlink->list);
 			kfree(unlink);
 			continue;
 		}
@@ -1002,8 +1007,6 @@ static void vhci_cleanup_unlink_list(struct vhci_device *vdev,
 
 		usb_hcd_unlink_urb_from_ep(hcd, urb);
 
-		list_del(&unlink->list);
-
 		spin_unlock(&vdev->priv_lock);
 		spin_unlock_irqrestore(&vhci->lock, flags);
 
@@ -1019,6 +1022,50 @@ static void vhci_cleanup_unlink_list(struct vhci_device *vdev,
 	spin_unlock_irqrestore(&vhci->lock, flags);
 }
 
+/*
+ * Drain any remaining URBs still queued on priv_tx (e.g. enqueued before
+ * USBIP_EH_SHUTDOWN was set or after tcp_tx stopped) or waiting for a reply
+ * on priv_rx so no orphaned URBs remain after connection teardown.
+ */
+static void vhci_cleanup_priv_list(struct vhci_device *vdev,
+				   struct list_head *priv_list)
+{
+	struct vhci_hcd *vhci_hcd = vdev_to_vhci_hcd(vdev);
+	struct usb_hcd *hcd = vhci_hcd_to_hcd(vhci_hcd);
+	struct vhci *vhci = vhci_hcd->vhci;
+	struct vhci_priv *priv;
+	unsigned long flags;
+
+	spin_lock_irqsave(&vhci->lock, flags);
+	spin_lock(&vdev->priv_lock);
+
+	while (!list_empty(priv_list)) {
+		struct urb *urb;
+
+		priv = list_first_entry(priv_list, struct vhci_priv, list);
+		urb = priv->urb;
+
+		list_del(&priv->list);
+		kfree(priv);
+		urb->hcpriv = NULL;
+
+		urb->status = -ENODEV;
+
+		usb_hcd_unlink_urb_from_ep(hcd, urb);
+
+		spin_unlock(&vdev->priv_lock);
+		spin_unlock_irqrestore(&vhci->lock, flags);
+
+		usb_hcd_giveback_urb(hcd, urb, urb->status);
+
+		spin_lock_irqsave(&vhci->lock, flags);
+		spin_lock(&vdev->priv_lock);
+	}
+
+	spin_unlock(&vdev->priv_lock);
+	spin_unlock_irqrestore(&vhci->lock, flags);
+}
+
 static void vhci_device_unlink_cleanup(struct vhci_device *vdev)
 {
 	/* give back URB of unsent unlink request */
@@ -1026,17 +1073,26 @@ static void vhci_device_unlink_cleanup(struct vhci_device *vdev)
 
 	/* give back URB of unanswered unlink request */
 	vhci_cleanup_unlink_list(vdev, &vdev->unlink_rx);
+
+	/* give back remaining unsent and unanswered URBs */
+	vhci_cleanup_priv_list(vdev, &vdev->priv_tx);
+	vhci_cleanup_priv_list(vdev, &vdev->priv_rx);
 }
 
 /*
- * The important thing is that only one context begins cleanup.
- * This is why error handling and cleanup become simple.
- * We do not want to consider race condition as possible.
+ * Tear down an active virtual port connection. After stopping tcp_rx and
+ * tcp_tx, tcp_socket is cleared under vhci->lock and ud->lock so concurrent
+ * vhci_urb_enqueue() and vhci_urb_dequeue() calls are serialized before
+ * vhci_device_unlink_cleanup() drains all pending URBs.
  */
 static void vhci_shutdown_connection(struct usbip_device *ud)
 {
 	struct vhci_device *vdev = container_of(ud, struct vhci_device, ud);
-	struct usb_hcd *hcd = vhci_hcd_to_hcd(vdev_to_vhci_hcd(vdev));
+	struct vhci_hcd *vhci_hcd = vdev_to_vhci_hcd(vdev);
+	struct usb_hcd *hcd = vhci_hcd_to_hcd(vhci_hcd);
+	struct vhci *vhci = vhci_hcd->vhci;
+	struct socket *socket;
+	unsigned long flags;
 
 	/* need this? see stub_dev.c */
 	if (ud->tcp_socket) {
@@ -1056,34 +1112,25 @@ static void vhci_shutdown_connection(struct usbip_device *ud)
 	dev_info(hcd_dev(hcd), "stop threads\n");
 
 	/* active connection is closed */
-	if (vdev->ud.tcp_socket) {
-		sockfd_put(vdev->ud.tcp_socket);
-		vdev->ud.tcp_socket = NULL;
-		vdev->ud.sockfd = -1;
-	}
+	spin_lock_irqsave(&vhci->lock, flags);
+	spin_lock(&vdev->ud.lock);
+	socket = vdev->ud.tcp_socket;
+	vdev->ud.tcp_socket = NULL;
+	vdev->ud.sockfd = -1;
+	spin_unlock(&vdev->ud.lock);
+	spin_unlock_irqrestore(&vhci->lock, flags);
+
+	if (socket)
+		sockfd_put(socket);
 	dev_info(hcd_dev(hcd), "release socket\n");
 
 	vhci_device_unlink_cleanup(vdev);
 
 	/*
-	 * rh_port_disconnect() is a trigger of ...
-	 *   usb_disable_device():
-	 *	disable all the endpoints for a USB device.
-	 *   usb_disable_endpoint():
-	 *	disable endpoints. pending urbs are unlinked(dequeued).
-	 *
-	 * NOTE: After calling rh_port_disconnect(), the USB device drivers of a
-	 * detached device should release used urbs in a cleanup function (i.e.
-	 * xxx_disconnect()). Therefore, vhci_hcd does not need to release
-	 * pushed urbs and their private data in this function.
-	 *
-	 * NOTE: vhci_dequeue() must be considered carefully. When shutting down
-	 * a connection, vhci_shutdown_connection() expects vhci_dequeue()
-	 * gives back pushed urbs and frees their private data by request of
-	 * the cleanup function of a USB driver. When unlinking a urb with an
-	 * active connection, vhci_dequeue() does not give back the urb which
-	 * is actually given back by vhci_rx after receiving its return pdu.
-	 *
+	 * rh_port_disconnect() triggers asynchronous USB device disconnect on
+	 * usb_hub_wq (usb_disconnect() -> usb_disable_device()). All pending
+	 * URBs and unlink requests on vdev have already been unlinked and
+	 * given back synchronously by vhci_device_unlink_cleanup() above.
 	 */
 	rh_port_disconnect(vdev);
 
-- 
2.56.0.212.g129ede20d9-goog

[PATCH 4/6] usbip: vhci_hcd: clear URB_DMA_MAP_SG on all giveback paths

[Raul E Rangel]

In vhci_send_cmd_submit(), URB_DMA_MAP_SG is set on urb->transfer_flags
when urb->num_sgs is non-zero so that setup_cmd_submit_pdu() packs
USBIP_URB_DMA_MAP_SG into the wire PDU, even though vhci_hcd never maps
the scatterlist with dma_map_sg().

Currently, only vhci_recv_ret_submit() clears URB_DMA_MAP_SG before
calling usb_hcd_giveback_urb(). If an URB with scatter-gather entries
is instead given back via vhci_recv_ret_unlink(),
vhci_cleanup_unlink_list(), vhci_cleanup_priv_list(), or
vhci_urb_dequeue(), __usb_hcd_giveback_urb() invokes
usb_hcd_unmap_urb_for_dma(), which sees URB_DMA_MAP_SG and calls
dma_unmap_sg() on an unmapped scatterlist.

Move the URB_DMA_MAP_SG clearing from vhci_recv_ret_submit() into
pickup_urb_and_free_priv() (covering vhci_recv_ret_submit(),
vhci_recv_ret_unlink(), and vhci_cleanup_unlink_list()), and also clear
URB_DMA_MAP_SG in vhci_urb_dequeue() and vhci_cleanup_priv_list() so all
URB giveback paths clear the flag.

Fixes: ea44d190764b ("usbip: Implement SG support to vhci-hcd and stub driver")
Cc: stable@vger.kernel.org
Assisted-by: Gemini
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/usb/usbip/vhci_hcd.c | 6 ++++++
 drivers/usb/usbip/vhci_rx.c  | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 78ea1fc7906e..522cbaa6eb0a 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -934,6 +934,9 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	 * CMD_UNLINK is needed.
 	 */
 	if (!vdev->ud.tcp_socket || !priv->submitted) {
+		if (urb->num_sgs)
+			urb->transfer_flags &= ~URB_DMA_MAP_SG;
+
 		list_del(&priv->list);
 		kfree(priv);
 		urb->hcpriv = NULL;
@@ -1045,6 +1048,9 @@ static void vhci_cleanup_priv_list(struct vhci_device *vdev,
 		priv = list_first_entry(priv_list, struct vhci_priv, list);
 		urb = priv->urb;
 
+		if (urb->num_sgs)
+			urb->transfer_flags &= ~URB_DMA_MAP_SG;
+
 		list_del(&priv->list);
 		kfree(priv);
 		urb->hcpriv = NULL;
diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c
index a678e7c89837..7e69e4b1bace 100644
--- a/drivers/usb/usbip/vhci_rx.c
+++ b/drivers/usb/usbip/vhci_rx.c
@@ -42,6 +42,9 @@ struct urb *pickup_urb_and_free_priv(struct vhci_device *vdev, __u32 seqnum)
 				 seqnum, status);
 		}
 
+		if (urb->num_sgs)
+			urb->transfer_flags &= ~URB_DMA_MAP_SG;
+
 		list_del(&priv->list);
 		kfree(priv);
 		urb->hcpriv = NULL;
@@ -95,9 +98,6 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev,
 	if (usbip_dbg_flag_vhci_rx)
 		usbip_dump_urb(urb);
 
-	if (urb->num_sgs)
-		urb->transfer_flags &= ~URB_DMA_MAP_SG;
-
 	usbip_dbg_vhci_rx("now giveback urb %u\n", pdu->base.seqnum);
 
 	spin_lock_irqsave(&vhci->lock, flags);
-- 
2.56.0.212.g129ede20d9-goog

[PATCH 5/6] usbip: vhci_hcd: allocate vhci_priv outside lock and handle ENOMEM

[Raul E Rangel]

In vhci_urb_enqueue(), usb_hcd_link_urb_to_ep() links the URB onto the
endpoint before calling vhci_tx_urb(), which allocates struct vhci_priv
with GFP_ATOMIC while holding vhci->lock. If that allocation fails,
vhci_tx_urb() triggers VDEV_EVENT_ERROR_MALLOC and returns while
vhci_urb_enqueue() still returns 0, leaving urb->hcpriv NULL and the URB
linked on ep->urb_list where vhci_urb_dequeue() rejects it with -EIDRM.
Moreover, failing a single URB submission should return -ENOMEM to the
caller rather than tearing down the entire USB/IP connection.

Allocate struct vhci_priv in vhci_urb_enqueue() using the caller's
mem_flags before acquiring vhci->lock, pass it to vhci_tx_urb(), and
free it on early-exit and non-transmit paths.

In vhci_urb_dequeue(), if allocating struct vhci_unlink fails after
usb_hcd_check_unlink_urb() has already marked urb->unlinked, return 0
after triggering VDEV_EVENT_ERROR_MALLOC so callers know the unlink was
accepted and connection teardown will drain priv_rx and give back the
URB.

Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
Cc: stable@vger.kernel.org
Assisted-by: Gemini
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/usb/usbip/vhci_hcd.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 522cbaa6eb0a..27dba51392b7 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -665,18 +665,12 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	return retval;
 }
 
-static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev)
+static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev,
+			struct vhci_priv *priv)
 {
-	struct vhci_priv *priv;
 	struct vhci_hcd *vhci_hcd = vdev_to_vhci_hcd(vdev);
 	unsigned long flags;
 
-	priv = kzalloc_obj(struct vhci_priv, GFP_ATOMIC);
-	if (!priv) {
-		usbip_event_add(&vdev->ud, VDEV_EVENT_ERROR_MALLOC);
-		return;
-	}
-
 	spin_lock_irqsave(&vdev->priv_lock, flags);
 
 	priv->seqnum = (u32)atomic_inc_return(&vhci_hcd->seqnum);
@@ -702,6 +696,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 	u8 portnum = urb->dev->portnum;
 	int ret = 0;
 	struct vhci_device *vdev;
+	struct vhci_priv *priv;
 	unsigned long flags;
 
 	if (!portnum || portnum > VHCI_HC_PORTS) {
@@ -716,11 +711,16 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 		return -EINVAL;
 	}
 
+	priv = kzalloc_obj(struct vhci_priv, mem_flags);
+	if (!priv)
+		return -ENOMEM;
+
 	spin_lock_irqsave(&vhci->lock, flags);
 
 	if (urb->status != -EINPROGRESS) {
 		dev_err(dev, "URB already unlinked!, status %d\n", urb->status);
 		spin_unlock_irqrestore(&vhci->lock, flags);
+		kfree(priv);
 		return urb->status;
 	}
 
@@ -734,6 +734,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 				    vdev->rhport);
 		spin_unlock(&vdev->ud.lock);
 		spin_unlock_irqrestore(&vhci->lock, flags);
+		kfree(priv);
 		return -ENODEV;
 	}
 	spin_unlock(&vdev->ud.lock);
@@ -829,7 +830,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 	}
 
 out:
-	vhci_tx_urb(urb, vdev);
+	vhci_tx_urb(urb, vdev, priv);
 	spin_unlock_irqrestore(&vhci->lock, flags);
 
 	return 0;
@@ -837,6 +838,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 no_need_xmit:
 	usb_hcd_unlink_urb_from_ep(hcd, urb);
 no_need_unlink:
+	kfree(priv);
 	if (!ret) {
 		/* usb_hcd_giveback_urb() should be called with
 		 * irqs disabled
@@ -956,10 +958,18 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 		/* setup CMD_UNLINK pdu */
 		unlink = kzalloc_obj(struct vhci_unlink, GFP_ATOMIC);
 		if (!unlink) {
+			/*
+			 * Leave urb->unlinked set and return 0 so callers know
+			 * the unlink was accepted and usb_hcd_flush_endpoint()
+			 * does not busy-spin retrying unlink1(); the shutdown
+			 * triggered below (assuming usbip_event_add() can
+			 * allocate its event node) will drain priv_rx and give
+			 * back the URB.
+			 */
 			spin_unlock(&vdev->priv_lock);
 			spin_unlock_irqrestore(&vhci->lock, flags);
 			usbip_event_add(&vdev->ud, VDEV_EVENT_ERROR_MALLOC);
-			return -ENOMEM;
+			return 0;
 		}
 
 		unlink->seqnum = atomic_inc_return(&vhci_hcd->seqnum);
-- 
2.56.0.212.g129ede20d9-goog

[PATCH 6/6] usbip: vhci_hcd: reject URBs from stale usb_device after port re-attach

[Raul E Rangel]

After vhci_shutdown_connection() runs, event_handler() immediately
calls vhci_device_reset(), setting vdev->ud.status to VDEV_ST_NULL and
clearing vdev->udev before usb_hub_wq finishes running usb_disconnect()
on the detached usb_device.

If attach_store() attaches a new connection to the same rhport before
usb_hub_wq disconnects the old usb_device, vdev->ud.tcp_socket becomes
non-NULL and vdev->ud.status becomes VDEV_ST_NOTASSIGNED. Because
vhci_urb_enqueue() maps urb->dev->portnum to vdev without verifying that
urb->dev matches vdev->udev, URBs submitted on the old usb_device pass
the connection state checks and get sent to the new remote device (or,
if a concurrent USBDEVFS_RESET resets the old usb_device to address 0,
overwrite vdev->udev).

Serialize vdev->udev clearing in vhci_device_reset() under vhci->lock
and ud->lock (dropping old usb_device and socket references outside the
spinlocks in both vhci_device_reset() and vhci_urb_enqueue()), and
reject URBs in vhci_urb_enqueue() when urb->dev != vdev->udev and either
usb_pipedevice(urb->pipe) != 0 or urb->dev is already registered
(device_is_registered(&urb->dev->dev)). During initial enumeration and
PORT_INIT_TRIES retries in hub_port_connect(), each candidate usb_device
is unregistered until usb_new_device() runs after hub_port_init() has
assigned vdev->udev, whereas an already-registered usb_device resetting
via usb_reset_device() is only valid when urb->dev == vdev->udev.

Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
Cc: stable@vger.kernel.org
Assisted-by: Gemini
Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/usb/usbip/vhci_hcd.c | 44 +++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 27dba51392b7..e8ff63c2276f 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -693,6 +693,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 	struct vhci_hcd *vhci_hcd = hcd_to_vhci_hcd(hcd);
 	struct vhci *vhci = vhci_hcd->vhci;
 	struct device *dev = &urb->dev->dev;
+	struct usb_device *old = NULL;
 	u8 portnum = urb->dev->portnum;
 	int ret = 0;
 	struct vhci_device *vdev;
@@ -737,6 +738,17 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 		kfree(priv);
 		return -ENODEV;
 	}
+	if ((usb_pipedevice(urb->pipe) != 0 ||
+	     device_is_registered(&urb->dev->dev)) &&
+	    urb->dev != vdev->udev) {
+		dev_err_ratelimited(dev,
+				    "enqueue from stale usb_device on port %d\n",
+				    vdev->rhport);
+		spin_unlock(&vdev->ud.lock);
+		spin_unlock_irqrestore(&vhci->lock, flags);
+		kfree(priv);
+		return -ENODEV;
+	}
 	spin_unlock(&vdev->ud.lock);
 
 	ret = usb_hcd_link_urb_to_ep(hcd, urb);
@@ -753,7 +765,6 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 	 *
 	 */
 	if (usb_pipedevice(urb->pipe) == 0) {
-		struct usb_device *old;
 		__u8 type = usb_pipetype(urb->pipe);
 		struct usb_ctrlrequest *ctrlreq =
 			(struct usb_ctrlrequest *) urb->setup_packet;
@@ -764,13 +775,13 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 			goto no_need_xmit;
 		}
 
-		old = vdev->udev;
 		switch (ctrlreq->bRequest) {
 		case USB_REQ_SET_ADDRESS:
 			/* set_address may come when a device is reset */
 			dev_info(dev, "SetAddress Request (%d) to port %d\n",
 				 ctrlreq->wValue, vdev->rhport);
 
+			old = vdev->udev;
 			vdev->udev = usb_get_dev(urb->dev);
 			/*
 			 * NOTE: A similar operation has been done via
@@ -783,7 +794,6 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 			 * should be dropped.
 			 */
 			dev_pm_syscore_device(&vdev->udev->dev, true);
-			usb_put_dev(old);
 
 			spin_lock(&vdev->ud.lock);
 			vdev->ud.status = VDEV_ST_USED;
@@ -802,6 +812,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 				usbip_dbg_vhci_hc(
 					"Not yet?:Get_Descriptor to device 0 (get max pipe size)\n");
 
+			old = vdev->udev;
 			vdev->udev = usb_get_dev(urb->dev);
 			/*
 			 * Set syscore PM flag for the virtually attached
@@ -814,7 +825,6 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 			 * on disconnect.
 			 */
 			dev_pm_syscore_device(&vdev->udev->dev, true);
-			usb_put_dev(old);
 			goto out;
 
 		default:
@@ -832,6 +842,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 out:
 	vhci_tx_urb(urb, vdev, priv);
 	spin_unlock_irqrestore(&vhci->lock, flags);
+	usb_put_dev(old);
 
 	return 0;
 
@@ -848,6 +859,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 		spin_lock(&vhci->lock);
 	}
 	spin_unlock_irqrestore(&vhci->lock, flags);
+	usb_put_dev(old);
 	return ret;
 }
 
@@ -1156,25 +1168,31 @@ static void vhci_shutdown_connection(struct usbip_device *ud)
 static void vhci_device_reset(struct usbip_device *ud)
 {
 	struct vhci_device *vdev = container_of(ud, struct vhci_device, ud);
-	struct usb_device *old = vdev->udev;
+	struct vhci *vhci = vdev_to_vhci_hcd(vdev)->vhci;
+	struct usb_device *old;
+	struct socket *socket;
 	unsigned long flags;
 
-	spin_lock_irqsave(&ud->lock, flags);
+	spin_lock_irqsave(&vhci->lock, flags);
+	spin_lock(&ud->lock);
 
 	vdev->speed  = 0;
 	vdev->devid  = 0;
 
+	old = vdev->udev;
 	vdev->udev = NULL;
-	usb_put_dev(old);
 
-	if (ud->tcp_socket) {
-		sockfd_put(ud->tcp_socket);
-		ud->tcp_socket = NULL;
-		ud->sockfd = -1;
-	}
+	socket = ud->tcp_socket;
+	ud->tcp_socket = NULL;
+	ud->sockfd = -1;
 	ud->status = VDEV_ST_NULL;
 
-	spin_unlock_irqrestore(&ud->lock, flags);
+	spin_unlock(&ud->lock);
+	spin_unlock_irqrestore(&vhci->lock, flags);
+
+	usb_put_dev(old);
+	if (socket)
+		sockfd_put(socket);
 }
 
 static void vhci_device_unusable(struct usbip_device *ud)
-- 
2.56.0.212.g129ede20d9-goog