diff mbox series

[2/6] scsi: mpt3sas: Use flexible arrays when less obviously possible

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

Commit Message

James Seo July 25, 2023, 4:13 p.m. UTC 
These old-style 1-length variable arrays can be directly converted
into C99 flexible array members without any further source changes
and without any meaningful binary changes. All uses of the affected
structs were investigated, and the existing code somehow manages to
weather the reduced sizeof() the affected structs in every case.

For example, we may have previously requested 26 bytes from the
device to fill a 26-byte buffer for a struct, and we are now dealing
with 12 bytes due to sizeof() reduction. However, either we never use
the variable array anyway, or we follow up with a subsequent request
for the same struct in its entirety after using its variable element
count (usually taken from one of the struct fields) to allocate a
larger buffer.

It also turns out that size calculations are always performed as e.g.
"offsetof(struct foo, arr_of_bar) + n * sizeof(bar)" instead of
"sizeof(struct foo) + (n - 1) * sizeof(bar)", and are therefore
already correct.

Signed-off-by: James Seo <james@equiv.tech>
---
 drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h | 48 +++++++++-------------------
 1 file changed, 15 insertions(+), 33 deletions(-)

Comments

Kees Cook July 28, 2023, 10:26 p.m. UTC | #1 
On Tue, Jul 25, 2023 at 09:13:27AM -0700, James Seo wrote:
> These old-style 1-length variable arrays can be directly converted
> into C99 flexible array members without any further source changes
> and without any meaningful binary changes. All uses of the affected
> structs were investigated, and the existing code somehow manages to
> weather the reduced sizeof() the affected structs in every case.

Doing build comparisons here, I see a lot of binary changes. They may
be, as you say, harmless, and since you've actually got hardware then
this is a good verification of the changes, but I do wonder if this
needs more detailed commit log (or split up patches).

However, the problem I see is that this code was already doing weird
stuff with structs that appear to not have been using flex arrays
actually. With "pahole" I can see struct MPT3SAS_ADAPTER changes:

-       Mpi2IOUnitPage8_t          iounit_pg8;           /*  3668    40 */
-       Mpi2IOCPage1_t             ioc_pg1_copy;         /*  3708    24 */
+       Mpi2IOUnitPage8_t          iounit_pg8;           /*  3668    16 */
+       Mpi2IOCPage1_t             ioc_pg1_copy;         /*  3684    24 */

struct _MPI2_CONFIG_PAGE_IO_UNIT_8 (Mpi2IOUnitPage8_t) is in the
_middle_ of struct MPT3SAS_ADAPTER.... :|

In the earlier attempts at this conversion, it seemed that some of these
are actually fixed-size:

https://lore.kernel.org/lkml/20210202235118.GA314410@embeddedor/

I think this patch needs to be broken up into per-struct changes, so
they can be reviewed individually.

-Kees
James Seo July 29, 2023, 8:09 a.m. UTC | #2 
Hi, thanks for reviewing.

On Fri, Jul 28, 2023 at 03:26:57PM -0700, Kees Cook wrote:
> Doing build comparisons here, I see a lot of binary changes. They may
> be, as you say, harmless, and since you've actually got hardware then
> this is a good verification of the changes, but I do wonder if this
> needs more detailed commit log (or split up patches).
> 
> However, the problem I see is that this code was already doing weird
> stuff with structs that appear to not have been using flex arrays
> actually. With "pahole" I can see struct MPT3SAS_ADAPTER changes:
> 
> -       Mpi2IOUnitPage8_t          iounit_pg8;           /*  3668    40 */
> -       Mpi2IOCPage1_t             ioc_pg1_copy;         /*  3708    24 */
> +       Mpi2IOUnitPage8_t          iounit_pg8;           /*  3668    16 */
> +       Mpi2IOCPage1_t             ioc_pg1_copy;         /*  3684    24 */
> 
> struct _MPI2_CONFIG_PAGE_IO_UNIT_8 (Mpi2IOUnitPage8_t) is in the
> _middle_ of struct MPT3SAS_ADAPTER.... :|

In this particular case, the flex array member of iounit_pg8 is never
used, and iounit_pg8 itself is never used outside of the function
that fetches and sets it on the per-adapter struct MPT3SAS_ADAPTER.

iounit_pg8 could probably be removed, now that I think about it.
Maybe I will.

> In the earlier attempts at this conversion, it seemed that some of these
> are actually fixed-size:
> 
> https://lore.kernel.org/lkml/20210202235118.GA314410@embeddedor/

Yes, I tried to leave such terminal arrays alone. But I'll revisit
each change in this commit.

> I think this patch needs to be broken up into per-struct changes, so
> they can be reviewed individually.

Sure, I can do that. I'll resubmit this commit and the one following
(which depends on this commit) as a new series with more details.
Hopefully this will encourage the Broadcom folks who know this driver
best to chime in as well.

By the way, I noticed you've done something like this in the past to
preserve struct size for userspace, just in case:

	/* MPI2_IOUNIT8_SENSOR		Sensor[1]; */
	union {
		MPI2_IOUNIT8_SENSOR	_LegacyPadding;
		__DECLARE_FLEX_ARRAY(MPI2_IOUNIT8_SENSOR, Sensor);
	};

I don't think userspace is a concern for us here, but would you be
more comfortable if I did this too/instead?

James
diff mbox series

Patch

diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
index 42d820159c44..f07215fbc140 100644
--- a/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
+++ b/drivers/scsi/mpt3sas/mpi/mpi2_cnfg.h
@@ -1200,12 +1200,9 @@  typedef struct _MPI2_IOUNIT8_SENSOR {
 #define MPI2_IOUNIT8_SENSOR_FLAGS_T0_ENABLE         (0x0001)
 
 /*
- *Host code (drivers, BIOS, utilities, etc.) should leave this define set to
- *one and check the value returned for NumSensors at runtime.
+ *Host code (drivers, BIOS, utilities, etc.) should check the value returned
+ *for NumSensors at runtime before using Sensor[].
  */
-#ifndef MPI2_IOUNITPAGE8_SENSOR_ENTRIES
-#define MPI2_IOUNITPAGE8_SENSOR_ENTRIES     (1)
-#endif
 
 typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_8 {
 	MPI2_CONFIG_PAGE_HEADER Header;                 /*0x00 */
@@ -1214,8 +1211,7 @@  typedef struct _MPI2_CONFIG_PAGE_IO_UNIT_8 {
 	U8                      NumSensors;             /*0x0C */
 	U8                      PollingInterval;        /*0x0D */
 	U16                     Reserved3;              /*0x0E */
-	MPI2_IOUNIT8_SENSOR
-		Sensor[MPI2_IOUNITPAGE8_SENSOR_ENTRIES];/*0x10 */
+	MPI2_IOUNIT8_SENSOR     Sensor[];               /*0x10 */
 } MPI2_CONFIG_PAGE_IO_UNIT_8,
 	*PTR_MPI2_CONFIG_PAGE_IO_UNIT_8,
 	Mpi2IOUnitPage8_t, *pMpi2IOUnitPage8_t;
@@ -1805,12 +1801,9 @@  typedef struct _MPI2_RAIDVOL0_SETTINGS {
 #define MPI2_RAIDVOL0_SETTING_ENABLE_WRITE_CACHING      (0x0002)
 
 /*
- *Host code (drivers, BIOS, utilities, etc.) should leave this define set to
- *one and check the value returned for NumPhysDisks at runtime.
+ *Host code (drivers, BIOS, utilities, etc.) should check the value returned
+ *for NumPhysDisks at runtime before using PhysDisk[].
  */
-#ifndef MPI2_RAID_VOL_PAGE_0_PHYSDISK_MAX
-#define MPI2_RAID_VOL_PAGE_0_PHYSDISK_MAX       (1)
-#endif
 
 typedef struct _MPI2_CONFIG_PAGE_RAID_VOL_0 {
 	MPI2_CONFIG_PAGE_HEADER Header;            /*0x00 */
@@ -1830,8 +1823,7 @@  typedef struct _MPI2_CONFIG_PAGE_RAID_VOL_0 {
 	U8                      Reserved2;         /*0x25 */
 	U8                      Reserved3;         /*0x26 */
 	U8                      InactiveStatus;    /*0x27 */
-	MPI2_RAIDVOL0_PHYS_DISK
-	PhysDisk[MPI2_RAID_VOL_PAGE_0_PHYSDISK_MAX]; /*0x28 */
+	MPI2_RAIDVOL0_PHYS_DISK PhysDisk[];        /*0x28 */
 } MPI2_CONFIG_PAGE_RAID_VOL_0,
 	*PTR_MPI2_CONFIG_PAGE_RAID_VOL_0,
 	Mpi2RaidVolPage0_t, *pMpi2RaidVolPage0_t;
@@ -2186,12 +2178,9 @@  typedef struct _MPI2_SAS_IO_UNIT0_PHY_DATA {
 	*pMpi2SasIOUnit0PhyData_t;
 
 /*
- *Host code (drivers, BIOS, utilities, etc.) should leave this define set to
- *one and check the value returned for NumPhys at runtime.
+ *Host code (drivers, BIOS, utilities, etc.) should check the value returned
+ *for NumPhys at runtime before using PhyData[].
  */
-#ifndef MPI2_SAS_IOUNIT0_PHY_MAX
-#define MPI2_SAS_IOUNIT0_PHY_MAX        (1)
-#endif
 
 typedef struct _MPI2_CONFIG_PAGE_SASIOUNIT_0 {
 	MPI2_CONFIG_EXTENDED_PAGE_HEADER    Header;   /*0x00 */
@@ -2199,8 +2188,7 @@  typedef struct _MPI2_CONFIG_PAGE_SASIOUNIT_0 {
 	U8                                  NumPhys;  /*0x0C */
 	U8                                  Reserved2;/*0x0D */
 	U16                                 Reserved3;/*0x0E */
-	MPI2_SAS_IO_UNIT0_PHY_DATA
-		PhyData[MPI2_SAS_IOUNIT0_PHY_MAX];    /*0x10 */
+	MPI2_SAS_IO_UNIT0_PHY_DATA          PhyData[];/*0x10 */
 } MPI2_CONFIG_PAGE_SASIOUNIT_0,
 	*PTR_MPI2_CONFIG_PAGE_SASIOUNIT_0,
 	Mpi2SasIOUnitPage0_t, *pMpi2SasIOUnitPage0_t;
@@ -2261,12 +2249,9 @@  typedef struct _MPI2_SAS_IO_UNIT1_PHY_DATA {
 	*pMpi2SasIOUnit1PhyData_t;
 
 /*
- *Host code (drivers, BIOS, utilities, etc.) should leave this define set to
- *one and check the value returned for NumPhys at runtime.
+ *Host code (drivers, BIOS, utilities, etc.) should check the value returned
+ *for NumPhys at runtime before using PhyData[].
  */
-#ifndef MPI2_SAS_IOUNIT1_PHY_MAX
-#define MPI2_SAS_IOUNIT1_PHY_MAX        (1)
-#endif
 
 typedef struct _MPI2_CONFIG_PAGE_SASIOUNIT_1 {
 	MPI2_CONFIG_EXTENDED_PAGE_HEADER    Header; /*0x00 */
@@ -2287,7 +2272,7 @@  typedef struct _MPI2_CONFIG_PAGE_SASIOUNIT_1 {
 	U8
 		IODeviceMissingDelay;               /*0x13 */
 	MPI2_SAS_IO_UNIT1_PHY_DATA
-		PhyData[MPI2_SAS_IOUNIT1_PHY_MAX];  /*0x14 */
+		PhyData[];                          /*0x14 */
 } MPI2_CONFIG_PAGE_SASIOUNIT_1,
 	*PTR_MPI2_CONFIG_PAGE_SASIOUNIT_1,
 	Mpi2SasIOUnitPage1_t, *pMpi2SasIOUnitPage1_t;
@@ -3683,12 +3668,9 @@  typedef struct _MPI26_PCIE_IO_UNIT1_PHY_DATA {
 #define MPI26_PCIEIOUNIT1_LINKFLAGS_SRNS_EN                 (0x02)
 
 /*
- *Host code (drivers, BIOS, utilities, etc.) should leave this define set to
- *one and check the value returned for NumPhys at runtime.
+ *Host code (drivers, BIOS, utilities, etc.) should check the value returned
+ *for NumPhys at runtime before using PhyData[].
  */
-#ifndef MPI26_PCIE_IOUNIT1_PHY_MAX
-#define MPI26_PCIE_IOUNIT1_PHY_MAX      (1)
-#endif
 
 typedef struct _MPI26_CONFIG_PAGE_PIOUNIT_1 {
 	MPI2_CONFIG_EXTENDED_PAGE_HEADER	Header;	/*0x00 */
@@ -3700,7 +3682,7 @@  typedef struct _MPI26_CONFIG_PAGE_PIOUNIT_1 {
 	U8	DMDReportPCIe;                      /*0x11 */
 	U16	Reserved2;                          /*0x12 */
 	MPI26_PCIE_IO_UNIT1_PHY_DATA
-		PhyData[MPI26_PCIE_IOUNIT1_PHY_MAX];/*0x14 */
+		PhyData[];                          /*0x14 */
 } MPI26_CONFIG_PAGE_PIOUNIT_1,
 	*PTR_MPI26_CONFIG_PAGE_PIOUNIT_1,
 	Mpi26PCIeIOUnitPage1_t, *pMpi26PCIeIOUnitPage1_t;