| Message ID | 20210202235118.GA314410@embeddedor |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,next] scsi: mpt3sas: Replace one-element array with flexible-array in struct _MPI2_CONFIG_PAGE_IO_UNIT_3 | expand |
Hi all, Friendly ping: who can review/take this, please? Thanks! -- Gustavo On Tue, Feb 02, 2021 at 05:51:18PM -0600, Gustavo A. R. Silva wrote: > There is a regular need in the kernel to provide a way to declare having > a dynamically sized set of trailing elements in a structure. Kernel code > should always use “flexible array members”[1] for these cases. The older > style of one-element or zero-length arrays should no longer be used[2]. > > Refactor the code according to the use of a flexible-array member in > struct _MPI2_CONFIG_PAGE_IO_UNIT_3, instead of a one-element array, > and use the struct_size() helper to calculate the size for the > allocation. > > Also, this helps the ongoing efforts to enable -Warray-bounds and fix the > following warnings: > > drivers/scsi/mpt3sas/mpt3sas_ctl.c:3193:63: warning: array subscript 24 > is above array bounds of ‘U16[1]’ {aka ‘short unsigned int[1]’} > [-Warray-bounds] > > [1] https://en.wikipedia.org/wiki/Flexible_array_member > [2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/109 > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > Changes in v2: > - Fix format specifier: use %zu for size_t type. > > drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h | 11 +---------- > drivers/scsi/mpt3sas/mpt3sas_ctl.c | 6 +++--- > 2 files changed, 4 insertions(+), 13 deletions(-) > > diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h > index 43a3bf8ff428..908b0ca63204 100644 > --- a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h > +++ b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h > @@ -987,21 +987,12 @@ typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_1 { > > /*IO Unit Page 3 */ > > -/* > - *Host code (drivers, BIOS, utilities, etc.) should leave this define set to > - *one and check the value returned for GPIOCount at runtime. > - */ > -#ifndef MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX > -#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (1) > -#endif > - > typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_3 { > MPI2_CONFIG_PAGE_HEADER Header; /*0x00 */ > U8 GPIOCount; /*0x04 */ > U8 Reserved1; /*0x05 */ > U16 Reserved2; /*0x06 */ > - U16 > - GPIOVal[MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX];/*0x08 */ > + U16 GPIOVal[]; /*0x08 */ > } MPI2_CONFIG_PAGE_IO_UNIT_3, > *PTR_MPI2_CONFIG_PAGE_IO_UNIT_3, > Mpi2IOUnitPage3_t, *pMpi2IOUnitPage3_t; > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > index c8a0ce18f2c5..ffb21f873058 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > @@ -3143,7 +3143,7 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, > Mpi2ConfigReply_t mpi_reply; > u16 backup_rail_monitor_status = 0; > u16 ioc_status; > - int sz; > + size_t sz; > ssize_t rc = 0; > > if (!ioc->is_warpdrive) { > @@ -3157,11 +3157,11 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, > goto out; > > /* allocate upto GPIOVal 36 entries */ > - sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36); > + sz = struct_size(io_unit_pg3, GPIOVal, 36); > io_unit_pg3 = kzalloc(sz, GFP_KERNEL); > if (!io_unit_pg3) { > rc = -ENOMEM; > - ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n", > + ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%zu) bytes\n", > __func__, sz); > goto out; > } > -- > 2.27.0 >
On Mon, 2021-03-08 at 13:32 -0600, Gustavo A. R. Silva wrote: > Hi all, > > Friendly ping: who can review/take this, please? Well, before embarking on a huge dynamic update, let's ask Broadcom the simpler question of why isn't MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX simply set to 36? There's no dynamic allocation of anything in the current code ... it's all hard coded to allocate 36 entries. If there's no need for anything dynamic then the kzalloc could become io_unit_pg3 = kzalloc(sizeof(*io_unit_pg3), GFP_KERNEL); James
On Mon, Mar 08, 2021 at 12:12:59PM -0800, James Bottomley wrote: > On Mon, 2021-03-08 at 13:32 -0600, Gustavo A. R. Silva wrote: > > Hi all, > > > > Friendly ping: who can review/take this, please? > > Well, before embarking on a huge dynamic update, let's ask Broadcom the > simpler question of why isn't MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX simply > set to 36? There's no dynamic allocation of anything in the current > code ... it's all hard coded to allocate 36 entries. If there's no > need for anything dynamic then the kzalloc could become Yeah; and if that is the case, then there is no even need for kzalloc() at all, and it can be replaced by memset(): diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h index 43a3bf8ff428..d00431f553e1 100644 --- a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h +++ b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h @@ -992,7 +992,7 @@ typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_1 { *one and check the value returned for GPIOCount at runtime. */ #ifndef MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX -#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (1) +#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (36) #endif typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_3 { diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c index 44f9a05db94e..23fcf29bfd67 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c @@ -3203,7 +3203,7 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, { struct Scsi_Host *shost = class_to_shost(cdev); struct MPT3SAS_ADAPTER *ioc = shost_priv(shost); - Mpi2IOUnitPage3_t *io_unit_pg3 = NULL; + Mpi2IOUnitPage3_t io_unit_pg3; Mpi2ConfigReply_t mpi_reply; u16 backup_rail_monitor_status = 0; u16 ioc_status; @@ -3221,16 +3221,10 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, goto out; /* allocate upto GPIOVal 36 entries */ - sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36); - io_unit_pg3 = kzalloc(sz, GFP_KERNEL); - if (!io_unit_pg3) { - rc = -ENOMEM; - ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n", - __func__, sz); - goto out; - } + sz = sizeof(io_unit_pg3); + memset(&io_unit_pg3, 0, sz); - if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, io_unit_pg3, sz) != + if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, &io_unit_pg3, sz) != 0) { ioc_err(ioc, "%s: failed reading iounit_pg3\n", __func__); @@ -3246,19 +3240,18 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, goto out; } - if (io_unit_pg3->GPIOCount < 25) { - ioc_err(ioc, "%s: iounit_pg3->GPIOCount less than 25 entries, detected (%d) entries\n", - __func__, io_unit_pg3->GPIOCount); + if (io_unit_pg3.GPIOCount < 25) { + ioc_err(ioc, "%s: iounit_pg3.GPIOCount less than 25 entries, detected (%d) entries\n", + __func__, io_unit_pg3.GPIOCount); rc = -EINVAL; goto out; } /* BRM status is in bit zero of GPIOVal[24] */ - backup_rail_monitor_status = le16_to_cpu(io_unit_pg3->GPIOVal[24]); + backup_rail_monitor_status = le16_to_cpu(io_unit_pg3.GPIOVal[24]); rc = snprintf(buf, PAGE_SIZE, "%d\n", (backup_rail_monitor_status & 1)); out: - kfree(io_unit_pg3); mutex_unlock(&ioc->pci_access_mutex); return rc; } > > io_unit_pg3 = kzalloc(sizeof(*io_unit_pg3), GFP_KERNEL); > Thanks -- Gustavo
On Mon, Mar 08, 2021 at 02:41:29PM -0600, Gustavo A. R. Silva wrote: > On Mon, Mar 08, 2021 at 12:12:59PM -0800, James Bottomley wrote: > > On Mon, 2021-03-08 at 13:32 -0600, Gustavo A. R. Silva wrote: > > > Hi all, > > > > > > Friendly ping: who can review/take this, please? > > > > Well, before embarking on a huge dynamic update, let's ask Broadcom the > > simpler question of why isn't MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX simply > > set to 36? There's no dynamic allocation of anything in the current > > code ... it's all hard coded to allocate 36 entries. If there's no > > need for anything dynamic then the kzalloc could become > > Yeah; and if that is the case, then there is no even need for kzalloc() > at all, and it can be replaced by memset(): > > diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h > index 43a3bf8ff428..d00431f553e1 100644 > --- a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h > +++ b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h > @@ -992,7 +992,7 @@ typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_1 { > *one and check the value returned for GPIOCount at runtime. > */ > #ifndef MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX > -#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (1) > +#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (36) > #endif > > typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_3 { > diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > index 44f9a05db94e..23fcf29bfd67 100644 > --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c > +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c > @@ -3203,7 +3203,7 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, > { > struct Scsi_Host *shost = class_to_shost(cdev); > struct MPT3SAS_ADAPTER *ioc = shost_priv(shost); > - Mpi2IOUnitPage3_t *io_unit_pg3 = NULL; > + Mpi2IOUnitPage3_t io_unit_pg3; > Mpi2ConfigReply_t mpi_reply; > u16 backup_rail_monitor_status = 0; > u16 ioc_status; > @@ -3221,16 +3221,10 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, > goto out; > > /* allocate upto GPIOVal 36 entries */ > - sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36); > - io_unit_pg3 = kzalloc(sz, GFP_KERNEL); > - if (!io_unit_pg3) { > - rc = -ENOMEM; > - ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n", > - __func__, sz); > - goto out; > - } > + sz = sizeof(io_unit_pg3); > + memset(&io_unit_pg3, 0, sz); I like this a lot. It makes the code way simpler. Putting this on the stack makes it faster, and it's less than 100 bytes, which seems entirely reasonable. > > - if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, io_unit_pg3, sz) != > + if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, &io_unit_pg3, sz) != The only thing I can imagine is if this ends up doing DMA, which isn't allowed on the stack. However, in looking down through the call path, it's _copied_ into DMA memory, so this appears entirely safe. Can you send this as a "normal" patch? Feel free to include: Reviewed-by: Kees Cook <keescook@chromium.org> Thanks! -Kees > 0) { > ioc_err(ioc, "%s: failed reading iounit_pg3\n", > __func__); > @@ -3246,19 +3240,18 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, > goto out; > } > > - if (io_unit_pg3->GPIOCount < 25) { > - ioc_err(ioc, "%s: iounit_pg3->GPIOCount less than 25 entries, detected (%d) entries\n", > - __func__, io_unit_pg3->GPIOCount); > + if (io_unit_pg3.GPIOCount < 25) { > + ioc_err(ioc, "%s: iounit_pg3.GPIOCount less than 25 entries, detected (%d) entries\n", > + __func__, io_unit_pg3.GPIOCount); > rc = -EINVAL; > goto out; > } > > /* BRM status is in bit zero of GPIOVal[24] */ > - backup_rail_monitor_status = le16_to_cpu(io_unit_pg3->GPIOVal[24]); > + backup_rail_monitor_status = le16_to_cpu(io_unit_pg3.GPIOVal[24]); > rc = snprintf(buf, PAGE_SIZE, "%d\n", (backup_rail_monitor_status & 1)); > > out: > - kfree(io_unit_pg3); > mutex_unlock(&ioc->pci_access_mutex); > return rc; > } > > > > > io_unit_pg3 = kzalloc(sizeof(*io_unit_pg3), GFP_KERNEL); > > > > Thanks > -- > Gustavo -- Kees Cook
On 3/10/21 13:07, Kees Cook wrote: >> diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h >> index 43a3bf8ff428..d00431f553e1 100644 >> --- a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h >> +++ b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h >> @@ -992,7 +992,7 @@ typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_1 { >> *one and check the value returned for GPIOCount at runtime. >> */ >> #ifndef MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX >> -#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (1) >> +#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (36) >> #endif >> >> typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_3 { >> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c >> index 44f9a05db94e..23fcf29bfd67 100644 >> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c >> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c >> @@ -3203,7 +3203,7 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, >> { >> struct Scsi_Host *shost = class_to_shost(cdev); >> struct MPT3SAS_ADAPTER *ioc = shost_priv(shost); >> - Mpi2IOUnitPage3_t *io_unit_pg3 = NULL; >> + Mpi2IOUnitPage3_t io_unit_pg3; >> Mpi2ConfigReply_t mpi_reply; >> u16 backup_rail_monitor_status = 0; >> u16 ioc_status; >> @@ -3221,16 +3221,10 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, >> goto out; >> >> /* allocate upto GPIOVal 36 entries */ >> - sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36); >> - io_unit_pg3 = kzalloc(sz, GFP_KERNEL); >> - if (!io_unit_pg3) { >> - rc = -ENOMEM; >> - ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n", >> - __func__, sz); >> - goto out; >> - } >> + sz = sizeof(io_unit_pg3); >> + memset(&io_unit_pg3, 0, sz); > > I like this a lot. It makes the code way simpler. > > Putting this on the stack makes it faster, and it's less than 100 bytes, > which seems entirely reasonable. > >> >> - if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, io_unit_pg3, sz) != >> + if (mpt3sas_config_get_iounit_pg3(ioc, &mpi_reply, &io_unit_pg3, sz) != > > The only thing I can imagine is if this ends up doing DMA, which isn't > allowed on the stack. However, in looking down through the call path, > it's _copied_ into DMA memory, so this appears entirely safe. > > Can you send this as a "normal" patch? Feel free to include: > > Reviewed-by: Kees Cook <keescook@chromium.org> Done: https://lore.kernel.org/lkml/20210310235951.GA108661@embeddedor/ Thanks for the comments! -- Gustavo
diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h index 43a3bf8ff428..908b0ca63204 100644 --- a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h +++ b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h @@ -987,21 +987,12 @@ typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_1 { /*IO Unit Page 3 */ -/* - *Host code (drivers, BIOS, utilities, etc.) should leave this define set to - *one and check the value returned for GPIOCount at runtime. - */ -#ifndef MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX -#define MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX (1) -#endif - typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_3 { MPI2_CONFIG_PAGE_HEADER Header; /*0x00 */ U8 GPIOCount; /*0x04 */ U8 Reserved1; /*0x05 */ U16 Reserved2; /*0x06 */ - U16 - GPIOVal[MPI2_IO_UNIT_PAGE_3_GPIO_VAL_MAX];/*0x08 */ + U16 GPIOVal[]; /*0x08 */ } MPI2_CONFIG_PAGE_IO_UNIT_3, *PTR_MPI2_CONFIG_PAGE_IO_UNIT_3, Mpi2IOUnitPage3_t, *pMpi2IOUnitPage3_t; diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c index c8a0ce18f2c5..ffb21f873058 100644 --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c @@ -3143,7 +3143,7 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, Mpi2ConfigReply_t mpi_reply; u16 backup_rail_monitor_status = 0; u16 ioc_status; - int sz; + size_t sz; ssize_t rc = 0; if (!ioc->is_warpdrive) { @@ -3157,11 +3157,11 @@ BRM_status_show(struct device *cdev, struct device_attribute *attr, goto out; /* allocate upto GPIOVal 36 entries */ - sz = offsetof(Mpi2IOUnitPage3_t, GPIOVal) + (sizeof(u16) * 36); + sz = struct_size(io_unit_pg3, GPIOVal, 36); io_unit_pg3 = kzalloc(sz, GFP_KERNEL); if (!io_unit_pg3) { rc = -ENOMEM; - ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%d) bytes\n", + ioc_err(ioc, "%s: failed allocating memory for iounit_pg3: (%zu) bytes\n", __func__, sz); goto out; }
There is a regular need in the kernel to provide a way to declare having a dynamically sized set of trailing elements in a structure. Kernel code should always use “flexible array members”[1] for these cases. The older style of one-element or zero-length arrays should no longer be used[2]. Refactor the code according to the use of a flexible-array member in struct _MPI2_CONFIG_PAGE_IO_UNIT_3, instead of a one-element array, and use the struct_size() helper to calculate the size for the allocation. Also, this helps the ongoing efforts to enable -Warray-bounds and fix the following warnings: drivers/scsi/mpt3sas/mpt3sas_ctl.c:3193:63: warning: array subscript 24 is above array bounds of ‘U16[1]’ {aka ‘short unsigned int[1]’} [-Warray-bounds] [1] https://en.wikipedia.org/wiki/Flexible_array_member [2] https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays Link: https://github.com/KSPP/linux/issues/79 Link: https://github.com/KSPP/linux/issues/109 Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- Changes in v2: - Fix format specifier: use %zu for size_t type. drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h | 11 +---------- drivers/scsi/mpt3sas/mpt3sas_ctl.c | 6 +++--- 2 files changed, 4 insertions(+), 13 deletions(-)