Re: [PATCH v2] usb: storage: don't hold dev_mutex across the max_lun control transfer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] usb: storage: unlocked usb_stor_Bulk_max_lun() races with reset
  and I/O
--

commit f8913af4317f2585b6b4e25b82f0040382c7198c
Author: Shuai Liu <847807867@qq.com>

usb: storage: don't hold dev_mutex across the max_lun control transfer

This commit changes usb_stor_Bulk_max_lun() to use usb_control_msg_recv()
and calls it outside us->dev_mutex in usb_stor_scan_dwork().

> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> index 823a4b2e9033b..d973bfd0f23d1 100644
> --- a/drivers/usb/storage/usb.c
> +++ b/drivers/usb/storage/usb.c
> @@ -980,19 +980,26 @@ static void usb_stor_scan_dwork(struct work_struct *work)
>  
>  	dev_dbg(dev, "starting scan\n");
>  
> -	/* For bulk-only devices, determine the max LUN value */
> +	/*
> +	 * For bulk-only devices, determine the max LUN value.
> +	 * usb_stor_Bulk_max_lun() sleeps in a synchronous transfer, so it
> +	 * must not run under us->dev_mutex (the reset path takes dev_mutex
> +	 * while holding the device lock).  Lock only to publish the result.
> +	 */
>  	if (us->protocol == USB_PR_BULK &&
>  	    !(us->fflags & US_FL_SINGLE_LUN) &&
>  	    !(us->fflags & US_FL_SCM_MULT_TARG)) {
> +		int max_lun = usb_stor_Bulk_max_lun(us);
> +
>  		mutex_lock(&us->dev_mutex);
> -		us->max_lun = usb_stor_Bulk_max_lun(us);
> +		us->max_lun = max_lun;
>  		/*
>  		 * Allow proper scanning of devices that present more than 8 LUNs
>  		 * While not affecting other devices that may need the previous
>  		 * behavior
>  		 */
> -		if (us->max_lun >= 8)
> -			us_to_host(us)->max_lun = us->max_lun+1;
> +		if (max_lun >= 8)
> +			us_to_host(us)->max_lun = max_lun + 1;
>  		mutex_unlock(&us->dev_mutex);
>  	}

[Severity: High]
Does calling usb_stor_Bulk_max_lun() without us->dev_mutex break the
quiescence contract of usb_stor_pre_reset() and usb_stor_suspend(), and
race with concurrent resets or SCSI commands?

usb_stor_pre_reset() and usb_stor_suspend() take us->dev_mutex to ensure
all driver URBs have completed and no new URBs are submitted during reset
or suspend:

drivers/usb/storage/usb.c:usb_stor_pre_reset() {
	struct us_data *us = usb_get_intfdata(iface);

	/* Make sure no command runs during the reset */
	mutex_lock(&us->dev_mutex);
	return 0;
}

With usb_stor_Bulk_max_lun() unlocked:

CPU0 (scan_dwork)                 CPU1 (usb_reset_device)
usb_stor_scan_dwork()
                                  usb_stor_pre_reset()
                                    mutex_lock(&us->dev_mutex)
                                  usb_reset_and_verify_device()
                                    hub_port_reset()
  usb_stor_Bulk_max_lun()
    usb_control_msg_recv()  <-- EP0 transfer runs during reset and fails
                                  usb_stor_post_reset()
                                    mutex_unlock(&us->dev_mutex)
  mutex_lock(&us->dev_mutex)
  us->max_lun = 0;          <-- LUNs > 0 permanently lost

Instead of waiting on us->dev_mutex for usb_stor_post_reset() to finish,
usb_control_msg_recv() runs during port reset and EP0 re-enumeration,
fails, and causes usb_stor_Bulk_max_lun() to return 0, hiding all LUNs
above 0 on multi-LUN devices.  Because scsi_add_host() is called in
usb_stor_probe2() before scan_dwork runs, an unlocked GET_MAX_LUN can also
run concurrently on the device with Bulk-Only transfers and resets in
usb_stor_control_thread() and device_reset().

Also, is the ABBA deadlock described in the comment and commit message
possible?  Neither usb_stor_control_msg() nor usb_submit_urb() acquires
udev->dev.mutex, and quiesce_and_remove_host() never takes us->dev_mutex
either; it waits in cancel_delayed_work_sync(&us->scan_dwork) for the work
item to finish, while switching from usb_stor_control_msg() to
usb_control_msg_recv() makes the 10-second wait uninterruptible and no
longer abortable via US_FLIDX_ABORTING.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_5A9A9E45415EADE916B7558C66A2C4DD4F05@qq.com?part=1