Hi Sean, On Sun, 1 Oct 2023 at 19:43, Sean Anderson <[email protected]> wrote: > > On 10/1/23 21:16, Simon Glass wrote: > > Hi Sean, > > > > On Sat, 30 Sept 2023 at 09:23, Sean Anderson <[email protected]> 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 <[email protected]> > >>> --- > >>> > >>> 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? > > Most of the time, yes. > > > 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. > > Yeah, this would be the best way to find errors in the current system. > > But maybe ll_entry_get should look like > > #define ll_entry_get(_type, _name, _list) \ > ({ \ > ll_entry_declare(_type, _name, _list); \ > _type *_ll_result = \ > &_u_boot_list_2_##_list##_2_##_name; \ > _ll_result; \ > }) > > (untested) > > Regardless, I think a link-time check would be a good sanity check.
OK, well if you take a crack at it, you have a failing case to test with! Regards, Simon
