[PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups

James Seo posted 12 patches 2 years, 1 month ago
drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h         | 231 ++++++-------------
drivers/scsi/mpt3sas/mpi/mpi2_image.h        |  32 +--
drivers/scsi/mpt3sas/mpi/mpi2_ioc.h          |  27 +--
drivers/scsi/mpt3sas/mpt3sas_base.c          |  35 ++-
drivers/scsi/mpt3sas/mpt3sas_base.h          |   2 -
drivers/scsi/mpt3sas/mpt3sas_config.c        |   6 +-
drivers/scsi/mpt3sas/mpt3sas_scsih.c         |  55 ++---
drivers/scsi/mpt3sas/mpt3sas_transport.c     |   9 +-
drivers/scsi/mpt3sas/mpt3sas_trigger_pages.h |  44 ++--
drivers/scsi/mpt3sas/mpt3sas_warpdrive.c     |   3 +-
10 files changed, 151 insertions(+), 293 deletions(-)
[PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups
Posted by James Seo 2 years, 1 month ago
Commit df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") has
resulted in the only arrays that UBSAN_BOUNDS considers unbounded
being trailing arrays declared with [] as the last member of a
struct. Unbounded trailing arrays declared with [1] are common in
mpt3sas, which is causing spurious warnings to appear in some
situations, e.g. when more than one physical disk is connected:

  UBSAN: array-index-out-of-bounds in drivers/scsi/mpt3sas/mpt3sas_scsih.c:6810:36
  index 1 is out of range for type 'MPI2_SAS_IO_UNIT0_PHY_DATA [1]'

which relates to this unbounded array access:

  port_id = sas_iounit_pg0->PhyData[i].Port;

and is just one example of 10 similar warnings currently occurring
for me during boot.

This series converts most trailing arrays declared with [1] in mptsas
into proper C99 flexible array members. Those that are not unbounded
and really are fixed-length arrays of length 1 are left alone.

I didn't find any conversions that required further source edits
besides changing [1] to [], and everything seems to work with my
SAS2008-based add-in card, but please look things over in case I
missed something subtle.

Rounding out the series are some opportunistic cleanups.

The only dependency is that patch 7 ("Use struct_size() for struct
size calculations") depends on patches 3-5.

History:
v1: https://lore.kernel.org/linux-scsi/20230725161331.27481-1-james@equiv.tech/

Changes v1->v2:
- Slightly reword and add Reviewed-by: tags to commit messages
- Split up a commit that was resulting in many binary changes
- Remove the iounit_pg8 member of the per-adapter struct
- Replace more dynamic allocations with local variables

James Seo (12):
  scsi: mpt3sas: Use flexible arrays when obviously possible
  scsi: mpt3sas: Make MPI2_CONFIG_PAGE_IO_UNIT_8::Sensor[] a flexible
    array
  scsi: mpt3sas: Make MPI2_CONFIG_PAGE_RAID_VOL_0::PhysDisk[] a flexible
    array
  scsi: mpt3sas: Make MPI2_CONFIG_PAGE_SASIOUNIT_0::PhyData[] a flexible
    array
  scsi: mpt3sas: Make MPI2_CONFIG_PAGE_SASIOUNIT_1::PhyData[] a flexible
    array
  scsi: mpt3sas: Make MPI26_CONFIG_PAGE_PIOUNIT_1::PhyData[] a flexible
    array
  scsi: mpt3sas: Use struct_size() for struct size calculations
  scsi: mpt3sas: Remove the iounit_pg8 member of the per-adapter struct
  scsi: mpt3sas: Fix an outdated comment
  scsi: mpt3sas: Fix typo of "TRIGGER"
  scsi: mpt3sas: Replace a dynamic allocation with a local variable
  scsi: mpt3sas: Replace dynamic allocations with local variables

 drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h         | 231 ++++++-------------
 drivers/scsi/mpt3sas/mpi/mpi2_image.h        |  32 +--
 drivers/scsi/mpt3sas/mpi/mpi2_ioc.h          |  27 +--
 drivers/scsi/mpt3sas/mpt3sas_base.c          |  35 ++-
 drivers/scsi/mpt3sas/mpt3sas_base.h          |   2 -
 drivers/scsi/mpt3sas/mpt3sas_config.c        |   6 +-
 drivers/scsi/mpt3sas/mpt3sas_scsih.c         |  55 ++---
 drivers/scsi/mpt3sas/mpt3sas_transport.c     |   9 +-
 drivers/scsi/mpt3sas/mpt3sas_trigger_pages.h |  44 ++--
 drivers/scsi/mpt3sas/mpt3sas_warpdrive.c     |   3 +-
 10 files changed, 151 insertions(+), 293 deletions(-)


base-commit: 6cae9a3910ac1b5daf5ac3db9576b78cc4eff5aa
-- 
2.39.2
Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups
Posted by Kees Cook 1 year, 10 months ago
On Sun, Aug 06, 2023 at 10:05:52AM -0700, James Seo wrote:
> Commit df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") has
> resulted in the only arrays that UBSAN_BOUNDS considers unbounded
> being trailing arrays declared with [] as the last member of a
> struct. Unbounded trailing arrays declared with [1] are common in
> mpt3sas, which is causing spurious warnings to appear in some
> situations, e.g. when more than one physical disk is connected:
> 
>   UBSAN: array-index-out-of-bounds in drivers/scsi/mpt3sas/mpt3sas_scsih.c:6810:36
>   index 1 is out of range for type 'MPI2_SAS_IO_UNIT0_PHY_DATA [1]'
> 
> which relates to this unbounded array access:
> 
>   port_id = sas_iounit_pg0->PhyData[i].Port;
> 
> and is just one example of 10 similar warnings currently occurring
> for me during boot.
> 
> This series converts most trailing arrays declared with [1] in mptsas
> into proper C99 flexible array members. Those that are not unbounded
> and really are fixed-length arrays of length 1 are left alone.
> 
> I didn't find any conversions that required further source edits
> besides changing [1] to [], and everything seems to work with my
> SAS2008-based add-in card, but please look things over in case I
> missed something subtle.
> 
> Rounding out the series are some opportunistic cleanups.
> 
> The only dependency is that patch 7 ("Use struct_size() for struct
> size calculations") depends on patches 3-5.
> 
> History:
> v1: https://lore.kernel.org/linux-scsi/20230725161331.27481-1-james@equiv.tech/
> 
> Changes v1->v2:
> - Slightly reword and add Reviewed-by: tags to commit messages
> - Split up a commit that was resulting in many binary changes
> - Remove the iounit_pg8 member of the per-adapter struct
> - Replace more dynamic allocations with local variables

Here's a tested-by: from Boris:

https://lore.kernel.org/all/20231023135615.GBZTZ7fwRh48euq3ew@fat_crate.local

-Kees

> 
> James Seo (12):
>   scsi: mpt3sas: Use flexible arrays when obviously possible
>   scsi: mpt3sas: Make MPI2_CONFIG_PAGE_IO_UNIT_8::Sensor[] a flexible
>     array
>   scsi: mpt3sas: Make MPI2_CONFIG_PAGE_RAID_VOL_0::PhysDisk[] a flexible
>     array
>   scsi: mpt3sas: Make MPI2_CONFIG_PAGE_SASIOUNIT_0::PhyData[] a flexible
>     array
>   scsi: mpt3sas: Make MPI2_CONFIG_PAGE_SASIOUNIT_1::PhyData[] a flexible
>     array
>   scsi: mpt3sas: Make MPI26_CONFIG_PAGE_PIOUNIT_1::PhyData[] a flexible
>     array
>   scsi: mpt3sas: Use struct_size() for struct size calculations
>   scsi: mpt3sas: Remove the iounit_pg8 member of the per-adapter struct
>   scsi: mpt3sas: Fix an outdated comment
>   scsi: mpt3sas: Fix typo of "TRIGGER"
>   scsi: mpt3sas: Replace a dynamic allocation with a local variable
>   scsi: mpt3sas: Replace dynamic allocations with local variables
> 
>  drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h         | 231 ++++++-------------
>  drivers/scsi/mpt3sas/mpi/mpi2_image.h        |  32 +--
>  drivers/scsi/mpt3sas/mpi/mpi2_ioc.h          |  27 +--
>  drivers/scsi/mpt3sas/mpt3sas_base.c          |  35 ++-
>  drivers/scsi/mpt3sas/mpt3sas_base.h          |   2 -
>  drivers/scsi/mpt3sas/mpt3sas_config.c        |   6 +-
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c         |  55 ++---
>  drivers/scsi/mpt3sas/mpt3sas_transport.c     |   9 +-
>  drivers/scsi/mpt3sas/mpt3sas_trigger_pages.h |  44 ++--
>  drivers/scsi/mpt3sas/mpt3sas_warpdrive.c     |   3 +-
>  10 files changed, 151 insertions(+), 293 deletions(-)
> 
> 
> base-commit: 6cae9a3910ac1b5daf5ac3db9576b78cc4eff5aa
> -- 
> 2.39.2
> 

-- 
Kees Cook
Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups
Posted by Martin K. Petersen 1 year, 10 months ago
Kees,

> Here's a tested-by: from Boris:
>
> https://lore.kernel.org/all/20231023135615.GBZTZ7fwRh48euq3ew@fat_crate.local

I'm a bit concerned bringing this in just before the merge window.
Please ping me if I forget to merge once -rc1 is out.

-- 
Martin K. Petersen	Oracle Linux Engineering
Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups
Posted by Kees Cook 1 year, 10 months ago
On Tue, Oct 24, 2023 at 10:05:33PM -0400, Martin K. Petersen wrote:
> 
> Kees,
> 
> > Here's a tested-by: from Boris:
> >
> > https://lore.kernel.org/all/20231023135615.GBZTZ7fwRh48euq3ew@fat_crate.local
> 
> I'm a bit concerned bringing this in just before the merge window.
> Please ping me if I forget to merge once -rc1 is out.

Sounds good; thanks!

-- 
Kees Cook
Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups
Posted by Martin K. Petersen 1 year, 10 months ago
Kees,

>> I'm a bit concerned bringing this in just before the merge window.
>> Please ping me if I forget to merge once -rc1 is out.

Applied to 6.8/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering
Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups
Posted by Kees Cook 1 year, 10 months ago
On Wed, Nov 15, 2023 at 08:54:22AM -0500, Martin K. Petersen wrote:
> >> I'm a bit concerned bringing this in just before the merge window.
> >> Please ping me if I forget to merge once -rc1 is out.
> 
> Applied to 6.8/scsi-staging, thanks!

Great! Thanks for picking this up. :)

-- 
Kees Cook
Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups
Posted by Martin K. Petersen 2 years ago
> Commit df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") has
> resulted in the only arrays that UBSAN_BOUNDS considers unbounded
> being trailing arrays declared with [] as the last member of a struct.
> Unbounded trailing arrays declared with [1] are common in mpt3sas,
> which is causing spurious warnings to appear in some situations, e.g.
> when more than one physical disk is connected:

Broadcom: Please review/test. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering
Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups
Posted by Kees Cook 1 year, 11 months ago
On Thu, Aug 24, 2023 at 11:00:57PM -0400, Martin K. Petersen wrote:
> 
> > Commit df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") has
> > resulted in the only arrays that UBSAN_BOUNDS considers unbounded
> > being trailing arrays declared with [] as the last member of a struct.
> > Unbounded trailing arrays declared with [1] are common in mpt3sas,
> > which is causing spurious warnings to appear in some situations, e.g.
> > when more than one physical disk is connected:
> 
> Broadcom: Please review/test. Thanks!

Another thread ping. Is anyone at broadcom around? I'd really like to
see this series (or some form of it) land to avoid all these runtime
warnings...

-- 
Kees Cook
Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups
Posted by James Seo 1 year, 10 months ago
On Tue, Oct 10, 2023 at 05:49:38PM -0700, Kees Cook wrote:
> On Thu, Aug 24, 2023 at 11:00:57PM -0400, Martin K. Petersen wrote:
>> 
>>> Commit df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") has
>>> resulted in the only arrays that UBSAN_BOUNDS considers unbounded
>>> being trailing arrays declared with [] as the last member of a struct.
>>> Unbounded trailing arrays declared with [1] are common in mpt3sas,
>>> which is causing spurious warnings to appear in some situations, e.g.
>>> when more than one physical disk is connected:
>> 
>> Broadcom: Please review/test. Thanks!
> 
> Another thread ping. Is anyone at broadcom around? I'd really like to
> see this series (or some form of it) land to avoid all these runtime
> warnings...
> 
> -- 
> Kees Cook

Looks like this series was accepted for -rc1. Thanks!

One last thread ping for the Broadcom folks, just in case.

-James Seo
Re: [PATCH v2 00/12] scsi: mpt3sas: Use flexible arrays and do a few cleanups
Posted by Martin K. Petersen 1 year, 9 months ago
On Sun, 06 Aug 2023 10:05:52 -0700, James Seo wrote:

> Commit df8fc4e934c1 ("kbuild: Enable -fstrict-flex-arrays=3") has
> resulted in the only arrays that UBSAN_BOUNDS considers unbounded
> being trailing arrays declared with [] as the last member of a
> struct. Unbounded trailing arrays declared with [1] are common in
> mpt3sas, which is causing spurious warnings to appear in some
> situations, e.g. when more than one physical disk is connected:
> 
> [...]

Applied to 6.8/scsi-queue, thanks!

[01/12] scsi: mpt3sas: Use flexible arrays when obviously possible
        https://git.kernel.org/mkp/scsi/c/aa4db51bbd51
[02/12] scsi: mpt3sas: Make MPI2_CONFIG_PAGE_IO_UNIT_8::Sensor[] a flexible array
        https://git.kernel.org/mkp/scsi/c/f7830af68eb6
[03/12] scsi: mpt3sas: Make MPI2_CONFIG_PAGE_RAID_VOL_0::PhysDisk[] a flexible array
        https://git.kernel.org/mkp/scsi/c/cb7c03c5d357
[04/12] scsi: mpt3sas: Make MPI2_CONFIG_PAGE_SASIOUNIT_0::PhyData[] a flexible array
        https://git.kernel.org/mkp/scsi/c/dccc1e3ed9e3
[05/12] scsi: mpt3sas: Make MPI2_CONFIG_PAGE_SASIOUNIT_1::PhyData[] a flexible array
        https://git.kernel.org/mkp/scsi/c/e249a957ce43
[06/12] scsi: mpt3sas: Make MPI26_CONFIG_PAGE_PIOUNIT_1::PhyData[] a flexible array
        https://git.kernel.org/mkp/scsi/c/1f1126609969
[07/12] scsi: mpt3sas: Use struct_size() for struct size calculations
        https://git.kernel.org/mkp/scsi/c/f4f76e141769
[08/12] scsi: mpt3sas: Remove the iounit_pg8 member of the per-adapter struct
        https://git.kernel.org/mkp/scsi/c/66f2a53fc620
[09/12] scsi: mpt3sas: Fix an outdated comment
        https://git.kernel.org/mkp/scsi/c/8a3db51e01d5
[10/12] scsi: mpt3sas: Fix typo of "TRIGGER"
        https://git.kernel.org/mkp/scsi/c/e5035459d302
[11/12] scsi: mpt3sas: Replace a dynamic allocation with a local variable
        https://git.kernel.org/mkp/scsi/c/dde41e0c1cc2
[12/12] scsi: mpt3sas: Replace dynamic allocations with local variables
        https://git.kernel.org/mkp/scsi/c/e18821556272

-- 
Martin K. Petersen	Oracle Linux Engineering