Replace one-element arrays with flexible-array members

Hi!

This series aims to replace one-element arrays with flexible-array members in drivers/scsi/megaraid/

I followed the below steps in order to verify the changes don't significally impact the code (.text) section generated by the compiler, for each object file involved:

1. Prepare the build with the following settings and configurations:

        linux$ KBF="KBUILD_BUILD_TIMESTAMP=1970-01-01 KBUILD_BUILD_USER=user
               KBUILD_BUILD_HOST=host KBUILD_BUILD_VERSION=1"
        linux$ make $KBF allyesconfig
        linux$ ./scripts/config -d GCOV_KERNEL -d KCOV -d GCC_PLUGINS \
                         -d IKHEADERS -d KASAN -d UBSAN \
                         -d DEBUG_INFO_NONE \
                         -e DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT
        linux$ make $KBF olddefconfig

2. Build drivers/scsi/megaraid/ with the same settings and configurations as in Step 1, and copy the generated object files in directory before/

linux$ make -j128 $KBF drivers/scsi/megaraid/ linux$ mkdir -p before
linux$ cp drivers/scsi/megaraid/*.o before/

3. Implement all the needed changes and create the patch series. In this case, six patches.

linux$ vi code.c
...do the magic :)
linux$ git format-patch ...all the rest

4. Apply a patch at a time (of the previously created series) and, after applying EACH patch, build (as in Step 2) drivers/scsi/megaraid/ and copy the generated object files in directory after/

5. Compare the code section (.text) of each before/file.o and after/file.o. I use the following bash script:

   compare.sh:
        ARGS="--disassemble --demangle --reloc --no-show-raw-insn --section=.text"
        for i in $(cd before && echo *.o); do
                echo $i
                diff -u <(objdump $ARGS before/$i | sed "0,/^Disassembly/d") \
                        <(objdump $ARGS after/$i  | sed "0,/^Disassembly/d")
        done

linux$ ./compare.sh > code_comparison.diff

6. Open the code_comparison.diff file from the example above, look for any differences that might show up and analyze them in order to determine their impact, and what (if something) should be changed or study further.

The above process (code section comparison of object files) is based on this[0] blog post by Kees Cook. The compiler used to build the code was GCC-12.

In this series I only found the following sorts of differences in files megaraid_sas.o and megaraid_sas_base.o:

...
...@@ -7094,24 +7094,24 @@

     6302:      movq   $0x0,0x1e20(%rbx)
     630d:      test   %r15,%r15
     6310:      je     6316 <megasas_aen_polling+0x56>
-                       6312: R_X86_64_PC32     .text.unlikely+0x3ae3
+                       6312: R_X86_64_PC32     .text.unlikely+0x3ae0
     6316:      mov    0x0(%rip),%eax        # 631c <megasas_aen_polling+0x5c>
                        6318: R_X86_64_PC32     event_log_level-0x4
     631c:      mov    0xc(%r15),%r14d
     6320:      lea    0x2(%rax),%edx
     6323:      cmp    $0x6,%edx
     6326:      ja     632c <megasas_aen_polling+0x6c>
-                       6328: R_X86_64_PC32     .text.unlikely+0x3ac3
+                       6328: R_X86_64_PC32     .text.unlikely+0x3ac0
     632c:      mov    %r14d,%edx
     632f:      sar    $0x18,%edx
     6332:      mov    %edx,%ecx
     6334:      cmp    %eax,%edx
     6336:      jge    633c <megasas_aen_polling+0x7c>
-                       6338: R_X86_64_PC32     .text.unlikely+0x399c
+                       6338: R_X86_64_PC32     .text.unlikely+0x3999

...

All of them have to do with the relocation of symbols in the .text.unlikely subsection and they don't seem to be of any actual relevance. So, we can safely ignore them.

Also, notice there is an open issue in bugzilla.kernel.org [1] that's seems could be fixed by this series. :)

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines on memcpy() and help us make progress towards globally enabling -fstrict-flex-arrays [2].

Thanks

[0] https://outflux.net/blog/archives/2022/06/24/finding-binary-differences/

Changes in v3:
- Split the struct_size() changes into a couple of separate patches.
- Use objdump to compare the code (.text) sections of the object files before and after the changes.
- Modify MR_FW_RAID_MAP_ALL and MR_DRV_RAID_MAP_ALL structures. Change suggested by Kees Cook.

Changes in v2:
- Revert changes in struct MR_FW_RAID_MAP_ALL.

Gustavo A. R. Silva (6):
scsi: megaraid_sas: Replace one-element array with flexible-array
member in MR_FW_RAID_MAP
scsi: megaraid_sas: Replace one-element array with flexible-array
member in MR_FW_RAID_MAP_DYNAMIC
scsi: megaraid_sas: Replace one-element array with flexible-array
member in MR_DRV_RAID_MAP
scsi: megaraid_sas: Replace one-element array with flexible-array
member in MR_PD_CFG_SEQ_NUM_SYNC
scsi: megaraid_sas: Use struct_size() in code related to struct
MR_FW_RAID_MAP
scsi: megaraid_sas: Use struct_size() in code related to struct
MR_PD_CFG_SEQ_NUM_SYNC

 drivers/scsi/megaraid/megaraid_sas_base.c   | 20 ++++++++++----------
 drivers/scsi/megaraid/megaraid_sas_fp.c     |  6 +++---
 drivers/scsi/megaraid/megaraid_sas_fusion.c |  2 +-
 drivers/scsi/megaraid/megaraid_sas_fusion.h | 12 ++++++------
 4 files changed, 20 insertions(+), 20 deletions(-)

2.34.1

One-element arrays are deprecated, and we are replacing them with flexible array members, instead. So, replace one-element array with flexible-array member in struct MR_DRV_RAID_MAP and refactor the the rest of the code accordingly.

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines on memcpy().

---
Changes in v3:
- Modify MR_FW_RAID_MAP_ALL structures.
- Revert use of struct_size() helper.

Changes in v2:
- Update Subject line and changelog text.

 drivers/scsi/megaraid/megaraid_sas_base.c   | 2 +-
 drivers/scsi/megaraid/megaraid_sas_fp.c     | 2 +-
 drivers/scsi/megaraid/megaraid_sas_fusion.h | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

index a3e117a4b8e7..c06a346a3eaf 100644
@@ -5159,7 +5159,7 @@ static void megasas_update_ext_vd_details(struct megasas_instance *instance)
 	} else {
 		fusion->old_map_sz =  sizeof(struct MR_FW_RAID_MAP) +
 					(sizeof(struct MR_LD_SPAN_MAP) *
-					(instance->fw_supported_vd_count - 1));
+					instance->fw_supported_vd_count);
 		fusion->new_map_sz =  sizeof(struct MR_FW_RAID_MAP_EXT);
 
 		fusion->max_map_sz =

index 83f69c33b01a..b2ec7a3f7650 100644
@@ -327,7 +327,7 @@ u8 MR_ValidateMapInfo(struct megasas_instance *instance, u64 map_id)
 		expected_size = sizeof(struct MR_FW_RAID_MAP_EXT);
 	else
 		expected_size =
-			(sizeof(struct MR_FW_RAID_MAP) - sizeof(struct MR_LD_SPAN_MAP) +
+			(sizeof(struct MR_FW_RAID_MAP) +
 			(sizeof(struct MR_LD_SPAN_MAP) * le16_to_cpu(pDrvRaidMap->ldCount)));
 
 	if (le32_to_cpu(pDrvRaidMap->totalSize) != expected_size) {

index ce84f811e5e1..5530c233fccb 100644
@@ -942,7 +942,7 @@ struct MR_FW_RAID_MAP {
 	u8                  reserved2[7];
 	struct MR_ARRAY_INFO       arMapInfo[MAX_RAIDMAP_ARRAYS];
 	struct MR_DEV_HANDLE_INFO  devHndlInfo[MAX_RAIDMAP_PHYSICAL_DEVICES];
-	struct MR_LD_SPAN_MAP      ldSpanMap[1];
+	struct MR_LD_SPAN_MAP      ldSpanMap[];
 };
 
 struct IO_REQUEST_INFO {

@@ -1148,7 +1148,7 @@ typedef struct LOG_BLOCK_SPAN_INFO {
 
 struct MR_FW_RAID_MAP_ALL {
 	struct MR_FW_RAID_MAP raidMap;
-	struct MR_LD_SPAN_MAP ldSpanMap[MAX_LOGICAL_DRIVES - 1];
+	struct MR_LD_SPAN_MAP ldSpanMap[MAX_LOGICAL_DRIVES];
 } __attribute__ ((packed));
 
 struct MR_DRV_RAID_MAP {

2.34.1

One-element arrays are deprecated, and we are replacing them with flexible array members, instead. So, replace one-element array with flexible-array member in struct MR_DRV_RAID_MAP and refactor the code accordingly.

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines on memcpy().

---
Changes in v3:
- Revert use of flex_array_size() helper.
- Modify MR_DRV_RAID_MAP_ALL structure.

Changes in v2:
- None.

 drivers/scsi/megaraid/megaraid_sas_fusion.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 66e13b74fcdc..df92d4369e04 100644
@@ -1182,7 +1182,7 @@ struct MR_DRV_RAID_MAP {
 		devHndlInfo[MAX_RAIDMAP_PHYSICAL_DEVICES_DYN];
 	u16 ldTgtIdToLd[MAX_LOGICAL_DRIVES_DYN];
 	struct MR_ARRAY_INFO arMapInfo[MAX_API_ARRAYS_DYN];
-	struct MR_LD_SPAN_MAP      ldSpanMap[1];
+	struct MR_LD_SPAN_MAP      ldSpanMap[];
 
 };
 

@@ -1193,7 +1193,7 @@ struct MR_DRV_RAID_MAP {
 struct MR_DRV_RAID_MAP_ALL {
 
 	struct MR_DRV_RAID_MAP raidMap;
-	struct MR_LD_SPAN_MAP ldSpanMap[MAX_LOGICAL_DRIVES_DYN - 1];
+	struct MR_LD_SPAN_MAP ldSpanMap[MAX_LOGICAL_DRIVES_DYN];
 } __packed;
 
 

2.34.1

One-element arrays are deprecated, and we are replacing them with flexible array members, instead. So, replace one-element array with flexible-array member in struct MR_PD_CFG_SEQ_NUM_SYNC and refactor the rest of the code accordingly.

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines on memcpy() and help us make progress towards globally enabling -fstrict-flex-arrays [0].

---
Changes in v3:
- Revert use of struct_size() helper.

Changes in v2:
- None.

 drivers/scsi/megaraid/megaraid_sas_base.c   | 4 ++--
 drivers/scsi/megaraid/megaraid_sas_fusion.c | 2 +-
 drivers/scsi/megaraid/megaraid_sas_fusion.h | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

index c06a346a3eaf..2dabe0b4823e 100644
@@ -5795,7 +5795,7 @@ megasas_setup_jbod_map(struct megasas_instance *instance)
 	u32 pd_seq_map_sz;
 
 	pd_seq_map_sz = sizeof(struct MR_PD_CFG_SEQ_NUM_SYNC) +
-		(sizeof(struct MR_PD_CFG_SEQ) * (MAX_PHYSICAL_DEVICES - 1));
+		(sizeof(struct MR_PD_CFG_SEQ) * MAX_PHYSICAL_DEVICES);
 
 	instance->use_seqnum_jbod_fp =
 		instance->support_seqnum_jbod_fp;

@@ -8048,7 +8048,7 @@ static void megasas_detach_one(struct pci_dev *pdev)
 		megasas_release_fusion(instance);
 		pd_seq_map_sz = sizeof(struct MR_PD_CFG_SEQ_NUM_SYNC) +
 				(sizeof(struct MR_PD_CFG_SEQ) *
-					(MAX_PHYSICAL_DEVICES - 1));
+					MAX_PHYSICAL_DEVICES);
 		for (i = 0; i < 2 ; i++) {
 			if (fusion->ld_map[i])
 				dma_free_coherent(&instance->pdev->dev,

index e48d4261d0bc..f17ec83f7c98 100644
@@ -1310,7 +1310,7 @@ megasas_sync_pd_seq_num(struct megasas_instance *instance, bool pend) {
 
 	pd_sync = (void *)fusion->pd_seq_sync[(instance->pd_seq_map_id & 1)];
 	pd_seq_h = fusion->pd_seq_phys[(instance->pd_seq_map_id & 1)];
-	pd_seq_map_sz = struct_size(pd_sync, seq, MAX_PHYSICAL_DEVICES - 1);
+	pd_seq_map_sz = struct_size(pd_sync, seq, MAX_PHYSICAL_DEVICES);
 
 	cmd = megasas_get_cmd(instance);
 	if (!cmd) {

index df92d4369e04..49e9a9048ee7 100644
@@ -1249,7 +1249,7 @@ struct MR_PD_CFG_SEQ {
 struct MR_PD_CFG_SEQ_NUM_SYNC {
 	__le32 size;
 	__le32 count;
-	struct MR_PD_CFG_SEQ seq[1];
+	struct MR_PD_CFG_SEQ seq[];
 } __packed;
 
 /* stream detection */

2.34.1

Prefer struct_size() over open-coded versions of idiom:

sizeof(struct-with-flex-array) + sizeof(type-of-flex-array) * count

where count is the max number of items the flexible array is supposed to have.

---
Changes in v3:
- This patch is new in the series.

 drivers/scsi/megaraid/megaraid_sas_base.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index 09eef1bef430..d57cb787db0b 100644
@@ -5792,10 +5792,10 @@ megasas_setup_jbod_map(struct megasas_instance *instance)
 {
 	int i;
 	struct fusion_context *fusion = instance->ctrl_context;
-	u32 pd_seq_map_sz;
+	size_t pd_seq_map_sz;
 
-	pd_seq_map_sz = sizeof(struct MR_PD_CFG_SEQ_NUM_SYNC) +
-		(sizeof(struct MR_PD_CFG_SEQ) * MAX_PHYSICAL_DEVICES);
+	pd_seq_map_sz = struct_size((struct MR_PD_CFG_SEQ_NUM_SYNC *)0, seq,
+				    MAX_PHYSICAL_DEVICES);
 
 	instance->use_seqnum_jbod_fp =
 		instance->support_seqnum_jbod_fp;

@@ -7974,7 +7974,7 @@ static void megasas_detach_one(struct pci_dev *pdev)
 	struct Scsi_Host *host;
 	struct megasas_instance *instance;
 	struct fusion_context *fusion;
-	u32 pd_seq_map_sz;
+	size_t pd_seq_map_sz;
 
 	instance = pci_get_drvdata(pdev);
 

@@ -8046,9 +8046,9 @@ static void megasas_detach_one(struct pci_dev *pdev)
 
 	if (instance->adapter_type != MFI_SERIES) {
 		megasas_release_fusion(instance);
-		pd_seq_map_sz = sizeof(struct MR_PD_CFG_SEQ_NUM_SYNC) +
-				(sizeof(struct MR_PD_CFG_SEQ) *
-					MAX_PHYSICAL_DEVICES);
+		pd_seq_map_sz =
+			struct_size((struct MR_PD_CFG_SEQ_NUM_SYNC *)0,
+				    seq, MAX_PHYSICAL_DEVICES);
 		for (i = 0; i < 2 ; i++) {
 			if (fusion->ld_map[i])
 				dma_free_coherent(&instance->pdev->dev,

2.34.1

One-element arrays are deprecated, and we are replacing them with flexible array members, instead. So, replace one-element array with flexible-array member in struct MR_FW_RAID_MAP_DYNAMIC.

This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines on memcpy().

---
Changes in v3:
- None.

Changes in v2:
- None.

 drivers/scsi/megaraid/megaraid_sas_fusion.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 5530c233fccb..66e13b74fcdc 100644
@@ -1053,7 +1053,7 @@ struct MR_FW_RAID_MAP_DYNAMIC {
 	struct MR_RAID_MAP_DESC_TABLE
 			raid_map_desc_table[RAID_MAP_DESC_TYPE_COUNT];
 	/* Variable Size buffer containing all data */
-	u32 raid_map_desc_data[1];
+	u32 raid_map_desc_data[];
 }; /* Dynamicaly sized RAID MAp structure */
 
 #define IEEE_SGE_FLAGS_ADDR_MASK            (0x03)

2.34.1

Prefer struct_size() over open-coded versions of idiom:

sizeof(struct-with-flex-array) + sizeof(type-of-flex-array) * count

where count is the max number of items the flexible array is supposed to have.

---
Changes in v3:
- This patch is new in the series.

 drivers/scsi/megaraid/megaraid_sas_base.c | 6 +++---
 drivers/scsi/megaraid/megaraid_sas_fp.c   | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

index 2dabe0b4823e..09eef1bef430 100644
@@ -5157,9 +5157,9 @@ static void megasas_update_ext_vd_details(struct megasas_instance *instance)
 		fusion->current_map_sz = ventura_map_sz;
 		fusion->max_map_sz = ventura_map_sz;
 	} else {
-		fusion->old_map_sz =  sizeof(struct MR_FW_RAID_MAP) +
-					(sizeof(struct MR_LD_SPAN_MAP) *
-					instance->fw_supported_vd_count);
+		fusion->old_map_sz =
+			struct_size((struct MR_FW_RAID_MAP *)0, ldSpanMap,
+				    instance->fw_supported_vd_count);
 		fusion->new_map_sz =  sizeof(struct MR_FW_RAID_MAP_EXT);
 
 		fusion->max_map_sz =

index b2ec7a3f7650..da1cad1ee123 100644
@@ -326,9 +326,9 @@ u8 MR_ValidateMapInfo(struct megasas_instance *instance, u64 map_id)
 	else if (instance->supportmax256vd)
 		expected_size = sizeof(struct MR_FW_RAID_MAP_EXT);
 	else
-		expected_size =
-			(sizeof(struct MR_FW_RAID_MAP) +
-			(sizeof(struct MR_LD_SPAN_MAP) * le16_to_cpu(pDrvRaidMap->ldCount)));
+		expected_size = struct_size((struct MR_FW_RAID_MAP *)0,
+					    ldSpanMap,
+					    le16_to_cpu(pDrvRaidMap->ldCount));
 
 	if (le32_to_cpu(pDrvRaidMap->totalSize) != expected_size) {
 		dev_dbg(&instance->pdev->dev, "megasas: map info structure size 0x%x",

2.34.1

If there's a revision of this series, it might make sense to explicitly state "no binary code differences" for these changes. (The location of the relocations don't matter, as you say.)

Kees Cook

Gustavo,

Applied to 6.1/scsi-staging, thanks!

Martin K. Petersen	Oracle Linux Engineering

Great. :)

Thanks, Martin.
--
Gustavo

Applied to 6.1/scsi-queue, thanks!

[1/6] scsi: megaraid_sas: Replace one-element array with flexible-array member in MR_FW_RAID_MAP

      https://git.kernel.org/mkp/scsi/c/ac23b92b27e3
[2/6] scsi: megaraid_sas: Replace one-element array with flexible-array member in MR_FW_RAID_MAP_DYNAMIC
      https://git.kernel.org/mkp/scsi/c/204a29a169f4
[3/6] scsi: megaraid_sas: Replace one-element array with flexible-array member in MR_DRV_RAID_MAP
      https://git.kernel.org/mkp/scsi/c/eeb3bab77244
[4/6] scsi: megaraid_sas: Replace one-element array with flexible-array member in MR_PD_CFG_SEQ_NUM_SYNC
      https://git.kernel.org/mkp/scsi/c/ee92366a8439
[5/6] scsi: megaraid_sas: Use struct_size() in code related to struct MR_FW_RAID_MAP
      https://git.kernel.org/mkp/scsi/c/41e830269d68
[6/6] scsi: megaraid_sas: Use struct_size() in code related to struct MR_PD_CFG_SEQ_NUM_SYNC
      https://git.kernel.org/mkp/scsi/c/48658213202c

Martin K. Petersen	Oracle Linux Engineering