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

Bart Van Assche
2017-12-20T16:25:50Z

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.