linux-kernel - Re: [PATCH v2] USB:UAS:return ENODEV when submit urbs fail with device not attached.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 1 Mar 2024 00:40:43 +0800
From: "WeitaoWang-oc@...oxin.com" <WeitaoWang-oc@...oxin.com>
To: Oliver Neukum <oneukum@...e.com>, <stern@...land.harvard.edu>,
	<gregkh@...uxfoundation.org>, <linux-usb@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-scsi@...r.kernel.org>,
	<usb-storage@...ts.one-eyed-alien.net>
CC: <WeitaoWang@...oxin.com>, <stable@...r.kernel.org>
Subject: Re: [PATCH v2] USB:UAS:return ENODEV when submit urbs fail with
 device not attached.

On 2024/2/29 16:08, Oliver Neukum wrote:
> 

> On 29.02.24 12:19, WeitaoWang-oc@...oxin.com wrote:
> 
>> When alloc urb fail in the same function uas_submit_urbs,
>> whether we should replace SCSI_MLQUEUE_DEVICE_BUSY with generic
>> error code -ENOMEM? Such like this:
>>
>> @@ -572,7 +571,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
>>           cmdinfo->data_in_urb = uas_alloc_data_urb(devinfo, GFP_ATOMIC,
>>                               cmnd, DMA_FROM_DEVICE);
>>           if (!cmdinfo->data_in_urb)
>> -            return SCSI_MLQUEUE_DEVICE_BUSY;
>> +            return -ENOMEM;
>>           cmdinfo->state &= ~ALLOC_DATA_IN_URB;
>>       }
> 
> Hi,
> 
> yes, and then you translate in one central place for the SCSI layer
> into DID_ERROR or DID_NO_CONNECT.
> 

OK, I'll submit a new version after you help to review the following patch.


diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 9707f53cfda9..689396777b6f 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -533,7 +533,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, 
gfp_t gfp,
   * daft to me.
   */

-static struct urb *uas_submit_sense_urb(struct scsi_cmnd *cmnd, gfp_t gfp)
+static int uas_submit_sense_urb(struct scsi_cmnd *cmnd, gfp_t gfp)
  {
  	struct uas_dev_info *devinfo = cmnd->device->hostdata;
  	struct urb *urb;
@@ -541,16 +541,15 @@ static struct urb *uas_submit_sense_urb(struct scsi_cmnd *cmnd, 
gfp_t gfp)

  	urb = uas_alloc_sense_urb(devinfo, gfp, cmnd);
  	if (!urb)
-		return NULL;
+		return -ENOMEM;
  	usb_anchor_urb(urb, &devinfo->sense_urbs);
  	err = usb_submit_urb(urb, gfp);
  	if (err) {
  		usb_unanchor_urb(urb);
  		uas_log_cmd_state(cmnd, "sense submit err", err);
  		usb_free_urb(urb);
-		return NULL;
  	}
-	return urb;
+	return err;
  }

  static int uas_submit_urbs(struct scsi_cmnd *cmnd,
@@ -562,9 +561,9 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,

  	lockdep_assert_held(&devinfo->lock);
  	if (cmdinfo->state & SUBMIT_STATUS_URB) {
-		urb = uas_submit_sense_urb(cmnd, GFP_ATOMIC);
-		if (!urb)
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+		err = uas_submit_sense_urb(cmnd, GFP_ATOMIC);
+		if (err)
+			return err;
  		cmdinfo->state &= ~SUBMIT_STATUS_URB;
  	}

@@ -572,7 +571,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
  		cmdinfo->data_in_urb = uas_alloc_data_urb(devinfo, GFP_ATOMIC,
  							cmnd, DMA_FROM_DEVICE);
  		if (!cmdinfo->data_in_urb)
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+			return -ENOMEM;
  		cmdinfo->state &= ~ALLOC_DATA_IN_URB;
  	}

@@ -582,7 +581,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
  		if (err) {
  			usb_unanchor_urb(cmdinfo->data_in_urb);
  			uas_log_cmd_state(cmnd, "data in submit err", err);
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+			return err;
  		}
  		cmdinfo->state &= ~SUBMIT_DATA_IN_URB;
  		cmdinfo->state |= DATA_IN_URB_INFLIGHT;
@@ -592,7 +591,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
  		cmdinfo->data_out_urb = uas_alloc_data_urb(devinfo, GFP_ATOMIC,
  							cmnd, DMA_TO_DEVICE);
  		if (!cmdinfo->data_out_urb)
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+			return -ENOMEM;
  		cmdinfo->state &= ~ALLOC_DATA_OUT_URB;
  	}

@@ -602,7 +601,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
  		if (err) {
  			usb_unanchor_urb(cmdinfo->data_out_urb);
  			uas_log_cmd_state(cmnd, "data out submit err", err);
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+			return err;
  		}
  		cmdinfo->state &= ~SUBMIT_DATA_OUT_URB;
  		cmdinfo->state |= DATA_OUT_URB_INFLIGHT;
@@ -611,7 +610,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
  	if (cmdinfo->state & ALLOC_CMD_URB) {
  		cmdinfo->cmd_urb = uas_alloc_cmd_urb(devinfo, GFP_ATOMIC, cmnd);
  		if (!cmdinfo->cmd_urb)
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+			return -ENOMEM;
  		cmdinfo->state &= ~ALLOC_CMD_URB;
  	}

@@ -621,7 +620,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd,
  		if (err) {
  			usb_unanchor_urb(cmdinfo->cmd_urb);
  			uas_log_cmd_state(cmnd, "cmd submit err", err);
-			return SCSI_MLQUEUE_DEVICE_BUSY;
+			return err;
  		}
  		cmdinfo->cmd_urb = NULL;
  		cmdinfo->state &= ~SUBMIT_CMD_URB;
@@ -698,7 +697,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd)
  	 * of queueing, no matter how fatal the error
  	 */
  	if (err == -ENODEV) {
-		set_host_byte(cmnd, DID_ERROR);
+		set_host_byte(cmnd, DID_NO_CONNECT);
  		scsi_done(cmnd);
  		goto zombie;
  	}

Thanks and best regards,
weitao