Re: [PATCH v2] usb: uas: wait for pending commands before unbinding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: uas: stalled wait and false timeout error in uas_pre_disconnect()
--

commit 4c54d6873e1e380dddee1542eeac75bb8256858a
Author: Jiayi Li <lijiayi@kylinos.cn>

usb: uas: wait for pending commands before unbinding

This commit enables soft_unbind on the uas driver and adds
uas_pre_disconnect() to block SCSI requests and wait for pending URBs and
commands to finish before unbinding the interface.

> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
> index 8655edbd66b16..300ab637be70b 100644
> --- a/drivers/usb/storage/uas.c
> +++ b/drivers/usb/storage/uas.c

[ ... ]

> @@ -1211,16 +1212,54 @@ static int uas_reset_resume(struct usb_interface *intf)
>  	return 0;
>  }
>  
> +static void uas_pre_disconnect(struct usb_interface *intf)
> +{
> +	struct Scsi_Host *shost = usb_get_intfdata(intf);
> +	struct uas_dev_info *devinfo = (struct uas_dev_info *)shost->hostdata;
> +	unsigned long start_time;
> +
> +	start_time = jiffies;
> +	for (;;) {
> +		if (READ_ONCE(devinfo->udev->state) == USB_STATE_NOTATTACHED)
> +			return;
> +
> +		flush_work(&devinfo->work);
> +
> +		if (!usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000))
> +			break;
> +
> +		if (!usb_wait_anchor_empty_timeout(&devinfo->data_urbs, 500))
> +			break;
> +
> +		if (time_after(jiffies, start_time + 5 * HZ))
> +			break;
> +		if (uas_cmnd_list_empty(devinfo))
> +			return;
> +
> +		msleep(20);
> +	}
> +
> +	shost_printk(KERN_ERR, shost, "%s: timed out\n", __func__);
> +}

[Severity: Medium]
Can uas_pre_disconnect() stall for 5 seconds or emit a false timeout error in
the following cases?

First, in uas_pre_disconnect(), time_after(jiffies, start_time + 5 * HZ) is
checked before uas_cmnd_list_empty(devinfo). If the waits on sense_urbs and
data_urbs both succeed (for example, 4.7s waiting on sense_urbs followed by
0.4s waiting on data_urbs, or a scheduling delay), can total elapsed time
exceed 5 * HZ and cause the loop to break out and log a timeout error even
when all commands have completed and uas_cmnd_list_empty(devinfo) is true?

Second, because uas_driver sets soft_unbind = 1, usb_unbind_interface() holds
the interface lock (and callers such as usb_set_configuration() or
usb_reset_device() also hold the device lock) when calling
uas_disconnect() -> uas_pre_disconnect() without disabling interface
endpoints. If the device is physically unplugged while uas_pre_disconnect()
is sleeping in usb_wait_anchor_empty_timeout(&devinfo->sense_urbs, 5000),
usb_disconnect() sets udev->state = USB_STATE_NOTATTACHED and calls
usb_disable_device(), which blocks on the device or interface lock before
reaching usb_disable_device_endpoints():

drivers/usb/core/message.c:usb_disable_device() {
	...
			remove_intf_ep_devs(interface);
			device_del(&interface->dev);
		}
	...
	usb_disable_device_endpoints(dev, skip_ep0);
}

Since pending sense_urbs are not unlinked by USB core until uas_disconnect()
returns, won't usb_wait_anchor_empty_timeout() wait the full 5000 ms, break
out of the loop, and log a timeout error without rechecking whether
devinfo->udev->state became USB_STATE_NOTATTACHED?

Third, if a status URB completes with a non-zero status, uas_stat_cmplt()
frees the URB without clearing COMMAND_INFLIGHT or devinfo->cmnd[idx]:

drivers/usb/storage/uas.c:uas_stat_cmplt() {
	...
	if (status) {
		if (status != -ENOENT && status != -ECONNRESET && status != -ESHUTDOWN)
			dev_err(&urb->dev->dev, "stat urb: status %d\n", status);
		goto bail;
	}
	...
bail:
	usb_free_urb(urb);
}

Once sense_urbs and data_urbs are empty and devinfo->work is flushed, no
further status URB will complete for that command until uas_zap_pending() runs
after uas_pre_disconnect(). Wouldn't uas_cmnd_list_empty(devinfo) remain false
and cause uas_pre_disconnect() to spin in msleep(20) for the full 5 seconds
before logging a timeout error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023013931.388916-1-lijiayi@kylinos.cn?part=1