mbox series

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

Message ID 20230806170604.16143-1-james@equiv.tech
Headers show
Series scsi: mpt3sas: Use flexible arrays and do a few cleanups | expand

Message

James Seo Aug. 6, 2023, 5:05 p.m. UTC 
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

Comments

Martin K. Petersen Aug. 25, 2023, 3 a.m. UTC | #1 
> 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!
Kees Cook Oct. 11, 2023, 12:49 a.m. UTC | #2 
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 Oct. 23, 2023, 4:30 p.m. UTC | #3 
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
>
James Seo Oct. 28, 2023, 7:32 p.m. UTC | #4 
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
Martin K. Petersen Nov. 15, 2023, 1:54 p.m. UTC | #5 
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!
Kees Cook Nov. 15, 2023, 2:38 p.m. UTC | #6 
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. :)
Martin K. Petersen Nov. 25, 2023, 2:54 a.m. UTC | #7 
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