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

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

[Shuai Liu]

usb_stor_scan_dwork() acquires us->dev_mutex and then calls
usb_stor_Bulk_max_lun(), which issues a synchronous GET_MAX_LUN control
transfer (10s timeout).  The reset and suspend callbacks
(usb_stor_pre_reset / usb_stor_suspend) also acquire us->dev_mutex, but
they run while the USB core already holds the device lock
(udev->dev.mutex) during usb_reset_device() or disconnect.  This
creates an AB-BA deadlock:

  scan work           reset/disconnect
  ---------           ----------------
  mutex_lock(dev_mutex)
                      device_lock(udev)      [USB core]
  control transfer  -- blocks waiting for
                        the device lock
                      mutex_lock(dev_mutex)  [pre_reset]
                        -- blocks forever

syzbot reports this as a task hung in usb_stor_scan_dwork(), with
cancel_delayed_work_sync() in quiesce_and_remove_host() unable to make
progress.

Fix it by making usb_stor_Bulk_max_lun() self-contained: use
usb_control_msg_recv(), which bounces the one-byte reply through a
DMA-safe buffer, instead of the shared per-device resources (us->iobuf,
us->cr, us->current_urb) that also belong to the control thread and the
reset path.  With no shared state touched, the transfer no longer needs
us->dev_mutex.  If the transfer races with a reset it simply fails and
we fall back to a single LUN, which is the existing behavior for
devices that reject GET_MAX_LUN.

In usb_stor_scan_dwork() call usb_stor_Bulk_max_lun() outside the lock
and only take us->dev_mutex briefly to publish us->max_lun (and the
SCSI host max_lun).  us->recv_ctrl_pipe and us->ifnum are set during
probe and never change, so they are safe to read lockless.

Reported-by: syzbot+30552b4cbe99d6d91306@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=30552b4cbe99d6d91306
Signed-off-by: Shuai Liu <847807867@qq.com>
---
v2:
- switch to usb_control_msg_recv() so the reply goes through a
  DMA-safe bounce buffer instead of a stack buffer (Sashiko)
- initialize lun to 0 so the debug print never reads it unset (Sashiko)
- pass the timeout in milliseconds (10000), not jiffies (Sashiko)
- trim comments; the full deadlock analysis is in this changelog

The remaining Sashiko finding (unlocked GET_MAX_LUN may race with a
reset) is benign: the transfer simply fails and we fall back to one
LUN, which is the existing fallback for devices rejecting the request.
The reported scan_dwork vs disconnect stall is pre-existing and not
introduced by this change.

 drivers/usb/storage/transport.c | 27 +++++++++++++++------------
 drivers/usb/storage/usb.c       | 15 +++++++++++----
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index 8de3015bce05..f32393634415 100644
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -1075,27 +1075,30 @@ EXPORT_SYMBOL_GPL(usb_stor_CB_transport);
 /* Determine what the maximum LUN supported is */
 int usb_stor_Bulk_max_lun(struct us_data *us)
 {
+	u8 lun = 0;
 	int result;
 
-	/* issue the command */
-	us->iobuf[0] = 0;
-	result = usb_stor_control_msg(us, us->recv_ctrl_pipe,
-				 US_BULK_GET_MAX_LUN, 
-				 USB_DIR_IN | USB_TYPE_CLASS | 
-				 USB_RECIP_INTERFACE,
-				 0, us->ifnum, us->iobuf, 1, 10*HZ);
+	/*
+	 * Use a private buffer via usb_control_msg_recv() (which bounces
+	 * through DMA-safe memory) so this transfer touches no shared state
+	 * and need not run under us->dev_mutex.
+	 */
+	result = usb_control_msg_recv(us->pusb_dev, 0, US_BULK_GET_MAX_LUN,
+				      USB_DIR_IN | USB_TYPE_CLASS |
+				      USB_RECIP_INTERFACE,
+				      0, us->ifnum, &lun, 1, 10000, GFP_NOIO);
 
 	usb_stor_dbg(us, "GetMaxLUN command result is %d, data is %d\n",
-		     result, us->iobuf[0]);
+		     result, lun);
 
 	/* If we have a successful request, return the result if valid. */
-	if (result > 0) {
-		if (us->iobuf[0] <= US_BULK_MAX_LUN_LIMIT) {
-			return us->iobuf[0];
+	if (result == 0) {
+		if (lun <= US_BULK_MAX_LUN_LIMIT) {
+			return lun;
 		} else {
 			dev_info(&us->pusb_intf->dev,
 				 "Max LUN %d is not valid, using 0 instead",
-				 us->iobuf[0]);
+				 lun);
 		}
 	}
 
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index 823a4b2e9033..d973bfd0f23d 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);
 	}
 	scsi_scan_host(us_to_host(us));
-- 
2.53.0

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

[sashiko-bot]

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

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

[Alan Stern]

On Mon, Nov 09, 2026 at 07:53:07AM +0800, 'Shuai Liu' via USB Mass Storage on Linux wrote:
> usb_stor_scan_dwork() acquires us->dev_mutex and then calls
> usb_stor_Bulk_max_lun(), which issues a synchronous GET_MAX_LUN control
> transfer (10s timeout).  The reset and suspend callbacks
> (usb_stor_pre_reset / usb_stor_suspend) also acquire us->dev_mutex, but
> they run while the USB core already holds the device lock
> (udev->dev.mutex) during usb_reset_device() or disconnect.  This
> creates an AB-BA deadlock:
> 
>   scan work           reset/disconnect
>   ---------           ----------------
>   mutex_lock(dev_mutex)
>                       device_lock(udev)      [USB core]
>   control transfer  -- blocks waiting for
>                         the device lock

You lost me.  Exactly where does usb_stor_scan_dwork() or 
usb_stor_Bulk_max_lun() try to lock udev?  Taking that lock while 
holding us->dev_mutex certainly would be a violation of the locking 
order used by the rest of the driver.

>                       mutex_lock(dev_mutex)  [pre_reset]
>                         -- blocks forever
> 
> syzbot reports this as a task hung in usb_stor_scan_dwork(), with
> cancel_delayed_work_sync() in quiesce_and_remove_host() unable to make
> progress.

Are you certain this diagnosis is correct?  I can imagine the task being 
hung inside the mutex_lock(&us->dev_mutex) call, but that's not what you 
are claiming.

Alan Stern

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

[Shuai Liu]

On Sun, Nov 08, 2026 at 09:10:50PM -0500, Alan Stern wrote:
> You lost me.  Exactly where does usb_stor_scan_dwork() or
> usb_stor_Bulk_max_lun() try to lock udev?  Taking that lock while
> holding us->dev_mutex certainly would be a violation of the locking
> order used by the rest of the driver.
>
> You are certain this diagnosis is correct?  I can imagine the task
> being hung inside the mutex_lock(&us->dev_mutex) call, but that's not
> what you are claiming.

You are right, and I am sorry for the confusion.  My analysis was wrong
and this patch cannot fix the reported hang.

I went back to the full report and read the "locks held" section, which
makes the picture unambiguous:

 - the hung kworker is blocked in mutex_lock(&us->dev_mutex) at the very
   entry of usb_stor_scan_dwork(), so it never reaches
   usb_stor_Bulk_max_lun();
 - that mutex is owned by a usb_hub_wq worker sitting in
   ene_ub6250_probe() (ene_ub6250.c:2363), which holds it while
   ene_get_card_type() -> ene_send_scsi_cmd() ->
   usb_stor_bulk_transfer_buf() waits for a bulk transfer;
 - the wait goes through usb_stor_msg_common(us, 0), i.e. with
   MAX_SCHEDULE_TIMEOUT and no timeout at all.  That is fine for a
   transfer belonging to a SCSI command, whose timeout and abort
   handling end the wait, but a card-type query has no SCSI command
   behind it, so a device which enumerates but never services bulk
   transfers blocks the probe forever;
 - the same probe path also holds udev->dev.mutex, which is why
   usbdev_open() is blocked in the report.

So the deadlock I described does not exist; the scan work is a victim,
not a participant.  Taking the transfer out of the dev_mutex critical
section leaves the mutex_lock() in place, so the hang would remain.

Please drop this patch.

The right fix is already on the list: Hongyu Xie's

  [PATCH v4] usb-storage: ene_ub6250: don't let the card-type probe hang forever
  https://lore.kernel.org/all/20261109085449.83117-1-xiehongyu1@kylinos.cn/

which bounds the whole card-type query with a time budget.  That is the
correct place to address this.

Sorry for the noise.

Shuai Liu