[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@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=30552b4cbe99d6d91306
Signed-off-by: Shuai Liu <847807867@xxxxxx>
---
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