| Message ID | cover.1660592640.git.gustavoars@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Replace one-element arrays with flexible-array members | expand |
On Mon, Aug 15, 2022 at 04:35:19PM -0500, Gustavo A. R. Silva wrote: > 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. 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.) Reviewed-by: Kees Cook <keescook@chromium.org> > 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]. > > Link: https://en.wikipedia.org/wiki/Flexible_array_member > Link: https://www.kernel.org/doc/html/v5.10/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 > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215943 [1] > Link: https://reviews.llvm.org/D126864 [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 >
Gustavo, > This series aims to replace one-element arrays with flexible-array > members in drivers/scsi/megaraid/ Applied to 6.1/scsi-staging, thanks!
On Mon, Aug 22, 2022 at 11:59:22PM -0400, Martin K. Petersen wrote: > > Gustavo, > > > This series aims to replace one-element arrays with flexible-array > > members in drivers/scsi/megaraid/ > > Applied to 6.1/scsi-staging, thanks! Great. :) Thanks, Martin. -- Gustavo
On Mon, 15 Aug 2022 16:35:19 -0500, Gustavo A. R. Silva wrote: > 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: > > [...] 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
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]. Link: https://en.wikipedia.org/wiki/Flexible_array_member Link: https://www.kernel.org/doc/html/v5.10/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 Link: https://bugzilla.kernel.org/show_bug.cgi?id=215943 [1] Link: https://reviews.llvm.org/D126864 [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(-)