[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@xxxxxxxxxxxxxxx
Assisted-by: Gemini
Signed-off-by: Raul E Rangel <rrangel@xxxxxxxxxxxx>
---

 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