On 10/1/23 21:16, Simon Glass wrote:
Hi Sean,
On Sat, 30 Sept 2023 at 09:23, Sean Anderson seanga2@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 seanga2@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?
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.
--Sean