diff mbox series

[v10,22/33] scsi: sd: Fix scsi_mode_sense caller's sshdr use

Message ID 20230714213419.95492-23-michael.christie@oracle.com (mailing list archive)
State Changes Requested
Headers show
Series scsi: Allow scsi_execute users to control retries | expand

Commit Message

Mike Christie July 14, 2023, 9:34 p.m. UTC 
The sshdr passed into scsi_execute_cmd is only initialized if
scsi_execute_cmd returns >= 0, and scsi_mode_sense will convert all non
good statuses like check conditions to -EIO. This has scsi_mode_sense
callers that were possibly accessing an uninitialized sshdrs to only
access it if we got -EIO.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/sd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

John Garry July 27, 2023, 7:57 a.m. UTC | #1 
On 14/07/2023 22:34, Mike Christie wrote:
> The sshdr passed into scsi_execute_cmd is only initialized if
> scsi_execute_cmd returns >= 0, and scsi_mode_sense will convert all non
> good statuses like check conditions to -EIO. This has scsi_mode_sense
> callers that were possibly accessing an uninitialized sshdrs to only
> access it if we got -EIO.
> 

sd_read_cache_type() -> sd_do_mode_sense() -> scsi_execute_cmd() may 
never return -EIO without getting as far as init'ing the sshdr, right?

> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/scsi/sd.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index deac35fb520b..53719cf9d86e 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2966,7 +2966,7 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned char *buffer)
>   	}
>   
>   bad_sense:
> -	if (scsi_sense_valid(&sshdr) &&
> +	if (res == -EIO && scsi_sense_valid(&sshdr) &&
>   	    sshdr.sense_key == ILLEGAL_REQUEST &&
>   	    sshdr.asc == 0x24 && sshdr.ascq == 0x0)
>   		/* Invalid field in CDB */
> @@ -3014,7 +3014,7 @@ static void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)
>   		sd_first_printk(KERN_WARNING, sdkp,
>   			  "getting Control mode page failed, assume no ATO\n");
>   
> -		if (scsi_sense_valid(&sshdr))
> +		if (res == -EIO && scsi_sense_valid(&sshdr))
>   			sd_print_sense_hdr(sdkp, &sshdr);
>   
>   		return;
Mike Christie July 31, 2023, 6:14 p.m. UTC | #2 
On 7/27/23 2:57 AM, John Garry wrote:
> On 14/07/2023 22:34, Mike Christie wrote:
>> The sshdr passed into scsi_execute_cmd is only initialized if
>> scsi_execute_cmd returns >= 0, and scsi_mode_sense will convert all non
>> good statuses like check conditions to -EIO. This has scsi_mode_sense
>> callers that were possibly accessing an uninitialized sshdrs to only
>> access it if we got -EIO.
>>
> 
> sd_read_cache_type() -> sd_do_mode_sense() -> scsi_execute_cmd() may never return -EIO without getting as far as init'ing the sshdr, right?
> 

Sorry for the late reply. I'm in and out of vacations.

That's correct.
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index deac35fb520b..53719cf9d86e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2966,7 +2966,7 @@  sd_read_cache_type(struct scsi_disk *sdkp, unsigned char *buffer)
 	}
 
 bad_sense:
-	if (scsi_sense_valid(&sshdr) &&
+	if (res == -EIO && scsi_sense_valid(&sshdr) &&
 	    sshdr.sense_key == ILLEGAL_REQUEST &&
 	    sshdr.asc == 0x24 && sshdr.ascq == 0x0)
 		/* Invalid field in CDB */
@@ -3014,7 +3014,7 @@  static void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)
 		sd_first_printk(KERN_WARNING, sdkp,
 			  "getting Control mode page failed, assume no ATO\n");
 
-		if (scsi_sense_valid(&sshdr))
+		if (res == -EIO && scsi_sense_valid(&sshdr))
 			sd_print_sense_hdr(sdkp, &sshdr);
 
 		return;