* [PATCH 0/3] scsi: check the whole result in some places
@ 2021-03-19 3:01 Jason Yan
2021-03-19 3:01 ` [PATCH 1/3] scsi: check the whole result for reading write protect flag Jason Yan
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jason Yan @ 2021-03-19 3:01 UTC (permalink / raw)
To: axboe, ming.lei, hch, keescook, kbusch, linux-block,
martin.petersen, jejb
Cc: linux-scsi, Jason Yan
Some bugs have been found related to the result of the command been
returned by the driver or the middle layer in some cases. Such as in
scsi_queue_rq() when the device is offline only the host byte is set.
Jason Yan (3):
scsi: check the whole result for reading write protect flag
scsi: only copy data to user when the whole result is good
scsi: switch to use scsi_result_is_good() in
scsi_result_to_blk_status()
block/scsi_ioctl.c | 2 +-
drivers/scsi/scsi_lib.c | 2 +-
drivers/scsi/sd.c | 6 +++---
include/scsi/scsi.h | 13 +++++++++++++
4 files changed, 18 insertions(+), 5 deletions(-)
--
2.25.4
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] scsi: check the whole result for reading write protect flag 2021-03-19 3:01 [PATCH 0/3] scsi: check the whole result in some places Jason Yan @ 2021-03-19 3:01 ` Jason Yan 2021-03-19 7:55 ` Hannes Reinecke 2021-03-25 2:50 ` Martin K. Petersen 2021-03-19 3:01 ` [PATCH 2/3] scsi: only copy data to user when the whole result is good Jason Yan 2021-03-19 3:01 ` [PATCH 3/3] scsi: switch to use scsi_result_is_good() in scsi_result_to_blk_status() Jason Yan 2 siblings, 2 replies; 10+ messages in thread From: Jason Yan @ 2021-03-19 3:01 UTC (permalink / raw) To: axboe, ming.lei, hch, keescook, kbusch, linux-block, martin.petersen, jejb Cc: linux-scsi, Jason Yan 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 -- 2.25.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] scsi: check the whole result for reading write protect flag 2021-03-19 3:01 ` [PATCH 1/3] scsi: check the whole result for reading write protect flag Jason Yan @ 2021-03-19 7:55 ` Hannes Reinecke 2021-03-25 2:50 ` Martin K. Petersen 1 sibling, 0 replies; 10+ messages in thread From: Hannes Reinecke @ 2021-03-19 7:55 UTC (permalink / raw) To: Jason Yan, axboe, ming.lei, hch, keescook, kbusch, linux-block, martin.petersen, jejb Cc: linux-scsi 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 -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] scsi: check the whole result for reading write protect flag 2021-03-19 3:01 ` [PATCH 1/3] scsi: check the whole result for reading write protect flag Jason Yan 2021-03-19 7:55 ` Hannes Reinecke @ 2021-03-25 2:50 ` Martin K. Petersen 2021-03-27 6:57 ` Jason Yan 1 sibling, 1 reply; 10+ messages in thread From: Martin K. Petersen @ 2021-03-25 2:50 UTC (permalink / raw) To: Jason Yan Cc: axboe, ming.lei, hch, keescook, kbusch, linux-block, martin.petersen, jejb, linux-scsi 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". -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] scsi: check the whole result for reading write protect flag 2021-03-25 2:50 ` Martin K. Petersen @ 2021-03-27 6:57 ` Jason Yan 0 siblings, 0 replies; 10+ messages in thread From: Jason Yan @ 2021-03-27 6:57 UTC (permalink / raw) To: Martin K. Petersen Cc: axboe, ming.lei, hch, keescook, kbusch, linux-block, jejb, linux-scsi 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] scsi: only copy data to user when the whole result is good 2021-03-19 3:01 [PATCH 0/3] scsi: check the whole result in some places Jason Yan 2021-03-19 3:01 ` [PATCH 1/3] scsi: check the whole result for reading write protect flag Jason Yan @ 2021-03-19 3:01 ` Jason Yan 2021-03-19 7:56 ` Hannes Reinecke 2021-03-19 3:01 ` [PATCH 3/3] scsi: switch to use scsi_result_is_good() in scsi_result_to_blk_status() Jason Yan 2 siblings, 1 reply; 10+ messages in thread From: Jason Yan @ 2021-03-19 3:01 UTC (permalink / raw) To: axboe, ming.lei, hch, keescook, kbusch, linux-block, martin.petersen, jejb Cc: linux-scsi, Jason Yan When the scsi device status is offline, mode sense command will return a result with only DID_NO_CONNECT set. Then in sg_scsi_ioctl(), only status byte of the result is checked, and because of bug [1], garbage data is copied to the userspace. Only copy the buffer to userspace when the whole result is good. [1] https://patchwork.kernel.org/project/linux-block/patch/20210318122621.330010-1-yanaijie@huawei.com/ Signed-off-by: Jason Yan <yanaijie@huawei.com> --- block/scsi_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c index 6599bac0a78c..359bf0003af4 100644 --- a/block/scsi_ioctl.c +++ b/block/scsi_ioctl.c @@ -503,7 +503,7 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode, if (copy_to_user(sic->data, req->sense, bytes)) err = -EFAULT; } - } else { + } else if (scsi_result_is_good(req->result)) { if (copy_to_user(sic->data, buffer, out_len)) err = -EFAULT; } -- 2.25.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] scsi: only copy data to user when the whole result is good 2021-03-19 3:01 ` [PATCH 2/3] scsi: only copy data to user when the whole result is good Jason Yan @ 2021-03-19 7:56 ` Hannes Reinecke 2021-03-19 8:22 ` Jason Yan 0 siblings, 1 reply; 10+ messages in thread From: Hannes Reinecke @ 2021-03-19 7:56 UTC (permalink / raw) To: Jason Yan, axboe, ming.lei, hch, keescook, kbusch, linux-block, martin.petersen, jejb Cc: linux-scsi 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 sg_scsi_ioctl(), > only status byte of the result is checked, and because of > bug [1], garbage data is copied to the userspace. > > Only copy the buffer to userspace when the whole result is good. > > [1] https://patchwork.kernel.org/project/linux-block/patch/20210318122621.330010-1-yanaijie@huawei.com/ > > Signed-off-by: Jason Yan <yanaijie@huawei.com> > --- > block/scsi_ioctl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c > index 6599bac0a78c..359bf0003af4 100644 > --- a/block/scsi_ioctl.c > +++ b/block/scsi_ioctl.c > @@ -503,7 +503,7 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode, > if (copy_to_user(sic->data, req->sense, bytes)) > err = -EFAULT; > } > - } else { > + } else if (scsi_result_is_good(req->result)) { > if (copy_to_user(sic->data, buffer, out_len)) > err = -EFAULT; > } > Hmm. Not sure about this one. The prime motivator behind sg is to get _precisely_ all flags of the command, and not do in-kernel error handling. So one could argue that this behaviour is intentional, and would break existing use-cases. Doug? Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] scsi: only copy data to user when the whole result is good 2021-03-19 7:56 ` Hannes Reinecke @ 2021-03-19 8:22 ` Jason Yan 0 siblings, 0 replies; 10+ messages in thread From: Jason Yan @ 2021-03-19 8:22 UTC (permalink / raw) To: Hannes Reinecke, axboe, ming.lei, hch, keescook, kbusch, linux-block, martin.petersen, jejb Cc: linux-scsi Hi Hannes, 在 2021/3/19 15:56, Hannes Reinecke 写道: > 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 sg_scsi_ioctl(), >> only status byte of the result is checked, and because of >> bug [1], garbage data is copied to the userspace. >> >> Only copy the buffer to userspace when the whole result is good. >> >> [1] >> https://patchwork.kernel.org/project/linux-block/patch/20210318122621.330010-1-yanaijie@huawei.com/ >> >> >> Signed-off-by: Jason Yan <yanaijie@huawei.com> >> --- >> block/scsi_ioctl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c >> index 6599bac0a78c..359bf0003af4 100644 >> --- a/block/scsi_ioctl.c >> +++ b/block/scsi_ioctl.c >> @@ -503,7 +503,7 @@ int sg_scsi_ioctl(struct request_queue *q, struct >> gendisk *disk, fmode_t mode, >> if (copy_to_user(sic->data, req->sense, bytes)) >> err = -EFAULT; >> } >> - } else { >> + } else if (scsi_result_is_good(req->result)) { >> if (copy_to_user(sic->data, buffer, out_len)) >> err = -EFAULT; >> } >> > Hmm. Not sure about this one. > The prime motivator behind sg is to get _precisely_ all flags of the > command, and not do in-kernel error handling. > So one could argue that this behaviour is intentional, and would break > existing use-cases. > Thanks for the review. The existing usersapce can do nothing with the uninitialized data. Or the driver or disk may fill some data and at the same time set host_byte or driver_byte to non-zero? I'm not sure about this. And the return value of sg_scsi_ioctl() just get the status byte(only 8 bit), how can the users know about this situation? Thanks, Jason > Doug? > > Cheers, > > Hannes ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] scsi: switch to use scsi_result_is_good() in scsi_result_to_blk_status() 2021-03-19 3:01 [PATCH 0/3] scsi: check the whole result in some places Jason Yan 2021-03-19 3:01 ` [PATCH 1/3] scsi: check the whole result for reading write protect flag Jason Yan 2021-03-19 3:01 ` [PATCH 2/3] scsi: only copy data to user when the whole result is good Jason Yan @ 2021-03-19 3:01 ` Jason Yan 2021-03-19 7:56 ` Hannes Reinecke 2 siblings, 1 reply; 10+ messages in thread From: Jason Yan @ 2021-03-19 3:01 UTC (permalink / raw) To: axboe, ming.lei, hch, keescook, kbusch, linux-block, martin.petersen, jejb Cc: linux-scsi, Jason Yan Use scsi_result_is_good() to simplify the code. Signed-off-by: Jason Yan <yanaijie@huawei.com> --- drivers/scsi/scsi_lib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 7d52a11e1b61..d1ec9f6a93f0 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -626,7 +626,7 @@ static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result) * to handle the case when a SCSI LLD sets result to * DRIVER_SENSE << 24 without setting SAM_STAT_CHECK_CONDITION. */ - if (scsi_status_is_good(result) && (result & ~0xff) == 0) + if (scsi_result_is_good(result)) return BLK_STS_OK; return BLK_STS_IOERR; case DID_TRANSPORT_FAILFAST: -- 2.25.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] scsi: switch to use scsi_result_is_good() in scsi_result_to_blk_status() 2021-03-19 3:01 ` [PATCH 3/3] scsi: switch to use scsi_result_is_good() in scsi_result_to_blk_status() Jason Yan @ 2021-03-19 7:56 ` Hannes Reinecke 0 siblings, 0 replies; 10+ messages in thread From: Hannes Reinecke @ 2021-03-19 7:56 UTC (permalink / raw) To: Jason Yan, axboe, ming.lei, hch, keescook, kbusch, linux-block, martin.petersen, jejb Cc: linux-scsi On 3/19/21 4:01 AM, Jason Yan wrote: > Use scsi_result_is_good() to simplify the code. > > Signed-off-by: Jason Yan <yanaijie@huawei.com> > --- > drivers/scsi/scsi_lib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 7d52a11e1b61..d1ec9f6a93f0 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -626,7 +626,7 @@ static blk_status_t scsi_result_to_blk_status(struct scsi_cmnd *cmd, int result) > * to handle the case when a SCSI LLD sets result to > * DRIVER_SENSE << 24 without setting SAM_STAT_CHECK_CONDITION. > */ > - if (scsi_status_is_good(result) && (result & ~0xff) == 0) > + if (scsi_result_is_good(result)) > return BLK_STS_OK; > return BLK_STS_IOERR; > case DID_TRANSPORT_FAILFAST: > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-03-27 6:58 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-19 3:01 [PATCH 0/3] scsi: check the whole result in some places Jason Yan 2021-03-19 3:01 ` [PATCH 1/3] scsi: check the whole result for reading write protect flag Jason Yan 2021-03-19 7:55 ` Hannes Reinecke 2021-03-25 2:50 ` Martin K. Petersen 2021-03-27 6:57 ` Jason Yan 2021-03-19 3:01 ` [PATCH 2/3] scsi: only copy data to user when the whole result is good Jason Yan 2021-03-19 7:56 ` Hannes Reinecke 2021-03-19 8:22 ` Jason Yan 2021-03-19 3:01 ` [PATCH 3/3] scsi: switch to use scsi_result_is_good() in scsi_result_to_blk_status() Jason Yan 2021-03-19 7:56 ` Hannes Reinecke
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).