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

[Sumit Saxena <sumit.saxena@broadcom.com>]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, May 12, 2016 12:05 PM
> To: Petros Koutoupis
> Cc: Sumit Saxena; Finn Thain; 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 Wed, May 11, 2016 at 08:49:51PM -0500, Petros Koutoupis wrote:
> > 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.
> >
>
> Please send it to the thread.
>
> To be honest, I totally can't understand this thread.  Sumit says it is
impossible
> and you are saying that you have seen it happen in real life.
> Are you using out of tree code or something?  What are you doing that is
> unexpected?
>
> I don't see the point of adding a WARN_ON().  NULL derefs normally
generate a
> pretty clear stack trace already (unless they are caused by memory
corruption).
> Why are we not just fixing the bugs instead of warning and then
crashing.
Agree, if there scsi_cmnd is coming as NULL, please attach logs. I will
look into them.
>
> Also when I'm doing static analysis people always tell me that "that bug
is
> impossible, trust me." and instead of trusting people I really wish they
would just
> show me the relevant code that prevents it from happening.
Inside megasas_build_io_fusion() function,  driver sets "cmd->scmd"
pointer(SCSI command pointer received from SCSI mid layer). Functions
called inside megasas_build_io_fusion()(which actually builds frame to be
sent to firmware)
are setting Function type- MPI2_FUNCTION_SCSI_IO_REQUEST (or)
MEGASAS_MPI2_FUNCTION_LD_IO_REQUEST. So in case Function type set to any
one these two, there must be valid "cmd->scmd".

Thanks,
Sumit

>
> regards,
> dan carpenter