Re: [PATCH] megaraid: add scsi_cmnd NULL check before use

[Petros Koutoupis <petros@petroskoutoupis.com>]

On Wed, 2016-05-11 at 15:11 +0530, Sumit Saxena wrote:
> > -----Original Message-----
> > From: Petros Koutoupis [mailto:petros@petroskoutoupis.com]
> > Sent: Tuesday, May 10, 2016 2:59 AM
> > To: Sumit Saxena; Dan Carpenter; Finn Thain
> > Cc: kashyap.desai@avagotech.com; sumit.saxena@avagotech.com;
> > uday.lingala@avagotech.com; megaraidlinux.pdl@avagotech.com; linux-
> > scsi@vger.kernel.org
> > Subject: Re: [PATCH] megaraid: add scsi_cmnd NULL check before use
> >
> > On Mon, 2016-05-09 at 15:18 +0530, Sumit Saxena wrote:
> > > >
> > > > -----Original Message-----
> > > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > > Sent: Monday, May 09, 2016 1:36 PM
> > > > To: Finn Thain
> > > > Cc: Petros Koutoupis; kashyap.desai@avagotech.com;
> > > > sumit.saxena@avagotech.com; uday.lingala@avagotech.com;
> > > > megaraidlinux.pdl@avagotech.com; linux-scsi@vger.kernel.org
> > > > Subject: Re: [PATCH] megaraid: add scsi_cmnd NULL check before use
> > > >
> > > > Smatch doesn't quite catch it because we check "cmd_fusion->scmd"
> > > > for
> > > NULL
> > > >
> > > > then assign "scmd_local = cmd_fusion->scmd;" and dereference
> > > > scmd_local unconditionally...
> > > >
> > > > It does catch part of the bug if you have cross function analysis:
> > > >
> > > >   drivers/scsi/megaraid/megaraid_sas_fusion.c:2318
> > > > complete_cmd_fusion()
> > > >   error: we previously assumed 'cmd_fusion->scmd' could be null (see
> > > line 2281)
> > > >
> > > >
> > > > But that code was from 2010 so I never reported it to the original
> > > author or the
> > > >
> > > > list.
> > > "cmd_fusion->scmd" should not be NULL if scsi_io_req->Function is set
> > > to MPI2_FUNCTION_SCSI_IO_REQUEST (OR)
> > > MEGASAS_MPI2_FUNCTION_LD_IO_REQUES
> > > (inside these two cases only, cmd_fusion->scmd will be dereferenced).
> > > If cmd_fusion->scmd is NULL for these "scsi_io_req->Function", that
> > > will a BUG and should not continue with other commands processing in
> > > that case.
> > >
> >
> > Sumit,
> >
> > To clarify, a detection of cmd_fusion->scmd being NULL with scsi_io_req-
> > >Function set to MPI2_FUNCTION_SCSI_IO_REQUEST or
> > MEGASAS_MPI2_FUNCTION_LD_IO_REQUEST should instead trigger a
> > BUG() and not attempt to iterate to the next command in the list. Thank
> > you.
> 
> Petros,
> 
> WARN_ON() can be used in this case. Upstream may have concerns on using
> BUG_ON() and also BUG_ON() won't help in this case. In production
> environment we never encountered this.
> 
> Thanks,
> Sumit
> >
> > --
> > Petros

Sumit,

I will resubmit the patch with all the recommendations. Thank you. In case
you are interested, I have a crash file showcasing the error. I can always
provide this outside of this mailing thread.

--
Petros