| Message ID | 20230511123432.5793-1-jgross@suse.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | scsi: Let scsi_execute_cmd() mark args->sshdr as invalid | expand |
On 5/11/23 05:34, Juergen Gross wrote: > Some callers of scsi_execute_cmd() (like e.g. sd_spinup_disk()) are > passing an uninitialized struct sshdr and don't look at the return > value of scsi_execute_cmd() before looking at the contents of that > struct. Shouldn't the scsi_execute_cmd() callers be fixed instead of modifying scsi_execute_cmd(), e.g. by zero-initializing the sshdr structure? Thanks, Bart.
On 11.05.23 14:49, Bart Van Assche wrote: > On 5/11/23 05:34, Juergen Gross wrote: >> Some callers of scsi_execute_cmd() (like e.g. sd_spinup_disk()) are >> passing an uninitialized struct sshdr and don't look at the return >> value of scsi_execute_cmd() before looking at the contents of that >> struct. > > Shouldn't the scsi_execute_cmd() callers be fixed instead of modifying > scsi_execute_cmd(), e.g. by zero-initializing the sshdr structure? This would be possible, yes, but introducing new buggy callers could happen. Additionally the amount of code churn would be much larger. Juergen
On Thu, 2023-05-11 at 14:34 +0200, Juergen Gross wrote: > Some callers of scsi_execute_cmd() (like e.g. sd_spinup_disk()) are > passing an uninitialized struct sshdr and don't look at the return > value of scsi_execute_cmd() before looking at the contents of that > struct. > > This can result in false positives when looking for specific error > conditions. > > In order to fix that let scsi_execute_cmd() zero sshdr- > >response_code, > resulting in scsi_sense_valid() returning false. > > Cc: stable@vger.kernel.org > Fixes: 3949e2f04262 ("scsi: simplify scsi_execute_req_flags") > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > I'm not aware of any real error having happened due to this problem, > but I thought it should be fixed anyway. > I _think_ 3949e2f04262 was introducing the problem, but I'm not 100% > sure it is really the commit to be blamed. > --- > drivers/scsi/scsi_lib.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) One nitpick below, otherwise it looks good to me. > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index b7c569a42aa4..923336620bff 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -209,11 +209,17 @@ int scsi_execute_cmd(struct scsi_device *sdev, > const unsigned char *cmd, > struct scsi_cmnd *scmd; > int ret; > > - if (!args) > + if (!args) { > args = &default_args; > - else if (WARN_ON_ONCE(args->sense && > - args->sense_len != > SCSI_SENSE_BUFFERSIZE)) > - return -EINVAL; > + } else { > + /* Mark sense data to be invalid. */ > + if (args->sshdr) > + args->sshdr->response_code = 0; We know for certain that sizeof(*sshdr) is 8 bytes, and will most probably remain so. Thus memset(sshdr, 0, sizeof(*sshdr)) would result in more efficient code. Martin
On 11.05.23 15:10, Martin Wilck wrote: > On Thu, 2023-05-11 at 14:34 +0200, Juergen Gross wrote: >> Some callers of scsi_execute_cmd() (like e.g. sd_spinup_disk()) are >> passing an uninitialized struct sshdr and don't look at the return >> value of scsi_execute_cmd() before looking at the contents of that >> struct. >> >> This can result in false positives when looking for specific error >> conditions. >> >> In order to fix that let scsi_execute_cmd() zero sshdr- >>> response_code, >> resulting in scsi_sense_valid() returning false. >> >> Cc: stable@vger.kernel.org >> Fixes: 3949e2f04262 ("scsi: simplify scsi_execute_req_flags") >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> I'm not aware of any real error having happened due to this problem, >> but I thought it should be fixed anyway. >> I _think_ 3949e2f04262 was introducing the problem, but I'm not 100% >> sure it is really the commit to be blamed. >> --- >> drivers/scsi/scsi_lib.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) > > One nitpick below, otherwise it looks good to me. > >> >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index b7c569a42aa4..923336620bff 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -209,11 +209,17 @@ int scsi_execute_cmd(struct scsi_device *sdev, >> const unsigned char *cmd, >> struct scsi_cmnd *scmd; >> int ret; >> >> - if (!args) >> + if (!args) { >> args = &default_args; >> - else if (WARN_ON_ONCE(args->sense && >> - args->sense_len != >> SCSI_SENSE_BUFFERSIZE)) >> - return -EINVAL; >> + } else { >> + /* Mark sense data to be invalid. */ >> + if (args->sshdr) >> + args->sshdr->response_code = 0; > > We know for certain that sizeof(*sshdr) is 8 bytes, and will most > probably remain so. Thus > > memset(sshdr, 0, sizeof(*sshdr)) > > would result in more efficient code. I fail to see why zeroing a single byte would be less efficient than zeroing a possibly unaligned 8-byte area. Juergen
On Thu, 2023-05-11 at 15:17 +0200, Juergen Gross wrote: > > > > We know for certain that sizeof(*sshdr) is 8 bytes, and will most > > probably remain so. Thus > > > > memset(sshdr, 0, sizeof(*sshdr)) > > > > would result in more efficient code. > > I fail to see why zeroing a single byte would be less efficient than > zeroing > a possibly unaligned 8-byte area. I don't think it can be unaligned. gcc seems to think the same. It compiles the memset(sshdr, ...) in scsi_normalize_sense() into a single instruction on x86_64. 0xffffffff8177e9d0 <scsi_normalize_sense>: nopl 0x0(%rax,%rax,1) [FTRACE NOP] 0xffffffff8177e9d5 <scsi_normalize_sense+5>: test %rdi,%rdi 0xffffffff8177e9d8 <scsi_normalize_sense+8>: movq $0x0,(%rdx) Martin
On 11.05.23 15:23, Martin Wilck wrote: > On Thu, 2023-05-11 at 15:17 +0200, Juergen Gross wrote: >>> >>> We know for certain that sizeof(*sshdr) is 8 bytes, and will most >>> probably remain so. Thus >>> >>> memset(sshdr, 0, sizeof(*sshdr)) >>> >>> would result in more efficient code. >> >> I fail to see why zeroing a single byte would be less efficient than >> zeroing >> a possibly unaligned 8-byte area. > > I don't think it can be unaligned. gcc seems to think the same. It > compiles the memset(sshdr, ...) in scsi_normalize_sense() into a single > instruction on x86_64. > > 0xffffffff8177e9d0 <scsi_normalize_sense>: nopl 0x0(%rax,%rax,1) [FTRACE NOP] > 0xffffffff8177e9d5 <scsi_normalize_sense+5>: test %rdi,%rdi > 0xffffffff8177e9d8 <scsi_normalize_sense+8>: movq $0x0,(%rdx) A struct with 8 "u8" fields can be unaligned. x86_64 can do unaligned 8-byte stores. Other architectures can't (e.g. MIPS). And 32-bit architectures might need 2 stores. Juergen
On Thu, 2023-05-11 at 15:32 +0200, Juergen Gross wrote: > On 11.05.23 15:23, Martin Wilck wrote: > > On Thu, 2023-05-11 at 15:17 +0200, Juergen Gross wrote: > > > > > > > > We know for certain that sizeof(*sshdr) is 8 bytes, and will > > > > most > > > > probably remain so. Thus > > > > > > > > memset(sshdr, 0, sizeof(*sshdr)) > > > > > > > > would result in more efficient code. > > > > > > I fail to see why zeroing a single byte would be less efficient > > > than > > > zeroing > > > a possibly unaligned 8-byte area. > > > > I don't think it can be unaligned. gcc seems to think the same. It > > compiles the memset(sshdr, ...) in scsi_normalize_sense() into a > > single > > instruction on x86_64. > > > > 0xffffffff8177e9d0 <scsi_normalize_sense>: nopl > > 0x0(%rax,%rax,1) [FTRACE NOP] > > 0xffffffff8177e9d5 <scsi_normalize_sense+5>: test %rdi,%rdi > > 0xffffffff8177e9d8 <scsi_normalize_sense+8>: movq $0x0,(%rdx) > > A struct with 8 "u8" fields can be unaligned. Right. I wrongly assumed this would be aligned like an u64. "The alignment of any given struct or union type is required by the ISO C standard to be at least a perfect multiple of the lowest common multiple of the alignments of all of the members of the struct". I wonder if this (non-)alignment of struct scsi_sense_hdr is intentional, but that's a different discussion. Thanks, Martin
On Thu, 2023-05-11 at 14:34 +0200, Juergen Gross wrote: > Some callers of scsi_execute_cmd() (like e.g. sd_spinup_disk()) are > passing an uninitialized struct sshdr and don't look at the return > value of scsi_execute_cmd() before looking at the contents of that > struct. > > This can result in false positives when looking for specific error > conditions. > > In order to fix that let scsi_execute_cmd() zero sshdr- > >response_code, > resulting in scsi_sense_valid() returning false. > > Cc: stable@vger.kernel.org > Fixes: 3949e2f04262 ("scsi: simplify scsi_execute_req_flags") > Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Martin Wilck <mwilck@suse.com> > --- > I'm not aware of any real error having happened due to this problem, > but I thought it should be fixed anyway. > I _think_ 3949e2f04262 was introducing the problem, but I'm not 100% > sure it is really the commit to be blamed. > --- > drivers/scsi/scsi_lib.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index b7c569a42aa4..923336620bff 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -209,11 +209,17 @@ int scsi_execute_cmd(struct scsi_device *sdev, > const unsigned char *cmd, > struct scsi_cmnd *scmd; > int ret; > > - if (!args) > + if (!args) { > args = &default_args; > - else if (WARN_ON_ONCE(args->sense && > - args->sense_len != > SCSI_SENSE_BUFFERSIZE)) > - return -EINVAL; > + } else { > + /* Mark sense data to be invalid. */ > + if (args->sshdr) > + args->sshdr->response_code = 0; > + > + if (WARN_ON_ONCE(args->sense && > + args->sense_len != > SCSI_SENSE_BUFFERSIZE)) > + return -EINVAL; > + } > > req = scsi_alloc_request(sdev->request_queue, opf, args- > >req_flags); > if (IS_ERR(req))
Juergen, > Some callers of scsi_execute_cmd() (like e.g. sd_spinup_disk()) are > passing an uninitialized struct sshdr and don't look at the return > value of scsi_execute_cmd() before looking at the contents of that > struct. Which callers? sd_spinup_disk() appears to do the right thing...
On 17.05.23 04:06, Martin K. Petersen wrote: > > Juergen, > >> Some callers of scsi_execute_cmd() (like e.g. sd_spinup_disk()) are >> passing an uninitialized struct sshdr and don't look at the return >> value of scsi_execute_cmd() before looking at the contents of that >> struct. > > Which callers? sd_spinup_disk() appears to do the right thing... > Not really. It is calling media_not_present() directly after the call of scsi_execute_cmd() without checking the result. media_not_present() is looking at sshdr, which is uninitialized in case of an early error in scsi_execute_cmd(). The same applies to read_capacity_1[06](). scsi_test_unit_ready() and scsi_report_lun_scan() have the problem, too. Do I need to find other examples? Juergen
On 17/05/2023 05:54, Juergen Gross wrote: > On 17.05.23 04:06, Martin K. Petersen wrote: >> >> Juergen, >> >>> Some callers of scsi_execute_cmd() (like e.g. sd_spinup_disk()) are >>> passing an uninitialized struct sshdr and don't look at the return >>> value of scsi_execute_cmd() before looking at the contents of that >>> struct. >> >> Which callers? sd_spinup_disk() appears to do the right thing... >> > > Not really. It is calling media_not_present() directly after the call of > scsi_execute_cmd() without checking the result. Is there a reason that callers of scsi_execute_cmd() are not always checking the result for a negative error code (before examining the buffer)? > media_not_present() is > looking > at sshdr, which is uninitialized in case of an early error in > scsi_execute_cmd(). The same applies to read_capacity_1[06](). > > scsi_test_unit_ready() and scsi_report_lun_scan() have the problem, too. > > Do I need to find other examples? Thanks, John
On 17.05.23 17:05, John Garry wrote: > On 17/05/2023 05:54, Juergen Gross wrote: >> On 17.05.23 04:06, Martin K. Petersen wrote: >>> >>> Juergen, >>> >>>> Some callers of scsi_execute_cmd() (like e.g. sd_spinup_disk()) are >>>> passing an uninitialized struct sshdr and don't look at the return >>>> value of scsi_execute_cmd() before looking at the contents of that >>>> struct. >>> >>> Which callers? sd_spinup_disk() appears to do the right thing... >>> >> >> Not really. It is calling media_not_present() directly after the call of >> scsi_execute_cmd() without checking the result. > > Is there a reason that callers of scsi_execute_cmd() are not always checking the > result for a negative error code (before examining the buffer)? I don't know. I've stumbled over the problem while looking into the code due to analyzing a customer's problem. I'm no SCSI expert, but the customer was running Xen and there was the suspicion this could be an underlying Xen issue (which is my area of interest). It became clear rather quickly that the uninitialized sshdr wasn't the root cause of the customer's problems, but I thought it should be fixed anyway. As there seem to be quite some problematic callers of scsi_execute_cmd(), I've chosen to add the minimal needed initialization of sshdr to scsi_execute_cmd() instead of trying to fix all callers. Reasoning why the code is looking like it does is surely not what _I_ want to do. Juergen
On 18/05/2023 05:53, Juergen Gross wrote: >> >> Is there a reason that callers of scsi_execute_cmd() are not always >> checking the result for a negative error code (before examining the >> buffer)? > > I don't know. > > I've stumbled over the problem while looking into the code due to > analyzing a > customer's problem. I'm no SCSI expert, but the customer was running Xen > and > there was the suspicion this could be an underlying Xen issue (which is my > area of interest). > > It became clear rather quickly that the uninitialized sshdr wasn't the root > cause of the customer's problems, but I thought it should be fixed > anyway. As > there seem to be quite some problematic callers of scsi_execute_cmd(), I've > chosen to add the minimal needed initialization of sshdr to > scsi_execute_cmd() > instead of trying to fix all callers. ok, understood. I am looking through this thread again, and you seem to have to repeat yourself - sorry about that. So I don't think that this code has changed from commit 3949e2, as you say. I think it's better to fix up the callers. Further to that, I dislike how we pass a pointer to this local sshdr structure. I would prefer if scsi_execute_cmd() could kmalloc() the mem for these buffers and the callers could handle free'ing them - I can put together a patch for that, to see what people think. @Martin, Do you have any preference for what we do now? This code which does not check for error and does not pre-zero sshdr is longstanding, so I am not sure if Juergen's change is required for for v6.4. I'm thinking to fix callers for v6.5 and also maybe change the API, as I described. Thanks, John
On 5/18/23 03:57, John Garry wrote: > I think it's better to fix up the callers. +1 > Further to that, I dislike > how we pass a pointer to this local sshdr structure. I would prefer if > scsi_execute_cmd() could kmalloc() the mem for these buffers and the > callers could handle free'ing them - I can put together a patch for > that, to see what people think. sizeof(struct scsi_sense_hdr) = 8. Using kmalloc() to allocate an eight byte data structure sounds like overkill to me. Additionally, making scsi_execute_cmd() allocate struct scsi_sense_hdr and letting the callers free that data structure will make it harder to review whether or not any memory leaks are triggered. No such review is necessary if the scsi_execute_cmd() caller allocates that data structure on the stack. Bart.
On 18/05/2023 20:54, Bart Van Assche wrote: > >> Further to that, I dislike how we pass a pointer to this local sshdr >> structure. I would prefer if scsi_execute_cmd() could kmalloc() the >> mem for these buffers and the callers could handle free'ing them - I >> can put together a patch for that, to see what people think. > > sizeof(struct scsi_sense_hdr) = 8. Using kmalloc() to allocate an eight > byte data structure sounds like overkill to me. Additionally, making > scsi_execute_cmd() allocate struct scsi_sense_hdr and letting the > callers free that data structure will make it harder to review whether > or not any memory leaks are triggered. No such review is necessary if > the scsi_execute_cmd() caller allocates that data structure on the stack. Sure, what I describe is ideal, but I still just dislike passing both sensebuf and hdr into scsi_execute_cmd(). The semantics of how scsi_execute_cmd() treats them is vague. Thanks, John
On 5/19/23 09:06, John Garry wrote: > Sure, what I describe is ideal, but I still just dislike passing both > sensebuf and hdr into scsi_execute_cmd(). The semantics of how > scsi_execute_cmd() treats them is vague. Is this something that can be addressed by improving the scsi_execute_cmd() documentation? Thanks, Bart.
On 19/05/2023 17:54, Bart Van Assche wrote: > On 5/19/23 09:06, John Garry wrote: >> Sure, what I describe is ideal, * not ideal To be clear, I mean something like: struct scsi_exec_args { unsigned char **sense; } scsi_execute_cmd() { ... *args->sense = kmemdup(scsi_cmd->sense_buffer); ... } some_func() { unsigned char *sense = NULL; struct scsi_exec_args = { .sense = &sense, }; ret = scsi_execute_cmd(); if (ret < 0) return ret; kfree(sense); } But not perfect as we need a separate small buffer for sensehdr and we need to always kfree those buffers. If only we could pass the actual scsi_cmnd sense buffer to the caller... >but I still just dislike passing both >> sensebuf and hdr into scsi_execute_cmd(). The semantics of how >> scsi_execute_cmd() treats them is vague. > > Is this something that can be addressed by improving the > scsi_execute_cmd() documentation? Hmmm, I'm not sure documentation helps too much avoiding all programming errors and better make the code foolproof. Anyway, if we fix up the callers of scsi_execute_cmd() to properly check for errors then if is not such a big deal. Thanks, John
On 5/19/23 10:12, John Garry wrote:
> If only we could pass the actual scsi_cmnd sense buffer to the caller...
How about something like the totally untested patch below?
Thanks,
Bart.
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 7bb12deab70c..7ff8d5c263f0 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -379,15 +379,14 @@ static int ata_get_identity(struct ata_port *ap, struct scsi_device *sdev,
int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
{
int rc = 0;
- u8 sensebuf[SCSI_SENSE_BUFFERSIZE];
+ u8 *sensebuf = NULL;
u8 scsi_cmd[MAX_COMMAND_SIZE];
u8 args[4], *argbuf = NULL;
int argsize = 0;
struct scsi_sense_hdr sshdr;
const struct scsi_exec_args exec_args = {
.sshdr = &sshdr,
- .sense = sensebuf,
- .sense_len = sizeof(sensebuf),
+ .sense = &sensebuf,
};
int cmd_result;
@@ -397,7 +396,6 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
if (copy_from_user(args, arg, sizeof(args)))
return -EFAULT;
- memset(sensebuf, 0, sizeof(sensebuf));
memset(scsi_cmd, 0, sizeof(scsi_cmd));
if (args[3]) {
@@ -469,6 +467,7 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
&& copy_to_user(arg + sizeof(args), argbuf, argsize))
rc = -EFAULT;
error:
+ scsi_free_sense_buffer(sensebuf);
kfree(argbuf);
return rc;
}
@@ -487,15 +486,14 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
{
int rc = 0;
- u8 sensebuf[SCSI_SENSE_BUFFERSIZE];
+ u8 *sensebuf = NULL;
u8 scsi_cmd[MAX_COMMAND_SIZE];
u8 args[7];
struct scsi_sense_hdr sshdr;
int cmd_result;
const struct scsi_exec_args exec_args = {
.sshdr = &sshdr,
- .sense = sensebuf,
- .sense_len = sizeof(sensebuf),
+ .sense = &sensebuf,
};
if (arg == NULL)
@@ -504,7 +502,6 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
if (copy_from_user(args, arg, sizeof(args)))
return -EFAULT;
- memset(sensebuf, 0, sizeof(sensebuf));
memset(scsi_cmd, 0, sizeof(scsi_cmd));
scsi_cmd[0] = ATA_16;
scsi_cmd[1] = (3 << 1); /* Non-data */
@@ -557,6 +554,7 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
}
error:
+ scsi_free_sense_buffer(sensebuf);
return rc;
}
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6bc1a9380e69..8197023e9077 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -210,9 +210,6 @@ int scsi_execute_cmd(struct scsi_device *sdev, const unsigned char *cmd,
if (!args)
args = &default_args;
- else if (WARN_ON_ONCE(args->sense &&
- args->sense_len != SCSI_SENSE_BUFFERSIZE))
- return -EINVAL;
req = scsi_alloc_request(sdev->request_queue, opf, args->req_flags);
if (IS_ERR(req))
@@ -248,8 +245,10 @@ int scsi_execute_cmd(struct scsi_device *sdev, const unsigned char *cmd,
if (args->resid)
*args->resid = scmd->resid_len;
- if (args->sense)
- memcpy(args->sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE);
+ if (args->sense) {
+ *args->sense = scmd->sense_buffer;
+ scmd->sense_buffer = NULL;
+ }
if (args->sshdr)
scsi_normalize_sense(scmd->sense_buffer, scmd->sense_len,
args->sshdr);
@@ -1825,6 +1824,12 @@ static int scsi_mq_init_request(struct blk_mq_tag_set *set, struct request *rq,
return ret;
}
+void scsi_free_sense_buffer(u8 *sense_buffer)
+{
+ kmem_cache_free(scsi_sense_cache, sense_buffer);
+}
+EXPORT_SYMBOL_GPL(scsi_free_sense_buffer);
+
static void scsi_mq_exit_request(struct blk_mq_tag_set *set, struct request *rq,
unsigned int hctx_idx)
{
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index ec093594ba53..7c37ef11c71a 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -217,4 +217,6 @@ static inline bool scsi_status_is_good(int status)
(status == SAM_STAT_COMMAND_TERMINATED));
}
+void scsi_free_sense_buffer(u8 *sense_buffer);
+
#endif /* _SCSI_SCSI_H */
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index f10a008e5bfa..9f713fcb150e 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -460,8 +460,7 @@ extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
/* Optional arguments to scsi_execute_cmd */
struct scsi_exec_args {
- unsigned char *sense; /* sense buffer */
- unsigned int sense_len; /* sense buffer len */
+ unsigned char **sense; /* sense buffer */
struct scsi_sense_hdr *sshdr; /* decoded sense header */
blk_mq_req_flags_t req_flags; /* BLK_MQ_REQ flags */
int scmd_flags; /* SCMD flags */
Juergen, >> Which callers? sd_spinup_disk() appears to do the right thing... >> > > Not really. It is calling media_not_present() directly after the call of > scsi_execute_cmd() without checking the result. My bad. I was looking at sd_check_events(), not sd_spinup_disk().
John, > @Martin, Do you have any preference for what we do now? This code > which does not check for error and does not pre-zero sshdr is > longstanding, so I am not sure if Juergen's change is required for for > v6.4. I'm thinking to fix callers for v6.5 and also maybe change the > API, as I described. As I alluded to in the tracing thread, I'd like to see SK/ASC/ASCQ being generally available in the scsi_cmnd results instead of all this sense buffer and sense header micromanagement in every caller. That's a pretty heavy lift, though. Short term we need all callers to be fixed up. I'm not a particularly big fan of scsi_execute_cmd() zeroing something being passed in. I wonder if it would be worth having a DECLARE_SENSE_HEADER()?
On 21.05.23 03:19, Martin K. Petersen wrote: > > John, > >> @Martin, Do you have any preference for what we do now? This code >> which does not check for error and does not pre-zero sshdr is >> longstanding, so I am not sure if Juergen's change is required for for >> v6.4. I'm thinking to fix callers for v6.5 and also maybe change the >> API, as I described. > > As I alluded to in the tracing thread, I'd like to see SK/ASC/ASCQ being > generally available in the scsi_cmnd results instead of all this sense > buffer and sense header micromanagement in every caller. That's a pretty > heavy lift, though. > > Short term we need all callers to be fixed up. I'm not a particularly > big fan of scsi_execute_cmd() zeroing something being passed in. I > wonder if it would be worth having a DECLARE_SENSE_HEADER()? sshdr is output only data, so setting it before returning seems to be a sensible thing to do. Letting the callers do that is kind of a layering violation IMHO, as this would spread the knowledge that scsi_execute_cmd() isn't setting its output data always. In the end its your decision, of course. Juergen
On 19/05/2023 18:39, Bart Van Assche wrote: Hi Bart, > *args->resid = scmd->resid_len; > - if (args->sense) > - memcpy(args->sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); > + if (args->sense) { > + *args->sense = scmd->sense_buffer; > + scmd->sense_buffer = NULL; I think that you will agree that this is not a good pattern to follow. We cannot have SCSI core allocating the sense buffer but a driver freeing it. Thanks, John
On 5/22/23 02:55, John Garry wrote: > On 19/05/2023 18:39, Bart Van Assche wrote: >> *args->resid = scmd->resid_len; >> - if (args->sense) >> - memcpy(args->sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); >> + if (args->sense) { >> + *args->sense = scmd->sense_buffer; >> + scmd->sense_buffer = NULL; > > I think that you will agree that this is not a good pattern to follow. > We cannot have SCSI core allocating the sense buffer but a driver > freeing it. Why not? Something similar can happen anywhere in the kernel anywhere reference counting is used. Bart.
On 22/05/2023 14:31, Bart Van Assche wrote: > On 5/22/23 02:55, John Garry wrote: >> On 19/05/2023 18:39, Bart Van Assche wrote: >>> *args->resid = scmd->resid_len; >>> - if (args->sense) >>> - memcpy(args->sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); >>> + if (args->sense) { >>> + *args->sense = scmd->sense_buffer; >>> + scmd->sense_buffer = NULL; >> >> I think that you will agree that this is not a good pattern to follow. >> We cannot have SCSI core allocating the sense buffer but a driver >> freeing it. > > Why not? Something similar can happen anywhere in the kernel anywhere > reference counting is used. Sure, but you are not using ref counting. If you could use ref counting then it would be better. Thanks, John
Juergen, > sshdr is output only data, so setting it before returning seems to be a > sensible thing to do. I would love to rototill all this and make sense make sense (!) but that's a major undertaking. Until then we need to validate that callers check the return value before they start poking at the sense data. Even if things were zeroed ahead of time, other things could have gone wrong that would affect how an error condition should be handled. > Letting the callers do that is kind of a layering violation IMHO, scsi_execute_cmd() is one big layering violation, I'm afraid.
On 5/22/23 10:54 AM, John Garry wrote: > On 22/05/2023 14:31, Bart Van Assche wrote: >> On 5/22/23 02:55, John Garry wrote: >>> On 19/05/2023 18:39, Bart Van Assche wrote: >>>> *args->resid = scmd->resid_len; >>>> - if (args->sense) >>>> - memcpy(args->sense, scmd->sense_buffer, SCSI_SENSE_BUFFERSIZE); >>>> + if (args->sense) { >>>> + *args->sense = scmd->sense_buffer; >>>> + scmd->sense_buffer = NULL; >>> >>> I think that you will agree that this is not a good pattern to follow. We cannot have SCSI core allocating the sense buffer but a driver freeing it. >> >> Why not? Something similar can happen anywhere in the kernel anywhere reference counting is used. > > Sure, but you are not using ref counting. If you could use ref counting then it would be better. > What about killing the sense buffer arg and doing a callback? For the retries patchset, one issue we had was scsi_execute_cmd callers for the most part just wanted to check different sense/asc/ascq codes. However, there are several places that want to do something more advanced and that's specific to their use. For them, Martin W and I had talked about a callback. For this sense case, the callback can look at the sense buffer scsi-ml creates for all cmds in scsi_mq_init_request, and just copy the values it wants to copy like in ata_task_ioctl. Something like scsi_check_passthrough() ... if (scsi_cmnd->failures->check_failure) scsi_cmnd->failures->check_failure(scmd, &sshdr) ata_task_ioctl() struct scsi_failure *failures = { .check_failure = ata_task_check_failure, .check_args = args, .... bool ata_task_check_failure(struct scsi_cmnd *cmd) { u8 *args = scsi_cmnd->failures->check_args; u8 *sense = cmd->sensebuf; ..... if (scsi_sense_valid(&sshdr)) {/* sense data available */ u8 *desc = sensebuf + 8; /* If we set cc then ATA pass-through will cause a * check condition even if no error. Filter that. */ if (cmd_result & SAM_STAT_CHECK_CONDITION) { if (sshdr.sense_key == RECOVERED_ERROR && sshdr.asc == 0 && sshdr.ascq == 0x1d) cmd_result &= ~SAM_STAT_CHECK_CONDITION; } /* Send userspace ATA registers */ if (sensebuf[0] == 0x72 && /* format is "descriptor" */ desc[0] == 0x09) {/* code is "ATA Descriptor" */ args[0] = desc[13]; /* status */ args[1] = desc[3]; /* error */ args[2] = desc[5]; /* sector count (0:7) */ args[3] = desc[7]; /* lbal */ args[4] = desc[9]; /* lbam */ args[5] = desc[11]; /* lbah */ args[6] = desc[12]; /* select */ } } }
On 5/20/23 8:19 PM, Martin K. Petersen wrote: > As I alluded to in the tracing thread, I'd like to see SK/ASC/ASCQ being > generally available in the scsi_cmnd results instead of all this sense Do you mean you just want a scsi_sense_hdr struct in the scsi_cmnd? If so, I can do this when I resend the scsi retries patches since I have to touch every scsi_execute_cmd user and test them (at least what I can because I couldn't test things like scsi_dh_hp/rdac). For the scsi_execute_cmd issue would we just do: scsi_mq_init_request() { cmd->sshdr = { 0 }; .... } scsi_execute_cmd() blk_execute_rq() if (sshdr) memcpy(sshdr, &scmd->sshdr, sizeof(*sshdr); ? > buffer and sense header micromanagement in every caller. That's a pretty > heavy lift, though.
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index b7c569a42aa4..923336620bff 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -209,11 +209,17 @@ int scsi_execute_cmd(struct scsi_device *sdev, const unsigned char *cmd, struct scsi_cmnd *scmd; int ret; - if (!args) + if (!args) { args = &default_args; - else if (WARN_ON_ONCE(args->sense && - args->sense_len != SCSI_SENSE_BUFFERSIZE)) - return -EINVAL; + } else { + /* Mark sense data to be invalid. */ + if (args->sshdr) + args->sshdr->response_code = 0; + + if (WARN_ON_ONCE(args->sense && + args->sense_len != SCSI_SENSE_BUFFERSIZE)) + return -EINVAL; + } req = scsi_alloc_request(sdev->request_queue, opf, args->req_flags); if (IS_ERR(req))
Some callers of scsi_execute_cmd() (like e.g. sd_spinup_disk()) are passing an uninitialized struct sshdr and don't look at the return value of scsi_execute_cmd() before looking at the contents of that struct. This can result in false positives when looking for specific error conditions. In order to fix that let scsi_execute_cmd() zero sshdr->response_code, resulting in scsi_sense_valid() returning false. Cc: stable@vger.kernel.org Fixes: 3949e2f04262 ("scsi: simplify scsi_execute_req_flags") Signed-off-by: Juergen Gross <jgross@suse.com> --- I'm not aware of any real error having happened due to this problem, but I thought it should be fixed anyway. I _think_ 3949e2f04262 was introducing the problem, but I'm not 100% sure it is really the commit to be blamed. --- drivers/scsi/scsi_lib.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)