Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
From:   Simon Holesch <simon@holesch.de>
To:     Valentina Manea <valentina.manea.m@gmail.com>,
        Shuah Khan <shuah@kernel.org>, Hongren Zheng <i@zenithal.me>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Simon Holesch <simon@holesch.de>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: [PATCH] usbip: Don't submit special requests twice
Date:   Mon, 27 Nov 2023 00:48:38 +0100
Message-ID: <20231126234839.52434-1-simon@holesch.de>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
UI-OutboundReport: notjunk:1;M01:P0:OYL6BJpPPx0=;lyOfkxQQ8fFqrfsKObRnto6BMwt
 sxEQQlL2c5tBX6MphCW5ku3M+7I5iqoS+TX38lSsU9Cqr5mOE6kWl3qQSkR/10ljSXwSlm5g1
 8Tr96lLS/hjrw1E2OnRqRvJUQLN/6cVnU8UY5MxK7IZ5lkDxYc+vyjc1qaGaDSsrA/u3wJN0A
 aQ1kdA/SAodc6CVnQ7agFFSFU85qmC1zk8G9VG87s8n09OpmkLYVEjGO13q6MrWhUbGhJOYlF
 9bpXjEj8JtEmfOa7zaWiO59kH1ksNLQQD4VjbwJXaqpK0vPOanPtxZAiK+nTHLP/Gh0YdRpoj
 bGcgYqgOmaXksPfzyulsu6vsRglxhXXORq/0V0HwDxcmMVX0hz1VnhMNf7zGiDsVhycCymZrc
 zT9t4AMGrW7aSgRmT6y0KAEuQxT9YaCy664RZgaLRyjVV2Fa8p4rJ+mr5sDClrjOBoH8Q8KDM
 454S+ezi+FGjNTT/33iZ1IoNJNB5NXLCrRLia9uLnfHg1fcRqopCPeBMJKk/FZY9V+dFCs4Vj
 yqpKHUxXsDK9IL5Rhm00JJXjvmkLAHV3fxWNNuzNLZGSFXvtTSykzSZqX7nYD700M/tW1hx6i
 gUTRTm5dCSKRQWO/ijKVz26sneVm68w9Ek3Jy98/L5IE1TX3YFIsvJ7l1Bcyw4wj5qZPsspSb
 2resWfRF98wu3rffBHZiF0VYhWoVWY7e6lZ3+TNafiNgm6/99uvCSet0XyqG26gPVU04ukyOf
 GOerVaPG1og+OBHSiLahszwA0BS6QtZWyJlYhTiAymTT3Ypjc47x/N9crX0tUaFtBVnxXLJk9
 oEsBVi8sk+AOMBjqB8RpYG7+fSpSqHK6j6vuzZjoVDKLTt6dfv0E1v9ywbymVwZWiGY6K0qJ2
 8xa8FIg8q34ukFg==
Precedence: bulk

Even though submitting those requests twice should be harmless, there
are USB devices that react poorly to some duplicated requests.

One example is the ChipIdea controller implementation in U-Boot: The
second SET_CONFIURATION request makes U-Boot disable and re-enable all
endpoints. Re-enabling an endpoint in the ChipIdea controller, however,
was broken until U-Boot commit b272c8792502 ("usb: ci: Fix gadget
reinit").

Signed-off-by: Simon Holesch <simon@holesch.de>
---
 drivers/usb/usbip/stub_rx.c | 50 ++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index fc01b31bbb87..f3fcc290ce5b 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -167,13 +167,13 @@ static int tweak_reset_device_cmd(struct urb *urb)
 /*
  * clear_halt, set_interface, and set_configuration require special tricks.
  */
-static void tweak_special_requests(struct urb *urb)
+static int tweak_special_requests(struct urb *urb)
 {
 	if (!urb || !urb->setup_packet)
-		return;
+		return 0;
 
 	if (usb_pipetype(urb->pipe) != PIPE_CONTROL)
-		return;
+		return 0;
 
 	if (is_clear_halt_cmd(urb))
 		/* tweak clear_halt */
@@ -189,8 +189,12 @@ static void tweak_special_requests(struct urb *urb)
 
 	else if (is_reset_device_cmd(urb))
 		tweak_reset_device_cmd(urb);
-	else
+	else {
 		usbip_dbg_stub_rx("no need to tweak\n");
+		return 0;
+	}
+
+	return 1;
 }
 
 /*
@@ -468,6 +472,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
 	int support_sg = 1;
 	int np = 0;
 	int ret, i;
+	int is_tweaked;
 
 	if (pipe == -1)
 		return;
@@ -580,8 +585,7 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
 		priv->urbs[i]->pipe = pipe;
 		priv->urbs[i]->complete = stub_complete;
 
-		/* no need to submit an intercepted request, but harmless? */
-		tweak_special_requests(priv->urbs[i]);
+		is_tweaked = tweak_special_requests(priv->urbs[i]);
 
 		masking_bogus_flags(priv->urbs[i]);
 	}
@@ -594,22 +598,32 @@ static void stub_recv_cmd_submit(struct stub_device *sdev,
 
 	/* urb is now ready to submit */
 	for (i = 0; i < priv->num_urbs; i++) {
-		ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
+		if (!is_tweaked) {
+			ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL);
 
-		if (ret == 0)
-			usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
-					pdu->base.seqnum);
-		else {
-			dev_err(&udev->dev, "submit_urb error, %d\n", ret);
-			usbip_dump_header(pdu);
-			usbip_dump_urb(priv->urbs[i]);
+			if (ret == 0)
+				usbip_dbg_stub_rx("submit urb ok, seqnum %u\n",
+						pdu->base.seqnum);
+			else {
+				dev_err(&udev->dev, "submit_urb error, %d\n", ret);
+				usbip_dump_header(pdu);
+				usbip_dump_urb(priv->urbs[i]);
 
+				/*
+				 * Pessimistic.
+				 * This connection will be discarded.
+				 */
+				usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
+				break;
+			}
+		} else {
 			/*
-			 * Pessimistic.
-			 * This connection will be discarded.
+			 * An identical URB was already submitted in
+			 * tweak_special_requests(). Skip submitting this URB to not
+			 * duplicate the request.
 			 */
-			usbip_event_add(ud, SDEV_EVENT_ERROR_SUBMIT);
-			break;
+			priv->urbs[i]->status = 0;
+			stub_complete(priv->urbs[i]);
 		}
 	}
 
-- 
2.43.0