Messages in this thread | | | Date | Thu, 18 May 2023 11:57:36 +0100 | Subject | Re: [PATCH] scsi: Let scsi_execute_cmd() mark args->sshdr as invalid | From | 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
| |