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@xxxxxxxxxx> >> Reviewed-by: Christoph Hellwig <hch@xxxxxx> >> Reviewed-by: Martin Wilck <mwilck@xxxxxxxx> >> --- >> 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.
