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