> -----Original Message----- > From: Finn Thain [mailto:fthain@xxxxxxxxxxxxxxxxxxx] > Sent: Friday, May 13, 2016 1:14 PM > To: Sumit Saxena > Cc: Dan Carpenter; Petros Koutoupis; kashyap.desai@xxxxxxxxxxxxx; > sumit.saxena@xxxxxxxxxxxxx; uday.lingala@xxxxxxxxxxxxx; > megaraidlinux.pdl@xxxxxxxxxxxxx; linux-scsi@xxxxxxxxxxxxxxx > Subject: RE: [PATCH] megaraid: add scsi_cmnd NULL check before use > > > On Thu, 12 May 2016, Sumit Saxena wrote: > > > > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > > > > > > 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". > > That doesn't show what prevents the bug. It merely shows that the bug does not > always manifest. > > For example, you might check whether anything prevents > megasas_build_io_fusion() from returning before assigning cmd->scmd, like > so: > > 2112 if (sge_count > instance->max_num_sge) { > 2113 dev_err(&instance->pdev->dev, "Error. sge_count (0x%x) > exceeds " > 2114 "max (0x%x) allowed\n", sge_count, > 2115 instance->max_num_sge); > 2116 return 1; > 2117 } > > Another possibility: cmd->io_request->Function is valid yet cmd->scmd is NULL > when seen from the interrupt handler if it intervenes between the two > statements in megasas_return_cmd_fusion(): For IOs returned by above error are not actually fired to firmware so there will be no interrupt handler called for the same. Anyways, if anyone has logs pertaining to the failure, please attach.. > > 180 inline void megasas_return_cmd_fusion(struct megasas_instance *instance, > 181 struct megasas_cmd_fusion *cmd) > 182 { > 183 cmd->scmd = NULL; > 184 memset(cmd->io_request, 0, sizeof(struct > MPI2_RAID_SCSI_IO_REQUEST)); > 185 } > > You might want to confirm that locking always prevents that. > > OTOH, without an actual backtrace, I too might be reluctant to pursue this kind > of speculation. > > -- -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
