[v2] usb: uas: fix urb unmapping issue when the uas device is remove during ongoing data transfer

[PATCH v2] usb: uas: fix urb unmapping issue when the uas device is remove during ongoing data transfer

Posted by guhuinan 2 months ago

From: Owen Gu <guhuinan@xiaomi.com>

When a UAS device is unplugged during data transfer, there is
a probability of a system panic occurring. The root cause is
an access to an invalid memory address during URB callback handling.
Specifically, this happens when the dma_direct_unmap_sg() function
is called within the usb_hcd_unmap_urb_for_dma() interface, but the
sg->dma_address field is 0 and the sg data structure has already been
freed.

The SCSI driver sends transfer commands by invoking uas_queuecommand_lck()
in uas.c, using the uas_submit_urbs() function to submit requests to USB.
Within the uas_submit_urbs() implementation, three URBs (sense_urb,
data_urb, and cmd_urb) are sequentially submitted. Device removal may
occur at any point during uas_submit_urbs execution, which may result
in URB submission failure. However, some URBs might have been successfully
submitted before the failure, and uas_submit_urbs will return the -ENODEV
error code in this case. The current error handling directly calls
scsi_done(). In the SCSI driver, this eventually triggers scsi_complete()
to invoke scsi_end_request() for releasing the sgtable. The successfully
submitted URBs, when being unlinked to giveback, call
usb_hcd_unmap_urb_for_dma() in hcd.c, leading to exceptions during sg
unmapping operations since the sg data structure has already been freed.

This patch modifies the error condition check in the uas_submit_urbs()
function. When a UAS device is removed but one or more URBs have already
been successfully submitted to USB, it avoids immediately invoking
scsi_done() and save the cmnd to devinfo->cmnd array. If the successfully
submitted URBs is completed before devinfo->resetting being set, then
the scsi_done() function will be called within uas_try_complete() after
all pending URB operations are finalized. Otherwise, the scsi_done()
function will be called within uas_zap_pending(), which is executed after
usb_kill_anchored_urbs(). The uas_zap_pending() iterates over
devinfo->cmnd array, invoking uas_try_complete() for each command to
finalize their handling.

Signed-off-by: Yu Chen <chenyu45@xiaomi.com>
Signed-off-by: Owen Gu <guhuinan@xiaomi.com>
---
v2: Upon uas_submit_urbs() returning -ENODEV despite successful URB
submission, the cmnd is added to the devinfo->cmnd array before
exiting uas_queuecommand_lck().
v1: https://lore.kernel.org/linux-usb/20250930045309.21588-1-guhuinan@xiaomi.com/
---
---
 drivers/usb/storage/uas.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 4ed0dc19afe0..45b01df364f7 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -698,6 +698,10 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd)
 	 * of queueing, no matter how fatal the error
 	 */
 	if (err == -ENODEV) {
+		if (cmdinfo->state & (COMMAND_INFLIGHT | DATA_IN_URB_INFLIGHT |
+				DATA_OUT_URB_INFLIGHT))
+			goto out;
+
 		set_host_byte(cmnd, DID_NO_CONNECT);
 		scsi_done(cmnd);
 		goto zombie;
@@ -711,6 +715,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd)
 		uas_add_work(cmnd);
 	}
 
+out:
 	devinfo->cmnd[idx] = cmnd;
 zombie:
 	spin_unlock_irqrestore(&devinfo->lock, flags);
-- 
2.43.0

Re: [PATCH v2] usb: uas: fix urb unmapping issue when the uas device is remove during ongoing data transfer

Posted by Owen Gu 1 month, 3 weeks ago

Hi Oliver,

I'm following up on my previous patch v2. Could you please provide feedback on it?
If there's anything I can improve, let me know.

Thanks,
Owen Gu

On Wed, Oct 15, 2025 at 11:31:57PM +0800, guhuinan wrote:
> From: Owen Gu <guhuinan@xiaomi.com>
> 
> When a UAS device is unplugged during data transfer, there is
> a probability of a system panic occurring. The root cause is
> an access to an invalid memory address during URB callback handling.
> Specifically, this happens when the dma_direct_unmap_sg() function
> is called within the usb_hcd_unmap_urb_for_dma() interface, but the
> sg->dma_address field is 0 and the sg data structure has already been
> freed.
> 
> The SCSI driver sends transfer commands by invoking uas_queuecommand_lck()
> in uas.c, using the uas_submit_urbs() function to submit requests to USB.
> Within the uas_submit_urbs() implementation, three URBs (sense_urb,
> data_urb, and cmd_urb) are sequentially submitted. Device removal may
> occur at any point during uas_submit_urbs execution, which may result
> in URB submission failure. However, some URBs might have been successfully
> submitted before the failure, and uas_submit_urbs will return the -ENODEV
> error code in this case. The current error handling directly calls
> scsi_done(). In the SCSI driver, this eventually triggers scsi_complete()
> to invoke scsi_end_request() for releasing the sgtable. The successfully
> submitted URBs, when being unlinked to giveback, call
> usb_hcd_unmap_urb_for_dma() in hcd.c, leading to exceptions during sg
> unmapping operations since the sg data structure has already been freed.
> 
> This patch modifies the error condition check in the uas_submit_urbs()
> function. When a UAS device is removed but one or more URBs have already
> been successfully submitted to USB, it avoids immediately invoking
> scsi_done() and save the cmnd to devinfo->cmnd array. If the successfully
> submitted URBs is completed before devinfo->resetting being set, then
> the scsi_done() function will be called within uas_try_complete() after
> all pending URB operations are finalized. Otherwise, the scsi_done()
> function will be called within uas_zap_pending(), which is executed after
> usb_kill_anchored_urbs(). The uas_zap_pending() iterates over
> devinfo->cmnd array, invoking uas_try_complete() for each command to
> finalize their handling.
> 
> Signed-off-by: Yu Chen <chenyu45@xiaomi.com>
> Signed-off-by: Owen Gu <guhuinan@xiaomi.com>
> ---
> v2: Upon uas_submit_urbs() returning -ENODEV despite successful URB
> submission, the cmnd is added to the devinfo->cmnd array before
> exiting uas_queuecommand_lck().
> v1: https://lore.kernel.org/linux-usb/20250930045309.21588-1-guhuinan@xiaomi.com/
> ---
> ---
>  drivers/usb/storage/uas.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 4ed0dc19afe0..45b01df364f7 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c
> @@ -698,6 +698,10 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd)
>  	 * of queueing, no matter how fatal the error
>  	 */
>  	if (err == -ENODEV) {
> +		if (cmdinfo->state & (COMMAND_INFLIGHT | DATA_IN_URB_INFLIGHT |
> +				DATA_OUT_URB_INFLIGHT))
> +			goto out;
> +
>  		set_host_byte(cmnd, DID_NO_CONNECT);
>  		scsi_done(cmnd);
>  		goto zombie;
> @@ -711,6 +715,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd)
>  		uas_add_work(cmnd);
>  	}
>  
> +out:
>  	devinfo->cmnd[idx] = cmnd;
>  zombie:
>  	spin_unlock_irqrestore(&devinfo->lock, flags);
> -- 
> 2.43.0
>

Re: [PATCH v2] usb: uas: fix urb unmapping issue when the uas device is remove during ongoing data transfer

Posted by Oliver Neukum 1 month, 3 weeks ago

Hi,

I think I was unclear the first time.
Sorry for that.

On 27.10.25 07:05, Owen Gu wrote:
> Hi Oliver,
> 

>> This patch modifies the error condition check in the uas_submit_urbs()
>> function. When a UAS device is removed but one or more URBs have already
>> been successfully submitted to USB, it avoids immediately invoking
>> scsi_done() and save the cmnd to devinfo->cmnd array. If the successfully
>> submitted URBs is completed before devinfo->resetting being set, then
>> the scsi_done() function will be called within uas_try_complete() after

This requires that uas_try_complete() is called.

And I am afraid uas_stat_cmplt() cannot guarantee that in the error case.
I think the following sequence of events is possible:

CPU A						CPU B

uas_queuecommand_lck() calls uas_submit_urbs()
COMMAND_INFLIGHT is set and URB submitted
						URB gets an error
						status = -EBABBLE (just an example)
						uas_stat_cmplt is called
						resetting is not set
						if (status)
							goto out;

						uas_try_complete _not_ called

The scsi command runs for indeterminate amount of time.
It seems to me that if you want to use your approach you also
need to change error handling in uas_stat_cmplt()

	Regards
		Oliver

Re: [PATCH v2] usb: uas: fix urb unmapping issue when the uas device is remove during ongoing data transfer

Posted by Owen Gu 1 month, 2 weeks ago

On Mon, Oct 27, 2025 at 02:35:37PM +0100, Oliver Neukum wrote:
> Hi,
> 
> I think I was unclear the first time.
> Sorry for that.
> 
> On 27.10.25 07:05, Owen Gu wrote:
> > Hi Oliver,
> > 
> 
> > > This patch modifies the error condition check in the uas_submit_urbs()
> > > function. When a UAS device is removed but one or more URBs have already
> > > been successfully submitted to USB, it avoids immediately invoking
> > > scsi_done() and save the cmnd to devinfo->cmnd array. If the successfully
> > > submitted URBs is completed before devinfo->resetting being set, then
> > > the scsi_done() function will be called within uas_try_complete() after
> 
> This requires that uas_try_complete() is called.
> 
> And I am afraid uas_stat_cmplt() cannot guarantee that in the error case.
> I think the following sequence of events is possible:
> 
> CPU A						CPU B
> 
> uas_queuecommand_lck() calls uas_submit_urbs()
> COMMAND_INFLIGHT is set and URB submitted
> 						URB gets an error
> 						status = -EBABBLE (just an example)
> 						uas_stat_cmplt is called
> 						resetting is not set
> 						if (status)
> 							goto out;
> 
> 						uas_try_complete _not_ called
> 
> The scsi command runs for indeterminate amount of time.
> It seems to me that if you want to use your approach you also
> need to change error handling in uas_stat_cmplt()
> 
> 	Regards
> 		Oliver
> 
>
Hi Oliver,
I think the error handling only takes effect when uas_queuecommand_lck() calls
uas_submit_urbs() and returns the error value -ENODEV . In this case, the device is
disconnected, and the flow proceeds to uas_disconnect(), where uas_zap_pending() is
invoked to call uas_try_complete().

Regards
Owen

Re: [usb-storage] Re: [PATCH v2] usb: uas: fix urb unmapping issue when the uas device is remove during ongoing data transfer

Posted by Oliver Neukum 1 month, 2 weeks ago

On 01.11.25 14:55, 'Owen Gu' via USB Mass Storage on Linux wrote:
> On Mon, Oct 27, 2025 at 02:35:37PM +0100, Oliver Neukum wrote:

> I think the error handling only takes effect when uas_queuecommand_lck() calls
> uas_submit_urbs() and returns the error value -ENODEV . In this case, the device is
> disconnected, and the flow proceeds to uas_disconnect(), where uas_zap_pending() is
> invoked to call uas_try_complete().

OK, I see. You are right. Please resubmit and I'll ack it.

	Regards
		Oliver