On Sat, 7 May 2016, Petros Koutoupis wrote:
> The current state of the code checks to see if the reference to
> scsi_cmnd is not null, but it never checks to see if it is null and
> always assumes it is valid before its use in below switch statement.
> This patch addresses that.
>
There is no sign-off here.
> --- linux/drivers/scsi/megaraid/megaraid_sas_fusion.c.orig 2016-05-07
> 09:12:56.748969851 -0500
> +++ linux/drivers/scsi/megaraid/megaraid_sas_fusion.c 2016-05-07
> 09:15:29.612967113 -0500
> @@ -2277,6 +2277,10 @@ complete_cmd_fusion(struct megasas_insta
>
> if (cmd_fusion->scmd)
> cmd_fusion->scmd->SCp.ptr = NULL;
> + else if ((!cmd_fusion->scmd) &&
> + ((scsi_io_req->Function ==
> MPI2_FUNCTION_SCSI_IO_REQUEST) ||
> + (scsi_io_req->Function ==
> MEGASAS_MPI2_FUNCTION_LD_IO_REQUEST)))
> + goto next;
That contains a tautology.
>
> scmd_local = cmd_fusion->scmd;
> status = scsi_io_req->RaidContext.status;
> @@ -2336,7 +2340,7 @@ complete_cmd_fusion(struct megasas_insta
> megasas_complete_cmd(instance, cmd_mfi, DID_OK);
> break;
> }
> -
> +next:
> fusion->last_reply_idx[MSIxIndex]++;
> if (fusion->last_reply_idx[MSIxIndex] >=
> fusion->reply_q_depth)
>
>
Your patch is equivalent to this one:
@@ -2294,6 +2294,8 @@
complete(&cmd_fusion->done);
break;
case MPI2_FUNCTION_SCSI_IO_REQUEST: /*Fast Path IO.*/
+ if (unlikely(!scmd_local))
+ break;
/* Update load balancing info */
device_id = MEGASAS_DEV_INDEX(scmd_local);
lbinfo = &fusion->load_balance_info[device_id];
@@ -2311,6 +2313,8 @@
}
/* Fall thru and complete IO */
case MEGASAS_MPI2_FUNCTION_LD_IO_REQUEST: /* LD-IO Path */
+ if (unlikely(!scmd_local))
+ break;
/* Map the FW Cmd Status */
map_cmd_status(cmd_fusion, status, extStatus);
scsi_io_req->RaidContext.status = 0;
I don't know anything about this driver or hardware so I'm not actually
advocating any of these changes, just pointing out that your patch has
issues.
I would have expected the 'smatch' checker to detect either the potential
NULL pointer derefs or alternatively the redundant test for a non-NULL
pointer. Maybe it has detected this already (?) or maybe the NULL pointer
deref can be shown to be impossible?
--
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
