> -----Original Message----- > From: Petros Koutoupis [mailto:pet...@petroskoutoupis.com] > Sent: Tuesday, May 10, 2016 2:59 AM > To: Sumit Saxena; Dan Carpenter; Finn Thain > Cc: kashyap.de...@avagotech.com; sumit.sax...@avagotech.com; > uday.ling...@avagotech.com; megaraidlinux....@avagotech.com; linux- > s...@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.carpen...@oracle.com] > > > Sent: Monday, May 09, 2016 1:36 PM > > > To: Finn Thain > > > Cc: Petros Koutoupis; kashyap.de...@avagotech.com; > > > sumit.sax...@avagotech.com; uday.ling...@avagotech.com; > > > megaraidlinux....@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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
