[PATCH] usb: usbip: stub_rx: remove outdated comment

[PATCH] usb: usbip: stub_rx: remove outdated comment

[Oliver Neukum]

After removal of the BUG() statement the error exit can be reached
and logs a report. Remove a comment saying otherwise.

Fixes: 635f545a7e8be ("usbip: fix stub_rx: get_pipe() to validate endpoint number")
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/usb/usbip/stub_rx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 1e9ae578810d..0ccc05b76c5a 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -391,7 +391,6 @@ static int get_pipe(struct stub_device *sdev, struct usbip_header *pdu)
 	}
 
 err_ret:
-	/* NOT REACHED */
 	dev_err(&sdev->udev->dev, "CMD_SUBMIT: invalid epnum %d\n", epnum);
 	return -1;
 }
-- 
2.55.0

Re: [PATCH] usb: usbip: stub_rx: remove outdated comment

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usbip: protocol desync and hung URBs on invalid endpoint
--

commit f15a0a944a703fedd99ceab9d659df84a495556c
Author: Oliver Neukum <oneukum@suse.com>
Subject: usb: usbip: stub_rx: remove outdated comment

This patch removes an outdated comment in the error handling path of
get_pipe(). The comment indicated the code was unreachable, but following the
removal of a BUG() statement, this path can now be reached and logs a report.

> diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
> index 1e9ae578810d9..0ccc05b76c5ab 100644
> --- a/drivers/usb/usbip/stub_rx.c
> +++ b/drivers/usb/usbip/stub_rx.c
> @@ -391,7 +391,6 @@ static int get_pipe(struct stub_device *sdev, struct usbip_header *pdu)
>  	}
>  
>  err_ret:
> -	/* NOT REACHED */
>  	dev_err(&sdev->udev->dev, "CMD_SUBMIT: invalid epnum %d\n", epnum);
>  	return -1;

[Severity: High]
When get_pipe() returns -1, does it cause a protocol desync regression in
stub_recv_cmd_submit()?

If a USBIP client sends a CMD_SUBMIT with an invalid endpoint number or an
out-of-bounds number of isoc packets, get_pipe() returns -1.

Looking at drivers/usb/usbip/stub_rx.c:stub_recv_cmd_submit():

	int pipe = get_pipe(sdev, pdu);
	...
	if (pipe == -1)
		return;

When get_pipe() returns -1, stub_recv_cmd_submit() returns early without
consuming the TCP payload or issuing a connection teardown (e.g., via
usbip_event_add(ud, SDEV_EVENT_ERROR_TCP)).

For OUT transfers with a payload, the unread bytes remain in the socket
buffer. The next loop iteration reads this leftover payload as a USBIP
header, desynchronizing the protocol stream and abruptly terminating the
connection.

For IN transfers, no payload is left, but the server drops the request
without sending a RET_SUBMIT response.

Does this cause the client's URB to hang indefinitely?

If the client later unlinks this hung URB, it seems the server replies
with a RET_UNLINK status of 0 (success) because it never tracked the
dropped URB.

Could this trick the client into completing the failed URB with a false
success status, breaking the protocol state machine?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005125842.197848-1-oneukum@suse.com?part=1

Re: [PATCH] usb: usbip: stub_rx: remove outdated comment

[Shuah Khan]

On 10/5/26 06:58, Oliver Neukum wrote:
> After removal of the BUG() statement the error exit can be reached
> and logs a report. Remove a comment saying otherwise.
> 
> Fixes: 635f545a7e8be ("usbip: fix stub_rx: get_pipe() to validate endpoint number")
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>   drivers/usb/usbip/stub_rx.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
> index 1e9ae578810d..0ccc05b76c5a 100644
> --- a/drivers/usb/usbip/stub_rx.c
> +++ b/drivers/usb/usbip/stub_rx.c
> @@ -391,7 +391,6 @@ static int get_pipe(struct stub_device *sdev, struct usbip_header *pdu)
>   	}
>   
>   err_ret:
> -	/* NOT REACHED */
>   	dev_err(&sdev->udev->dev, "CMD_SUBMIT: invalid epnum %d\n", epnum);
>   	return -1;
>   }

Looks good to me.

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah