Hi Sean, On Sat, 30 Sept 2023 at 09:23, Sean Anderson <sean...@gmail.com> wrote: > > On 9/30/23 10:36, Sean Anderson wrote: > > When ll_entry_get is used on a list entry ll_entry_declare'd in the same > > file, the lack of alignment on the access will override the > > ll_entry_declare alignment. This causes GCC to use the default section > > alignment of 32 bytes. As list entries are not necessarily 32-byte aligned, > > this will cause a gap in the linker list, corrupting further entries. > > > > As a specific example, get_fs_loader uses DM_DRIVER_GET(fs_loader) in the > > same file where U_BOOT_DRIVER(fs_loader) is present. This causes a crash > > when walking the driver list. > > > > Fix this by adding appropriate alignment to all accesses. > > > > Fixes: 42ebaae3a33 ("common: Implement support for linker-generated arrays") > > Signed-off-by: Sean Anderson <sean...@gmail.com> > > --- > > > > include/linker_lists.h | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/include/linker_lists.h b/include/linker_lists.h > > index f9a2ee0c762..e0c8a01b9ba 100644 > > --- a/include/linker_lists.h > > +++ b/include/linker_lists.h > > @@ -209,7 +209,8 @@ > > */ > > #define ll_entry_get(_type, _name, _list) \ > > ({ \ > > - extern _type _u_boot_list_2_##_list##_2_##_name; \ > > + extern _type __aligned(4) \ > > + _u_boot_list_2_##_list##_2_##_name; \ > > _type *_ll_result = \ > > &_u_boot_list_2_##_list##_2_##_name; \ > > _ll_result; \ > > @@ -229,7 +230,7 @@ > > * @_list: name of the list > > */ > > #define ll_entry_ref(_type, _name, _list) \ > > - ((_type *)&_u_boot_list_2_##_list##_2_##_name) > > + ((_type __aligned(4) *)&_u_boot_list_2_##_list##_2_##_name) > > OK, so this causes an error in clang. And it isn't really necessary > because the entry is already declared at this point. > > So I guess the right fix is to replace DM_DRIVER_GET with DM_DRIVER_REF in > get_fs_loader. But this seems like a really big footgun. You can use the > wrong one and there are no errors except at runtime. I wonder if we can add > a warning of some kind?
I can imagine having a runtime check, something like: ll_check(sizeof(struct something)) which checks that the linker list (end - start) is a multiple of the struct size. Do you think that would find the problem? If so, then it could be perhaps be turned into a link-time check. This produces a list of the linker lists along with their individual members: or ll in $(nm /tmp/b/coreboot/u-boot |grep u_boot_list_2 |sed 's/.*_u_boot_list_2_\(.*\)_2_.*/\1/' |uniq); do echo; echo "linker list: %ll"; nm /tmp/b/coreboot/u-boot |grep $ll; done ... linker list: ut_str_test 011a9a20 D _u_boot_list_2_ut_str_test_2_str_dectoul 011a9a30 D _u_boot_list_2_ut_str_test_2_str_hextoul 011a9a40 D _u_boot_list_2_ut_str_test_2_str_itoa 011a9a50 D _u_boot_list_2_ut_str_test_2_str_simple_strtoul 011a9a60 D _u_boot_list_2_ut_str_test_2_str_simple_strtoull 011a9a70 D _u_boot_list_2_ut_str_test_2_str_trailing 011a9a80 D _u_boot_list_2_ut_str_test_2_str_upper 011a9a90 D _u_boot_list_2_ut_str_test_2_str_xtoa 011a9aa0 D _u_boot_list_2_ut_str_test_2_test_str_to_list ... Then you can check that the address of each one increments by the same amount. Maybe. Regards, Simon