[PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Juergen Gross]

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(-)

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))
-- 
2.35.3

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Bart Van Assche]

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.

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 620 bytes --]

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



[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Martin Wilck]

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

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2385 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Martin Wilck]

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

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1030 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Martin Wilck]

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

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Martin Wilck]

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))

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Martin K. Petersen]

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...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 777 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[John Garry]

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

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1485 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[John Garry]

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

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Bart Van Assche]

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.

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[John Garry]

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

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Bart Van Assche]

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.

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[John Garry]

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

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Bart Van Assche]

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 */

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Martin K. Petersen]

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().

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Martin K. Petersen]

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()?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1159 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[John Garry]

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

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Bart Van Assche]

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.

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[John Garry]

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

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Martin K. Petersen]

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.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[michael.christie]

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 */

                }
        }

}

	

Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid

[Mike Christie]

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.