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@xxxxxxxxxxxx> > 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 > -- Kees Cook
