ARM: decompressor: fix BSS size calculation for LLVM ld.lld
PATCH 1/1 ARM: decompressor: fix BSS size calculation for LLVM ld.lld
The LLVM ld.lld linker uses a different symbol type for __bss_start, resulting in the calculation of KBSS_SZ to be thrown off. Up until now, this has gone unnoticed as it only affects the appended DTB case, but pending changes for ARM in the way the decompressed kernel is cleaned from the caches has uncovered this problem.
On a ld.lld build:
$ nm vmlinux |grep bss_ c1c22034 D __bss_start c1c86e98 B __bss_stop
resulting in
433: c1c86e98 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size
which is obviously incorrect, and may cause the cache clean to access unmapped memory, or cause the size calculation to wrap, resulting in no cache clean to be performed at all.
Fix this by updating the sed regex to take D type symbols into account.
--- arch/arm/boot/compressed/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm/boot/compressed/Makefile +2 -2
@@ -115,8 +115,8 @@ asflags-y := -DZIMAGE # Supply kernel BSS size to the decompressor via a linker symbol. KBSS_SZ = $(shell echo $$(($$($(NM) $(obj)/../../../../vmlinux | \ - sed -n -e 's/^\([^ ]*\) [AB] __bss_start$$/-0x\1/p' \ - -e 's/^\([^ ]*\) [AB] __bss_stop$$/+0x\1/p') )) ) + sed -n -e 's/^\([^ ]*\) [ABD] __bss_start$$/-0x\1/p' \ + -e 's/^\([^ ]*\) [ABD] __bss_stop$$/+0x\1/p') )) ) LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ) # Supply ZRELADDR to the decompressor via a linker symbol. ifneq ($(CONFIG_AUTO_ZRELADDR),y)
Signature · 7 lines
2.30.0 _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 7 lines · Ard Biesheuvel
The LLVM ld.lld linker uses a different symbol type for
__bss_start, resulting in the calculation ofKBSS_SZto be thrown off. Up until now, this has gone unnoticed as it only affects the appended DTB case, but pending changes for ARM in the way the decompressed kernel is cleaned from the caches has uncovered this problem.
Quoted 8 lines
On a ld.lld build:
$ nm vmlinux |grep bss_ c1c22034 D __bss_start c1c86e98 B __bss_stopresulting in
$ readelf -s arch/arm/boot/compressed/vmlinux | grep bss_size
Quoted 6 lines
433:
c1c86e980 NOTYPE GLOBAL DEFAULT ABS_kernel_bss_sizewhich is obviously incorrect, and may cause the cache clean to access unmapped memory, or cause the size calculation to wrap, resulting in no cache clean to be performed at all.
Quoted 7 lines
Fix this by updating the sed regex to take D type symbols into account.
Cc Nick Desaulniers <[email protected]>Cc Nathan Chancellor <[email protected]>Cc Guillaume Tucker <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>
Thanks for debugging+fixing this, and Guillaume for the report. It's nice to see a fix so early; thinking back to last year before KernelCI integration, we probably would have only noticed when CrOS went to upgrade their rk3288 platform devices.
Some other tags that might be nice to apply:
Tests run:
Before (LLD):
$ llvm-nm vmlinux | grep bss_ c1c1a3fc D __bss_start c1c7e998 B __bss_stop $ llvm-readelf -s arch/arm/boot/compressed/vmlinux | grep bss_size 440: c1c7e998 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size
After (LLD):
$ llvm-nm vmlinux | grep bss_ c1c1a3fc D __bss_start c1c7e998 B __bss_stop $ llvm-readelf -s arch/arm/boot/compressed/vmlinux | grep bss_size 440: 0006459c 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size
Before (BFD):
$ llvm-nm vmlinux | grep bss_ c1c1a3fc B __bss_start c1c7e998 B __bss_stop $ llvm-readelf -s arch/arm/boot/compressed/vmlinux | grep bss_size 463: 0006459c 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size
After (BFD):
$ llvm-nm vmlinux | grep bss_ c1c1a3fc B __bss_start c1c7e998 B __bss_stop $ llvm-readelf -s arch/arm/boot/compressed/vmlinux | grep bss_size 463: 0006459c 0 NOTYPE GLOBAL DEFAULT ABS _kernel_bss_size
+ Fangrui,
Fangrui, __bss_start looks like it's linker script defined by the BSS_SECTION macro from include/asm-generic/vmlinux.lds.h:1160 being used in arch/arm/kernel/vmlinux.lds.S:149. Should these symbols be placed in .bss? Might save a few bytes in the image, unless there's an initial value that's encoded with them?
Quoted 9 lines
--- arch/arm/boot/compressed/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/arm/boot/compressed/Makefile +0
-0
arch/arm/boot/compressed/Makefile asflags-y := -DZIMAGE · line 119
Quoted 7 lines
arch/arm/boot/compressed/Makefile +2
-2@@ -115,8 +115,8 @@ asflags-y := -DZIMAGE # Supply kernel BSS size to the decompressor via a linker symbol. KBSS_SZ = $(shell echo $$(($$($(NM) $(obj)/../../../../vmlinux | \ - sed -n -e 's/^\([^ ]*\) [AB] __bss_start$$/-0x\1/p' \ - -e 's/^\([^ ]*\) [AB] __bss_stop$$/+0x\1/p') )) ) + sed -n -e 's/^\([^ ]*\) [ABD] __bss_start$$/-0x\1/p' \ + -e 's/^\([^ ]*\) [ABD] __bss_stop$$/+0x\1/p') )) )
I wasn't sure whether we still needed A, but
commit 6cea14f55474 ("ARM: replace unnecessary perl with sed and the shell $(( )) operator")
references that depending on the version of binutils you might observe that. There's no more info on which version or under what conditions. Lest we reintroduce this same problem for that version, it's fine to leave it.
Quoted 6 lines
arch/arm/boot/compressed/Makefile +0
-1LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ) # Supply ZRELADDR to the decompressor via a linker symbol. ifneq ($(CONFIG_AUTO_ZRELADDR),y) --2.30.0
Signature · 7 lines
Thanks, ~Nick Desaulniers _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 43 lines · Nick Desaulniers
Quoted 7 lines · Ard Biesheuvel
The LLVM ld.lld linker uses a different symbol type for
__bss_start, resulting in the calculation ofKBSS_SZto be thrown off. Up until now, this has gone unnoticed as it only affects the appended DTB case, but pending changes for ARM in the way the decompressed kernel is cleaned from the caches has uncovered this problem.Quoted 8 lines
On a ld.lld build:
$ nm vmlinux |grep bss_ c1c22034 D __bss_start c1c86e98 B __bss_stopresulting in
$ readelf -s arch/arm/boot/compressed/vmlinux | grep bss_sizeQuoted 6 lines
433:
c1c86e980 NOTYPE GLOBAL DEFAULT ABS_kernel_bss_sizewhich is obviously incorrect, and may cause the cache clean to access unmapped memory, or cause the size calculation to wrap, resulting in no cache clean to be performed at all.
Quoted 7 lines
Fix this by updating the sed regex to take D type symbols into account.
Cc Nick Desaulniers <[email protected]>Cc Nathan Chancellor <[email protected]>Cc Guillaume Tucker <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>Reviewed-by Nick Desaulniers <[email protected]>Tested-by Nick Desaulniers <[email protected]>Thanks for debugging+fixing this, and Guillaume for the report. It's nice to see a fix so early; thinking back to last year before KernelCI integration, we probably would have only noticed when CrOS went to upgrade their rk3288 platform devices.
Quoted 7 lines
Some other tags that might be nice to apply:
Fixes 429f7a062e3b ("ARM: decompressor: fix BSS size calculation")Reported-by Guillaume Tucker <[email protected]>Reported-by "kernelci.org bot" <[email protected]>
Thanks. I'll add these tags and drop this patch into the patch system
Quoted 2 lines
Tests run:
...
Quoted 8 lines
+ Fangrui,
Fangrui,__bss_startlooks like it's linker script defined by theBSS_SECTIONmacro frominclude/asm-generic/vmlinux.lds.h:1160being used inarch/arm/kernel/vmlinux.lds.S:149. Should these symbols be placed in .bss? Might save a few bytes in the image, unless there's an initial value that's encoded with them?
Not sure what you are asking here. These symbols just delineate .bss, they don't take up any space themselves.
What seems to be happening is that the placement of __bss_start outside of the .sbss/.bss section declarations causes it to be annotated as residing in .data.
Quoted 10 lines
Quoted 10 lines
--- arch/arm/boot/compressed/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/arm/boot/compressed/Makefile +0
-0@@ -115,8 +115,8 @@ asflags-y := -DZIMAGE
Quoted 15 lines
arch/arm/boot/compressed/Makefile asflags-y := -DZIMAGE · line 119
Quoted 6 lines
arch/arm/boot/compressed/Makefile +2
-2# Supply kernel BSS size to the decompressor via a linker symbol. KBSS_SZ = $(shell echo $$(($$($(NM) $(obj)/../../../../vmlinux | \ - sed -n -e 's/^\([^ ]*\) [AB] __bss_start$$/-0x\1/p' \ - -e 's/^\([^ ]*\) [AB] __bss_stop$$/+0x\1/p') )) ) + sed -n -e 's/^\([^ ]*\) [ABD] __bss_start$$/-0x\1/p' \ + -e 's/^\([^ ]*\) [ABD] __bss_stop$$/+0x\1/p') )) )I wasn't sure whether we still needed
A, but
commit6cea14f55474("ARM: replace unnecessary perl with sed and the shell $(( )) operator")
references that depending on the version of binutils you might observe that. There's no more info on which version or under what conditions. Lest we reintroduce this same problem for that version, it's fine to leave it.
Agreed.
Quoted 11 lines
Quoted 6 lines
arch/arm/boot/compressed/Makefile +0
-1LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ) # Supply ZRELADDR to the decompressor via a linker symbol. ifneq ($(CONFIG_AUTO_ZRELADDR),y) --2.30.0
--
Thanks,
~Nick Desaulniers
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 66 lines · Ard Biesheuvel
Quoted 43 lines · Nick Desaulniers
Quoted 7 lines · Ard Biesheuvel
The LLVM ld.lld linker uses a different symbol type for
__bss_start, resulting in the calculation ofKBSS_SZto be thrown off. Up until now, this has gone unnoticed as it only affects the appended DTB case, but pending changes for ARM in the way the decompressed kernel is cleaned from the caches has uncovered this problem.Quoted 8 lines
On a ld.lld build:
$ nm vmlinux |grep bss_ c1c22034 D __bss_start c1c86e98 B __bss_stopresulting in
$ readelf -s arch/arm/boot/compressed/vmlinux | grep bss_sizeQuoted 6 lines
433:
c1c86e980 NOTYPE GLOBAL DEFAULT ABS_kernel_bss_sizewhich is obviously incorrect, and may cause the cache clean to access unmapped memory, or cause the size calculation to wrap, resulting in no cache clean to be performed at all.
Quoted 7 lines
Fix this by updating the sed regex to take D type symbols into account.
Cc Nick Desaulniers <[email protected]>Cc Nathan Chancellor <[email protected]>Cc Guillaume Tucker <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>Reviewed-by Nick Desaulniers <[email protected]>Tested-by Nick Desaulniers <[email protected]>Thanks for debugging+fixing this, and Guillaume for the report. It's nice to see a fix so early; thinking back to last year before KernelCI integration, we probably would have only noticed when CrOS went to upgrade their rk3288 platform devices.
Quoted 7 lines
Some other tags that might be nice to apply:
Fixes 429f7a062e3b ("ARM: decompressor: fix BSS size calculation")Reported-by Guillaume Tucker <[email protected]>Reported-by "kernelci.org bot" <[email protected]>Thanks. I'll add these tags and drop this patch into the patch system
Quoted 2 lines
Tests run:
...
Quoted 8 lines
+ Fangrui,
Fangrui,__bss_startlooks like it's linker script defined by theBSS_SECTIONmacro frominclude/asm-generic/vmlinux.lds.h:1160being used inarch/arm/kernel/vmlinux.lds.S:149. Should these symbols be placed in .bss? Might save a few bytes in the image, unless there's an initial value that's encoded with them?
Quoted 6 lines
Not sure what you are asking here. These symbols just delineate .bss, they don't take up any space themselves.
What seems to be happening is that the placement of
__bss_startoutside of the .sbss/.bss section declarations causes it to be annotated as residing in .data.
In my build (make -sk -j 50 ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- LLVM=1 O=/tmp/out/arm -j 50 defconfig vmlinux), vmlinux has:
[34] .data..percpu PROGBITS c195f000 151f000 00c20c 00 WA 0 0 64 [35] .data PROGBITS c1a00000 1530000 217914 00 WA 0 0 4096 [36] __bug_table PROGBITS c1c17918 1747918 00a5cc 00 WA 0 0 4 [37] .sbss PROGBITS c1c21ee4 1751ee4 000000 00 WA 0 0 1 [38] .bss NOBITS c1c21f00 1751f00 063f98 00 WA 0 0 64
__bss_start is defined relative to __bug_table.
The issue is that __bss_start is defined outside of the output section description of .bss, so its st_shndx (section) is not guaranteed.
I noticed that many linker script fragments used by the kernel have similar problems. The delineating symbols are defined outside of output sections. With ld --orphan-handling=, we can somewhat guarantee there are no undesired interleaving sections, but it is hard to ensure their st_shndx values.
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
--- arch/arm/boot/compressed/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/arm/boot/compressed/Makefile +0
-0@@ -115,8 +115,8 @@ asflags-y := -DZIMAGE
Quoted 15 lines
Quoted 7 lines
arch/arm/boot/compressed/Makefile asflags-y := -DZIMAGE · line 119
Quoted 6 lines
arch/arm/boot/compressed/Makefile +2
-2# Supply kernel BSS size to the decompressor via a linker symbol. KBSS_SZ = $(shell echo $$(($$($(NM) $(obj)/../../../../vmlinux | \ - sed -n -e 's/^\([^ ]*\) [AB] __bss_start$$/-0x\1/p' \ - -e 's/^\([^ ]*\) [AB] __bss_stop$$/+0x\1/p') )) ) + sed -n -e 's/^\([^ ]*\) [ABD] __bss_start$$/-0x\1/p' \ + -e 's/^\([^ ]*\) [ABD] __bss_stop$$/+0x\1/p') )) )Quoted 8 lines
I wasn't sure whether we still needed
A, but
commit6cea14f55474("ARM: replace unnecessary perl with sed and the shell $(( )) operator")
references that depending on the version of binutils you might observe that. There's no more info on which version or under what conditions. Lest we reintroduce this same problem for that version, it's fine to leave it.
In my observation a symbol may be SHN_ABS (nm key 'A') if it is
* defined by --defsym
* a symbol assignment outside of output sections in -no-pie link mode
Quoted 13 lines
Agreed.
Quoted 11 lines
Quoted 6 lines
arch/arm/boot/compressed/Makefile +0
-1LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ) # Supply ZRELADDR to the decompressor via a linker symbol. ifneq ($(CONFIG_AUTO_ZRELADDR),y) --2.30.0
--
Thanks,
~Nick Desaulniers
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 25 lines · Ard Biesheuvel
Quoted 15 lines · Nick Desaulniers
Quoted 7 lines · Ard Biesheuvel
The LLVM ld.lld linker uses a different symbol type for
__bss_start, resulting in the calculation ofKBSS_SZto be thrown off. Up until now, this has gone unnoticed as it only affects the appended DTB case, but pending changes for ARM in the way the decompressed kernel is cleaned from the caches has uncovered this problem.Quoted 6 lines
On a ld.lld build:
$ nm vmlinux |grep bss_ c1c22034 D __bss_start c1c86e98 B __bss_stopQuoted 7 lines
+ Fangrui,
Fangrui,__bss_startlooks like it's linker script defined by theBSS_SECTIONmacro frominclude/asm-generic/vmlinux.lds.h:1160being used inarch/arm/kernel/vmlinux.lds.S:149. Should these symbols be placed in .bss? Might save a few bytes in the image, unless there's an initial value that's encoded with them?
Quoted 6 lines
Not sure what you are asking here. These symbols just delineate .bss, they don't take up any space themselves.
What seems to be happening is that the placement of
__bss_startoutside of the .sbss/.bss section declarations causes it to be annotated as residing in .data.
Perhaps a misunderstanding on my part on how symbols are represented in ELF, but my understanding is:
$ cat foo.c int foo; int bar = 0; int baz = 42; $ cc -c foo.c $ nm foo.o 0000000000000004 B bar 0000000000000000 D baz 0000000000000000 B foo $ ls -l foo.o -rw-r----- 1 ndesaulniers primarygroup 1016 Feb 5 10:47 foo.o
$ cat bar.c int foo; int bar = 0; int baz = 0; // changed from foo.c $ cc -c bar.o $ nm bar.o 0000000000000004 B bar 0000000000000008 B baz 0000000000000000 B foo $ ls -l bar.o -rw-r----- 1 ndesaulniers primarygroup 1008 Feb 5 10:48 bar.o # ^ smaller object file
That if a symbol's value was an address within .bss, then there was no additional space reserved in an ELF object since the initial value for all such symbols in the section can memset to 0 by the loader. But if a symbol's value was an address in .data, that initial value must occupy space in the object file. Perhaps that's not the case though for linker defined symbols, since I'm not sure what data/initial value they would correspond to besides the Elf{32|64}_Sym's value in the symbol table? (I should go reread the relevant section from Expert C
Programming: Deep C Secrets, or finish reading Linkers and Loaders).
Signature · 7 lines
Thanks, ~Nick Desaulniers _______________________________________________ linux-arm-kernel mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Quoted 62 lines · Nick Desaulniers
Quoted 25 lines · Ard Biesheuvel
Quoted 15 lines · Nick Desaulniers
Quoted 7 lines · Ard Biesheuvel
The LLVM ld.lld linker uses a different symbol type for
__bss_start, resulting in the calculation ofKBSS_SZto be thrown off. Up until now, this has gone unnoticed as it only affects the appended DTB case, but pending changes for ARM in the way the decompressed kernel is cleaned from the caches has uncovered this problem.Quoted 6 lines
On a ld.lld build:
$ nm vmlinux |grep bss_ c1c22034 D __bss_start c1c86e98 B __bss_stopQuoted 7 lines
+ Fangrui,
Fangrui,__bss_startlooks like it's linker script defined by theBSS_SECTIONmacro frominclude/asm-generic/vmlinux.lds.h:1160being used inarch/arm/kernel/vmlinux.lds.S:149. Should these symbols be placed in .bss? Might save a few bytes in the image, unless there's an initial value that's encoded with them?Quoted 6 lines
Not sure what you are asking here. These symbols just delineate .bss, they don't take up any space themselves.
What seems to be happening is that the placement of
__bss_startoutside of the .sbss/.bss section declarations causes it to be annotated as residing in .data.Perhaps a misunderstanding on my part on how symbols are represented in ELF, but my understanding is:
$ cat foo.c int foo; int bar = 0; int baz = 42; $ cc -c foo.c $ nm foo.o 0000000000000004 B bar 0000000000000000 D baz 0000000000000000 B foo $ ls -l foo.o -rw-r----- 1 ndesaulniers primarygroup 1016 Feb 5 10:47 foo.o$ cat bar.c int foo; int bar = 0; int baz = 0; // changed from foo.c $ cc -c bar.o $ nm bar.o 0000000000000004 B bar 0000000000000008 B baz 0000000000000000 B foo $ ls -l bar.o -rw-r----- 1 ndesaulniers primarygroup 1008 Feb 5 10:48 bar.o # ^ smaller object file
Quoted 9 lines
That if a symbol's value was an address within .bss, then there was no additional space reserved in an ELF object since the initial value for all such symbols in the section can memset to 0 by the loader. But if a symbol's value was an address in .data, that initial value must occupy space in the object file. Perhaps that's not the case though for linker defined symbols, since I'm not sure what data/initial value they would correspond to besides the Elf{32|64}_Sym's value in the symbol table? (I should go reread the relevant section from Expert C
Programming: Deep C Secrets, or finish reading Linkers and Loaders).
A symbol defined in C allocates space in the binary (either in .data or in .bss), along with a symbol to refer to it.
A linker defined symbol such as __bss_start is just a pointer into the binary. It will alias with whichever C variable ended up at offset 0x0 in the section. Remember that clearing [__bss_start, __bss_end] in the startup code is what initializes those variables to zero.
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Guillaume Tucker
Tested-by kernelci.org botQuoted 46 lines · Ard Biesheuvel
Quoted 45 lines · Nick Desaulniers
Quoted 7 lines · Ard Biesheuvel
The LLVM ld.lld linker uses a different symbol type for
__bss_start, resulting in the calculation ofKBSS_SZto be thrown off. Up until now, this has gone unnoticed as it only affects the appended DTB case, but pending changes for ARM in the way the decompressed kernel is cleaned from the caches has uncovered this problem.Quoted 8 lines
On a ld.lld build:
$ nm vmlinux |grep bss_ c1c22034 D __bss_start c1c86e98 B __bss_stopresulting in
$ readelf -s arch/arm/boot/compressed/vmlinux | grep bss_sizeQuoted 6 lines
433:
c1c86e980 NOTYPE GLOBAL DEFAULT ABS_kernel_bss_sizewhich is obviously incorrect, and may cause the cache clean to access unmapped memory, or cause the size calculation to wrap, resulting in no cache clean to be performed at all.
Quoted 7 lines
Fix this by updating the sed regex to take D type symbols into account.
Cc Nick Desaulniers <[email protected]>Cc Nathan Chancellor <[email protected]>Cc Guillaume Tucker <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>Reviewed-by Nick Desaulniers <[email protected]>Tested-by Nick Desaulniers <[email protected]>Thanks for debugging+fixing this, and Guillaume for the report. It's nice to see a fix so early; thinking back to last year before KernelCI integration, we probably would have only noticed when CrOS went to upgrade their rk3288 platform devices.
Some other tags that might be nice to apply:
Quoted 7 lines
Quoted 5 lines
Fixes 429f7a062e3b ("ARM: decompressor: fix BSS size calculation")Reported-by Guillaume Tucker <[email protected]>Reported-by "kernelci.org bot" <[email protected]>Thanks. I'll add these tags and drop this patch into the patch system
Thanks! You may even add:
See some results here, showing it's all booting fine:
with this revision from your for-kernelci branch:
Best wishes,
Guillaume
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel