| Message ID | 20231004210013.5601-9-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | scsi: sshdr and retry fixes | expand |
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 ? > 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;
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.
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. > > >
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 --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;