diff mbox series

[1/3] scsi: check the whole result for reading write protect flag

Message ID 20210319030128.1345061-2-yanaijie@huawei.com (mailing list archive)
State Changes Requested
Headers show
Series scsi: check the whole result in some places | expand

Commit Message

Jason Yan March 19, 2021, 3:01 a.m. UTC
When the scsi device status is offline, mode sense command will return a
result with only DID_NO_CONNECT set. Then in sd_read_write_protect_flag(),
only status byte of the result is checked, we still consider the command
returned good, and read sdkp->write_prot from the buffer. And because of
bug [1], garbage data is copied to the buffer, the disk sometimes
be set readonly. When the scsi device is set running again, users cannot
write data to the disk.

Fix this by check the whole result returned by the driver.

[1] https://patchwork.kernel.org/project/linux-block/patch/20210318122621.330010-1-yanaijie@huawei.com/

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/sd.c   |  6 +++---
 include/scsi/scsi.h | 13 +++++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

Comments

Hannes Reinecke March 19, 2021, 7:55 a.m. UTC | #1
On 3/19/21 4:01 AM, Jason Yan wrote:
> When the scsi device status is offline, mode sense command will return a
> result with only DID_NO_CONNECT set. Then in sd_read_write_protect_flag(),
> only status byte of the result is checked, we still consider the command
> returned good, and read sdkp->write_prot from the buffer. And because of
> bug [1], garbage data is copied to the buffer, the disk sometimes
> be set readonly. When the scsi device is set running again, users cannot
> write data to the disk.
> 
> Fix this by check the whole result returned by the driver.
> 
> [1] https://patchwork.kernel.org/project/linux-block/patch/20210318122621.330010-1-yanaijie@huawei.com/
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>   drivers/scsi/sd.c   |  6 +++---
>   include/scsi/scsi.h | 13 +++++++++++++
>   2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index ed0b1bb99f08..16f8cd2895fd 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2669,18 +2669,18 @@ sd_read_write_protect_flag(struct scsi_disk *sdkp, unsigned char *buffer)
>   		 * 5: Illegal Request, Sense Code 24: Invalid field in
>   		 * CDB.
>   		 */
> -		if (!scsi_status_is_good(res))
> +		if (!scsi_result_is_good(res))
>   			res = sd_do_mode_sense(sdkp, 0, 0, buffer, 4, &data, NULL);
>   
>   		/*
>   		 * Third attempt: ask 255 bytes, as we did earlier.
>   		 */
> -		if (!scsi_status_is_good(res))
> +		if (!scsi_result_is_good(res))
>   			res = sd_do_mode_sense(sdkp, 0, 0x3F, buffer, 255,
>   					       &data, NULL);
>   	}
>   
> -	if (!scsi_status_is_good(res)) {
> +	if (!scsi_result_is_good(res)) {
>   		sd_first_printk(KERN_WARNING, sdkp,
>   			  "Test WP failed, assume Write Enabled\n");
>   	} else {
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index e75cca25338a..db0f346a31b2 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -55,6 +55,19 @@ static inline int scsi_status_is_good(int status)
>   		(status == SAM_STAT_COMMAND_TERMINATED));
>   }
>   
> +/** scsi_result_is_good - check the result return.
> + *
> + * @result: the result passed up from the driver (including host and
> + *          driver components)
> + *
> + * Drivers may only set other bytes but not status byte.
> + * This checks both the status byte and other bytes.
> + */
> +static inline int scsi_result_is_good(int result)
> +{
> +	return scsi_status_is_good(result) && (result & ~0xff) == 0;
> +}
> +
>   
>   /*
>    * standard mode-select header prepended to all mode-select commands
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
Martin K. Petersen March 25, 2021, 2:50 a.m. UTC | #2
Hi Jason!

> @@ -55,6 +55,19 @@ static inline int scsi_status_is_good(int status)
>  		(status == SAM_STAT_COMMAND_TERMINATED));
>  }
>  
> +/** scsi_result_is_good - check the result return.
> + *
> + * @result: the result passed up from the driver (including host and
> + *          driver components)
> + *
> + * Drivers may only set other bytes but not status byte.
> + * This checks both the status byte and other bytes.
> + */
> +static inline int scsi_result_is_good(int result)
> +{
> +	return scsi_status_is_good(result) && (result & ~0xff) == 0;
> +}
> +
>  
>  /*
>   * standard mode-select header prepended to all mode-select commands

Instead of introducing a "don't be broken" variant of
scsi_status_is_good(), I'd prefer you to fix the latter to do the right
thing wrt. offline devices.

There aren't a ton of scsi_result_is_good() call sites to check. And I
suspect that most of them wouldn't actually consider the DID_NO_CONNECT
scenario to be "good".
Jason Yan March 27, 2021, 6:57 a.m. UTC | #3
Hi Martin,

在 2021/3/25 10:50, Martin K. Petersen 写道:
> 
> Hi Jason!
> 
>> @@ -55,6 +55,19 @@ static inline int scsi_status_is_good(int status)
>>   		(status == SAM_STAT_COMMAND_TERMINATED));
>>   }
>>   
>> +/** scsi_result_is_good - check the result return.
>> + *
>> + * @result: the result passed up from the driver (including host and
>> + *          driver components)
>> + *
>> + * Drivers may only set other bytes but not status byte.
>> + * This checks both the status byte and other bytes.
>> + */
>> +static inline int scsi_result_is_good(int result)
>> +{
>> +	return scsi_status_is_good(result) && (result & ~0xff) == 0;
>> +}
>> +
>>   
>>   /*
>>    * standard mode-select header prepended to all mode-select commands
> 
> Instead of introducing a "don't be broken" variant of
> scsi_status_is_good(), I'd prefer you to fix the latter to do the right
> thing wrt. offline devices.
> 
> There aren't a ton of scsi_result_is_good() call sites to check. And I
> suspect that most of them wouldn't actually consider the DID_NO_CONNECT
> scenario to be "good".
> 

OK, I'll have a try and check if it works for most of the call sites.

Thanks,
Jason
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index ed0b1bb99f08..16f8cd2895fd 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2669,18 +2669,18 @@  sd_read_write_protect_flag(struct scsi_disk *sdkp, unsigned char *buffer)
 		 * 5: Illegal Request, Sense Code 24: Invalid field in
 		 * CDB.
 		 */
-		if (!scsi_status_is_good(res))
+		if (!scsi_result_is_good(res))
 			res = sd_do_mode_sense(sdkp, 0, 0, buffer, 4, &data, NULL);
 
 		/*
 		 * Third attempt: ask 255 bytes, as we did earlier.
 		 */
-		if (!scsi_status_is_good(res))
+		if (!scsi_result_is_good(res))
 			res = sd_do_mode_sense(sdkp, 0, 0x3F, buffer, 255,
 					       &data, NULL);
 	}
 
-	if (!scsi_status_is_good(res)) {
+	if (!scsi_result_is_good(res)) {
 		sd_first_printk(KERN_WARNING, sdkp,
 			  "Test WP failed, assume Write Enabled\n");
 	} else {
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index e75cca25338a..db0f346a31b2 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -55,6 +55,19 @@  static inline int scsi_status_is_good(int status)
 		(status == SAM_STAT_COMMAND_TERMINATED));
 }
 
+/** scsi_result_is_good - check the result return.
+ *
+ * @result: the result passed up from the driver (including host and
+ *          driver components)
+ *
+ * Drivers may only set other bytes but not status byte.
+ * This checks both the status byte and other bytes.
+ */
+static inline int scsi_result_is_good(int result)
+{
+	return scsi_status_is_good(result) && (result & ~0xff) == 0;
+}
+
 
 /*
  * standard mode-select header prepended to all mode-select commands