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

[Raul E Rangel <rrangel@chromium.org>]

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