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

From: "Madhani, Himanshu" <Himanshu.Madhani@cavium.com>
To: Bart Van Assche <bart.vanassche@wdc.com>
Cc: "James.Bottomley@HansenPartnership.com"
	<James.Bottomley@HansenPartnership.com>,
	"martin.petersen@oracle.com" <martin.petersen@oracle.com>,
	"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>
Subject: Re: [PATCH 01/43] qla2xxx: Fix stale memory access for name pointer
Date: Wed, 20 Dec 2017 20:37:26 +0000	[thread overview]
Message-ID: <FD878B9C-451A-4D55-8488-0A14CBD20598@cavium.com> (raw)
In-Reply-To: <1513787147.2603.10.camel@wdc.com>

Hi Bart, 

> On Dec 20, 2017, at 10:25 AM, Bart Van Assche <bart.vanassche@wdc.com> wrote:
> 
> 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”?
> 

Sure. The intention of patch was to cleanup and make sure there is memory allocated
on the stack for name.

>> +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",
> 	...
> };
> 

I will drop this patch from the current submission.

>> --- 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?
> 

Sure. will drop this change

> Thanks,
> 
> Bart.

Thanks,
- Himanshu