* Re: [PATCH] usb: storage: bound timeout=0 URB waits to 30s
2026-10-14 9:38 [PATCH] usb: storage: bound timeout=0 URB waits to 30s Hongyu Xie
@ 2026-10-14 9:55 ` sashiko-bot
2026-10-14 11:47 ` Greg KH
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-10-14 9:55 UTC (permalink / raw)
To: Hongyu Xie; +Cc: linux-usb
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb-storage: hardcoded 30s URB timeout breaks long-running SCSI commands
--
commit e1a1498b59035fae65ddf1b4b34a943e9c28791c
Author: Hongyu Xie <xiehongyu1@kylinos.cn>
usb: storage: bound timeout=0 URB waits to 30s
This commit bounds timeout=0 URB waits in usb_stor_msg_common() to 30 seconds
to prevent permanent hangs when a device stops responding mid-transfer. It
replaces MAX_SCHEDULE_TIMEOUT with a 30-second timeout, matching the SCSI
layer's default command timeout for disk and CD-ROM LUs.
> diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
> index 8de3015bce050..a6588e55312c2 100644
> --- a/drivers/usb/storage/transport.c
> +++ b/drivers/usb/storage/transport.c
[ ... ]
> @@ -164,7 +173,7 @@ static int usb_stor_msg_common(struct us_data *us, int timeout)
>
> /* wait for the completion of the URB */
> timeleft = wait_for_completion_interruptible_timeout(
> - &urb_done, timeout ? : MAX_SCHEDULE_TIMEOUT);
> + &urb_done, timeout ? : USB_STOR_URB_TIMEOUT);
[Severity: High]
Does this hardcoded 30-second timeout break legitimate long-running SCSI
commands like FORMAT UNIT or tape rewinds?
This logic unconditionally truncates unbounded transport waits to 30 seconds,
ignoring dynamic timeouts present in us->srb->request->timeout. A BOT SCSI
command taking longer than 30 seconds could legitimately withhold the CSW
until the operation finishes.
Could this cause usb_stor_msg_common() to kill the URB and return
USB_STOR_XFER_ERROR before the SCSI layer's own command timeout (which could
be hours) expires?
>
> clear_bit(US_FLIDX_URB_ACTIVE, &us->dflags);
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261014093856.415812-1-xiehongyu1@kylinos.cn?part=1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] usb: storage: bound timeout=0 URB waits to 30s
2026-10-14 9:38 [PATCH] usb: storage: bound timeout=0 URB waits to 30s Hongyu Xie
2026-10-14 9:55 ` sashiko-bot
@ 2026-10-14 11:47 ` Greg KH
2026-10-14 14:44 ` [usb-storage] " Alan Stern
2026-10-15 13:10 ` Oliver Neukum
3 siblings, 0 replies; 5+ messages in thread
From: Greg KH @ 2026-10-14 11:47 UTC (permalink / raw)
To: Hongyu Xie; +Cc: stern, linux-usb, usb-storage, linux-kernel, stable
On Wed, Oct 14, 2026 at 05:38:56PM +0800, Hongyu Xie wrote:
> usb_stor_msg_common() is the common URB submission/wait helper for
> usb-storage transfers. Callers that pass a positive timeout get that
> timeout, but the three internal transfer helpers (usb_stor_ctrl_transfer,
> usb_stor_intr_transfer and usb_stor_bulk_transfer_buf) all pass 0,
> which selects MAX_SCHEDULE_TIMEOUT: the control thread waits for the URB
> forever.
>
> A device that stops responding while such a transfer is pending hangs the
> control thread permanently, and the hang propagates into the SCSI error
> handler:
>
> scsi command timeout -> eh_abort_handler -> command_abort()
> -> usb_stor_stop_transport() /* URB unlink issued, but the URB of
> a device wedged mid-transition
> never completes */
> -> wait_for_completion(&us->notify) /* no timeout */
>
> The unbounded wait also closes a deadlock cycle observed on arm64 servers
> running 5.4.x, where a Realtek USB WiFi dongle in CD-ROM mode (sr0)
> wedges during usb_modeswitch's re-enumeration transition: the system
> shutdown hook (mdadm --wait-clean --scan, which opens every block device)
> blocks in disk_block_events() holding ev->block_mutex, waiting for the
> disk events work whose TEST UNIT READY timed out into the
> command_abort() wait above. No timeout anywhere in the cycle means the
> shutdown hangs forever -- hung_task warnings every 120s until power loss,
> two production incidents with identical stacks.
>
> Bound the fallback wait to 30s, matching the SCSI layer's default command
> timeout for disk and CD-ROM LUs (SD_TIMEOUT / SR_TIMEOUT). When the bound
> fires, the URB is killed, the transport reports USB_STOR_XFER_ERROR, and
> the command fails through the regular paths: the control thread
> completes, notify is signalled, the abort returns, and the events work
> (and with it the shutdown) can finish.
>
> For disk and CD-ROM LUs this bound introduces no new policy: a transfer
> that produced no URB completion for 30s has already exhausted the SCSI
> layer's patience, and the error handler trying to abort it cannot make
> progress while the control thread is stuck in the unbounded wait.
>
> A blanket bound for this helper was suggested before [1] but dropped over
> concern that legitimate SCSI commands (format, tape rewind) can exceed
> 60 seconds. Those commands are issued to tape LUs whose SCSI timeouts
> are much longer than 30s; usb-storage-attached tapes holding back a CSW
> for more than 30 seconds remain the residual false-positive window of
> this change (modern tape bridges use UAS, which is unaffected).
>
> The scatter-gather data path (usb_stor_bulk_transfer_sglist(), which
> waits in usb_sg_wait()) is a separate unbounded wait and is not covered
> by this change; a device that stalls mid data phase can still wedge the
> control thread there.
>
> Validation: on the affected production machine the hang occurred twice
> across ~4800 hibernate cycles with identical stacks; with this bound in
> place, 990+ hibernate cycles completed without a recurrence.
>
> [1] https://lkml.rescloud.iu.edu/hypermail/linux/kernel/2207.0/05816.html
Please only use lore.kernel.org links.
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
> ---
No diffstat? How did you generate this?
> --- a/drivers/usb/storage/transport.c
> +++ b/drivers/usb/storage/transport.c
> @@ -112,6 +112,15 @@
> * command _must_ pass through this function (or something like it) for the
> * abort mechanisms to work properly.
> */
> +
> +/*
> + * Bound for the URB wait when the caller passes timeout == 0: matches the
> + * SCSI layer's default command timeout for disk and CD-ROM LUs (SD_TIMEOUT /
> + * SR_TIMEOUT), so a wedged device surfaces as a regular command failure
> + * instead of hanging the control thread forever.
Please rewrite this from scratch and do not use a LLM for it, as this
doesn't make much sense to me at all.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [usb-storage] [PATCH] usb: storage: bound timeout=0 URB waits to 30s
2026-10-14 9:38 [PATCH] usb: storage: bound timeout=0 URB waits to 30s Hongyu Xie
2026-10-14 9:55 ` sashiko-bot
2026-10-14 11:47 ` Greg KH
@ 2026-10-14 14:44 ` Alan Stern
2026-10-15 13:10 ` Oliver Neukum
3 siblings, 0 replies; 5+ messages in thread
From: Alan Stern @ 2026-10-14 14:44 UTC (permalink / raw)
To: Hongyu Xie; +Cc: gregkh, linux-usb, usb-storage, linux-kernel, stable
On Wed, Oct 14, 2026 at 05:38:56PM +0800, Hongyu Xie wrote:
> usb_stor_msg_common() is the common URB submission/wait helper for
> usb-storage transfers. Callers that pass a positive timeout get that
> timeout, but the three internal transfer helpers (usb_stor_ctrl_transfer,
> usb_stor_intr_transfer and usb_stor_bulk_transfer_buf) all pass 0,
> which selects MAX_SCHEDULE_TIMEOUT: the control thread waits for the URB
> forever.
>
> A device that stops responding while such a transfer is pending hangs the
> control thread permanently,
Why do you say it hangs _permanently_? I've seen plenty of examples
where the hang ended when the transfer was aborted.
> and the hang propagates into the SCSI error
> handler:
>
> scsi command timeout -> eh_abort_handler -> command_abort()
> -> usb_stor_stop_transport() /* URB unlink issued, but the URB of
> a device wedged mid-transition
> never completes */
Why do you say the URB never completes? Unlinking an URB should always
cause it to complete, whether the device is responding or not.
> -> wait_for_completion(&us->notify) /* no timeout */
>
> The unbounded wait also closes a deadlock cycle observed on arm64 servers
> running 5.4.x, where a Realtek USB WiFi dongle in CD-ROM mode (sr0)
> wedges during usb_modeswitch's re-enumeration transition: the system
> shutdown hook (mdadm --wait-clean --scan, which opens every block device)
> blocks in disk_block_events() holding ev->block_mutex, waiting for the
> disk events work whose TEST UNIT READY timed out into the
> command_abort() wait above. No timeout anywhere in the cycle means the
> shutdown hangs forever -- hung_task warnings every 120s until power loss,
> two production incidents with identical stacks.
Then the appropriate fix is to add a suitable timeout at the appropriate
point in this cycle. Not to force every command to be limited to a
pre-defined 30-second timeout.
> Bound the fallback wait to 30s, matching the SCSI layer's default command
> timeout for disk and CD-ROM LUs (SD_TIMEOUT / SR_TIMEOUT). When the bound
> fires, the URB is killed, the transport reports USB_STOR_XFER_ERROR, and
> the command fails through the regular paths: the control thread
> completes, notify is signalled, the abort returns, and the events work
> (and with it the shutdown) can finish.
>
> For disk and CD-ROM LUs this bound introduces no new policy: a transfer
> that produced no URB completion for 30s has already exhausted the SCSI
> layer's patience, and the error handler trying to abort it cannot make
> progress while the control thread is stuck in the unbounded wait.
>
> A blanket bound for this helper was suggested before [1] but dropped over
> concern that legitimate SCSI commands (format, tape rewind) can exceed
> 60 seconds. Those commands are issued to tape LUs whose SCSI timeouts
> are much longer than 30s; usb-storage-attached tapes holding back a CSW
> for more than 30 seconds remain the residual false-positive window of
> this change (modern tape bridges use UAS, which is unaffected).
>
> The scatter-gather data path (usb_stor_bulk_transfer_sglist(), which
> waits in usb_sg_wait()) is a separate unbounded wait and is not covered
> by this change; a device that stalls mid data phase can still wedge the
> control thread there.
Again, why doesn't the scsi error handler procedure unwedge the thread?
Alan Stern
> Validation: on the affected production machine the hang occurred twice
> across ~4800 hibernate cycles with identical stacks; with this bound in
> place, 990+ hibernate cycles completed without a recurrence.
>
> [1] https://lkml.rescloud.iu.edu/hypermail/linux/kernel/2207.0/05816.html
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hongyu Xie <xiehongyu1@kylinos.cn>
> ---
> --- a/drivers/usb/storage/transport.c
> +++ b/drivers/usb/storage/transport.c
> @@ -112,6 +112,15 @@
> * command _must_ pass through this function (or something like it) for the
> * abort mechanisms to work properly.
> */
> +
> +/*
> + * Bound for the URB wait when the caller passes timeout == 0: matches the
> + * SCSI layer's default command timeout for disk and CD-ROM LUs (SD_TIMEOUT /
> + * SR_TIMEOUT), so a wedged device surfaces as a regular command failure
> + * instead of hanging the control thread forever.
> + */
> +#define USB_STOR_URB_TIMEOUT (30 * HZ)
> +
> static int usb_stor_msg_common(struct us_data *us, int timeout)
> {
> struct completion urb_done;
> @@ -164,7 +173,7 @@
>
> /* wait for the completion of the URB */
> timeleft = wait_for_completion_interruptible_timeout(
> - &urb_done, timeout ? : MAX_SCHEDULE_TIMEOUT);
> + &urb_done, timeout ? : USB_STOR_URB_TIMEOUT);
>
> clear_bit(US_FLIDX_URB_ACTIVE, &us->dflags);
>
> --
> 2.25.1
>
> --
> You received this message because you are subscribed to the Google Groups "USB Mass Storage on Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to usb-storage+unsubscribe@lists.one-eyed-alien.net.
> To view this discussion visit https://groups.google.com/a/lists.one-eyed-alien.net/d/msgid/usb-storage/20261014093856.415812-1-xiehongyu1%40kylinos.cn.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] usb: storage: bound timeout=0 URB waits to 30s
2026-10-14 9:38 [PATCH] usb: storage: bound timeout=0 URB waits to 30s Hongyu Xie
` (2 preceding siblings ...)
2026-10-14 14:44 ` [usb-storage] " Alan Stern
@ 2026-10-15 13:10 ` Oliver Neukum
3 siblings, 0 replies; 5+ messages in thread
From: Oliver Neukum @ 2026-10-15 13:10 UTC (permalink / raw)
To: Hongyu Xie, stern, gregkh; +Cc: linux-usb, usb-storage, linux-kernel, stable
On 14.10.26 11:38, Hongyu Xie wrote:
> A blanket bound for this helper was suggested before [1] but dropped over
> concern that legitimate SCSI commands (format, tape rewind) can exceed
> 60 seconds. Those commands are issued to tape LUs whose SCSI timeouts
> are much longer than 30s; usb-storage-attached tapes holding back a CSW
> for more than 30 seconds remain the residual false-positive window of
> this change (modern tape bridges use UAS, which is unaffected).
And these concerns are entirely justified.
I am sorry, but there is no uniform timeout suitable for SCSI commands.
If anything hangs based on wrong timeouts, that is the bug to fix.
Regards
Oliver
^ permalink raw reply [flat|nested] 5+ messages in thread