On 10/1/23 21:16, Simon Glass wrote:
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?
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
