diff mbox series

[v2,08/12] scsi: sd: Fix scsi_mode_sense caller's sshdr use

Message ID 20231004210013.5601-9-michael.christie@oracle.com
State New
Headers show
Series scsi: sshdr and retry fixes | expand

Commit Message

Mike Christie Oct. 4, 2023, 9 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>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/sd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Damien Le Moal Oct. 6, 2023, 1:12 a.m. UTC | #1
On 10/6/23 09:36, Mike Christie wrote:
> On 10/4/23 5:37 PM, Damien Le Moal wrote:
>> On 10/5/23 06:00, 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.
>>>
>>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> Reviewed-by: Martin Wilck <mwilck@suse.com>
>>> ---
>>>  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 6d4787ff6e96..538ebdf42c69 100644
>>> --- a/drivers/scsi/sd.c
>>> +++ b/drivers/scsi/sd.c
>>> @@ -2942,7 +2942,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) &&
>>
>> 	if (ret < 0 && ...
>>
>> would be safer and avoid any issue if we ever change scsi_execute_cmd() to
>> return other error codes than -EIO, no ?
> 
> If we do that, then we will have the same problem we have today
> where we can access the sshdr when it's not setup.
> 
> If scsi_execute_cmd returns < 0, then the sshdr is not setup, so
> we shouldn't access it. The res value above is from scsi_mode_sense
> which actually does the scsi_execute_cmd call, but it doesn't always
> pass the return vale from scsi_execute_cmd directly to its callers.
> 
> If there is valid sense then scsi_mode_sense returns -EIO so above
> that's why we check for that return code.
> 
> As far as future safety goes, this patch is not great. Right now
> we assume scsi_execute_cmd and the functions it calls does not
> return -EIO. To make it safer we could change scsi_mode_sense to
> return 1 for the case there is sense or add another arg which
> gets set when there is sense.

Indeed, that would be better because scsi does not prevent a device from
returning sense data for successfull commands as well (see device statistics or
CDL as examples). So that would be a better solution than relying on -EIO for
sense data validity.

> 
> 
>
Mike Christie Oct. 6, 2023, 10:39 p.m. UTC | #2
On 10/5/23 8:12 PM, Damien Le Moal wrote:
>> As far as future safety goes, this patch is not great. Right now
>> we assume scsi_execute_cmd and the functions it calls does not
>> return -EIO. To make it safer we could change scsi_mode_sense to
>> return 1 for the case there is sense or add another arg which
>> gets set when there is sense.
> Indeed, that would be better because scsi does not prevent a device from
> returning sense data for successfull commands as well (see device statistics or
> CDL as examples). So that would be a better solution than relying on -EIO for
> sense data validi

For the sd_read_app_tag_own case and similar ones are you wanting to
log the sense for cases like the CDL where scsi_execute_cmd returns 0?

The thing is that this patches uses -EIO to indicate if the sshdr is
setup (not exactly that sense is valid). If it is then we check if the sense
is valid and if is then sd_read_app_tag_own will log it.

In other places we do something similar and check if
scsi_execute_cmd returns > 0. If it does then we know the sshdr is setup
and will do something with it like print it. Those cases will not work
for your CDL example because scsi_execute_cmd returns 0.
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6d4787ff6e96..538ebdf42c69 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2942,7 +2942,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 */
@@ -2990,7 +2990,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;