Re: [PATCH 01/43] qla2xxx: Fix stale memory access for name pointer

[Bart Van Assche <Bart.VanAssche@wdc.com>]

On Tue, 2017-12-19 at 22:56 -0800, Himanshu Madhani wrote:
> Name pointer for sp describing each command is assigned with stack
> frame's memory. The stack frame could eventually be re-use, where
> name pointer access can get get garbage. This patch designates
> static memory for name pointer to fix this problem.

Which stack memory accesses have been removed by this patch? Sorry but I
haven't found any stack memory access changes in this patch. Additionally,
I haven't found any changes in this patch that look useful to me. Are you
aware that for statements like "str = "unknown"" the compiler allocates
static memory for the string "unknown"?

> +struct sp_name {
> +	uint16_t cmd;
> +	const char *str;
> +};
> +

[ ... ]
 
> +struct sp_name sp_str[] = {
> +	{ SPCN_UNKNOWN, "unknown" },
> +	{ SPCN_GIDPN, "gidpn" },
> +	{ SPCN_GPSC, "gpsc" },
> +	{ SPCN_GPNID, "gpnid" },
> +	{ SPCN_GPNFT, "gpnft" },
> +	{ SPCN_GNNID, "gnnid" },
> +	{ SPCN_GFPNID, "gfpnid" },
> +	{ SPCN_GFFID, "gffid" },
> +	{ SPCN_LOGIN, "login" },
> +	{ SPCN_LOGOUT, "logout" },
> +	{ SPCN_ADISC, "adisc" },
> +	{ SPCN_GNLIST, "gnlist" },
> +	{ SPCN_GPDB, "gpdb" },
> +	{ SPCN_TMF, "tmf" },
> +	{ SPCN_ABORT, "abort" },
> +	{ SPCN_NACK, "nack" },
> +	{ SPCN_BSG_RPT, "bsg_els_rpt" },
> +	{ SPCN_BSG_HST, "bsg_els_hst" },
> +	{ SPCN_BSG_CT, "bsg_ct" },
> +	{ SPCN_BSG_FX_MGMT, "bsg_fx_mgmt" },
> +	{ SPCN_ELS_DCMD, "ELS_DCMD" },
> +	{ SPCN_FXDISC, "fxdisc" },
> +	{ SPCN_PRLI, "prli" },
> +	{ SPCN_NVME_LS, "nvme_ls" },
> +	{ SPCN_NVME_CMD, "nvme_cmd" },
> +};

If you want to keep the sp_str[] array after what I wrote above, please
remove the sp_name structure and change sp_str[] into something like the
following:

static const char *sp_str[] = {
	[SPCN_UNKNOWN] = "unknown",
	...
};

> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -14,6 +14,7 @@ static struct mb_cmd_name {
>  	uint16_t cmd;
>  	const char *str;
>  } mb_str[] = {
> +	{0, "unknown mb"},
>  	{MBC_GET_PORT_DATABASE,		"GPDB"},
>  	{MBC_GET_ID_LIST,		"GIDList"},
>  	{MBC_GET_LINK_PRIV_STATS,	"Stats"},
> @@ -24,12 +25,12 @@ static const char *mb_to_str(uint16_t cmd)
>  	int i;
>  	struct mb_cmd_name *e;
>  
> -	for (i = 0; i < ARRAY_SIZE(mb_str); i++) {
> +	for (i = 1; i < ARRAY_SIZE(mb_str); i++) {
>  		e = mb_str + i;
>  		if (cmd == e->cmd)
>  			return e->str;
>  	}
> -	return "unknown";
> +	return mb_str[0].str;
>  }

Sorry but the above change does not look useful to me in any way. Is this
just code churn?

Thanks,

Bart.