kallsyms: Avoid weak references for kallsyms symbols
RFC PATCH 1/1 kallsyms: Avoid weak references for kallsyms symbols
kallsyms is a directory of all the symbols in the vmlinux binary, and so creating it is somewhat of a chicken-and-egg problem, as its non-zero size affects the layout of the binary, and therefore the values of the symbols.
For this reason, the kernel is linked more than once, and the first pass does not include any kallsyms data at all. For the linker to accept this, the symbol declarations describing the kallsyms metadata are emitted as having weak linkage, so they can remain unsatisfied. During the subsequent passes, the weak references are satisfied by the kallsyms metadata that was constructed based on information gathered from the preceding passes.
Weak references lead to somewhat worse codegen, because taking their address may need to produce NULL (if the reference was unsatisfied), and this is not usually supported by RIP or PC relative symbol references.
Given that these references are ultimately always satisfied in the final link, let's drop the weak annotation, and instead, provide fallback definitions in the linker script that are only emitted if an unsatisfied reference exists.
While at it, drop the FRV specific annotation that these symbols reside in .rodata - FRV is long gone.
--- include/asm-generic/vmlinux.lds.h | 9 +++++++ kernel/kallsyms.c | 6 ----- kernel/kallsyms_internal.h | 25 +++++++------------- 3 files changed, 18 insertions(+), 22 deletions(-)
include/asm-generic/vmlinux.lds.h +9 -0
@@ -460,6 +460,15 @@*/ #define RO_DATA (align) \ . = ALIGN((align)); \ + PROVIDE(kallsyms_addresses = .); \ + PROVIDE(kallsyms_offsets = .); \ + PROVIDE(kallsyms_names = .); \ + PROVIDE(kallsyms_num_syms = .); \ + PROVIDE(kallsyms_relative_base = .); \ + PROVIDE(kallsyms_token_table = .); \ + PROVIDE(kallsyms_token_index = .); \ + PROVIDE(kallsyms_markers = .); \ + PROVIDE(kallsyms_seqs_of_names = .); \ .rodata : AT(ADDR(.rodata) - LOAD_OFFSET) { \ __start_rodata = .; \ *(.rodata) *(.rodata.*) \
kernel/kallsyms.c +0 -6
@@ -331,12 +331,6 @@ static unsigned long get_symbol_pos(unsigned long addr,unsigned long symbol_start =0 , symbol_end =0 ;unsigned long i, low, high, mid; -/* This kernel should never had been booted. */ -if (!IS_ENABLED (CONFIG_KALLSYMS_BASE_RELATIVE )) -BUG_ON (!kallsyms_addresses); -else -BUG_ON (!kallsyms_offsets); -/* Do a binary search on the sorted kallsyms_addresses array. */ low =0 ; high = kallsyms_num_syms;
kernel/kallsyms_internal.h +9 -16
@@ -8,24 +8,17 @@* These will be re-linked against their real values * during the second link stage. */ -extern const unsigned long kallsyms_addresses[]__weak ; -extern const int kallsyms_offsets[]__weak ; -extern const u8 kallsyms_names[]__weak ; +extern const unsigned long kallsyms_addresses[]; +extern const int kallsyms_offsets[]; +extern const u8 kallsyms_names[]; -/* -* Tell the compiler that the count isn't in the small data section if the arch -* has one (eg: FRV). -*/ -extern const unsigned int kallsyms_num_syms -__section(".rodata") __attribute__((weak)); - -extern const unsigned long kallsyms_relative_base -__section(".rodata") __attribute__((weak)); +extern const unsigned int kallsyms_num_syms; +extern const unsigned long kallsyms_relative_base; -extern const char kallsyms_token_table[]__weak ; -extern const u16 kallsyms_token_index[]__weak ; +extern const char kallsyms_token_table[]; +extern const u16 kallsyms_token_index[]; -extern const unsigned int kallsyms_markers[]__weak ; -extern const u8 kallsyms_seqs_of_names[]__weak ; +extern const unsigned int kallsyms_markers[]; +extern const u8 kallsyms_seqs_of_names[];#endif // LINUX_KALLSYMS_INTERNAL_H_
Signature · 1 line
2.39.2
Quoted 23 lines · Ard Biesheuvel
kallsyms is a directory of all the symbols in the vmlinux binary, and so creating it is somewhat of a chicken-and-egg problem, as its non-zero size affects the layout of the binary, and therefore the values of the symbols.
For this reason, the kernel is linked more than once, and the first pass does not include any kallsyms data at all. For the linker to accept this, the symbol declarations describing the kallsyms metadata are emitted as having weak linkage, so they can remain unsatisfied. During the subsequent passes, the weak references are satisfied by the kallsyms metadata that was constructed based on information gathered from the preceding passes.
Weak references lead to somewhat worse codegen, because taking their address may need to produce NULL (if the reference was unsatisfied), and this is not usually supported by RIP or PC relative symbol references.
Given that these references are ultimately always satisfied in the final link, let's drop the weak annotation, and instead, provide fallback definitions in the linker script that are only emitted if an unsatisfied reference exists.
Quoted 7 lines
While at it, drop the FRV specific annotation that these symbols reside in .rodata - FRV is long gone.
Cc Arnd Bergmann <[email protected]>Cc Fangrui Song <[email protected]>Cc Nathan Chancellor <[email protected]>Cc Nick Desaulniers <[email protected]>
Thanks for the patch. I did some quick boot tests of this with:
- x86 defconfig + CONFIG_KALLSYMS_SELFTEST=y
- x86 defconfig + CONFIG_KALLSYMS_SELFTEST=y + CONFIG_LTO_CLANG_THIN=y
- arm64 defconfig + CONFIG_KALLSYMS_SELFTEST=y
Curiously, I only see:
[ 1.002200] kallsyms_selftest: start
in the output (when grepping for kallsyms_selftest as instructed by the help text for KALLSYMS_SELFTEST in init/Kconfig). But that happens regardless of this patch.
I did not test backtraces or live patching (seems like kallsyms is related to those reading through the help texts in init/Kconfig), or measure for binary changes.
Based on my interpretation of
https://sourceware.org/binutils/docs/ld/PROVIDE.html, this LGTM.
Quoted 33 lines
Cc Kees Cook <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>--- include/asm-generic/vmlinux.lds.h | 9 +++++++ kernel/kallsyms.c | 6 ----- kernel/kallsyms_internal.h | 25 +++++++------------- 3 files changed, 18 insertions(+), 22 deletions(-)include/asm-generic/vmlinux.lds.h +9
-0@@ -460,6 +460,15 @@*/ #define RO_DATA (align) \ . = ALIGN((align)); \ + PROVIDE(kallsyms_addresses = .); \ + PROVIDE(kallsyms_offsets = .); \ + PROVIDE(kallsyms_names = .); \ + PROVIDE(kallsyms_num_syms = .); \ + PROVIDE(kallsyms_relative_base = .); \ + PROVIDE(kallsyms_token_table = .); \ + PROVIDE(kallsyms_token_index = .); \ + PROVIDE(kallsyms_markers = .); \ + PROVIDE(kallsyms_seqs_of_names = .); \ .rodata : AT(ADDR(.rodata) - LOAD_OFFSET) { \ __start_rodata = .; \ *(.rodata) *(.rodata.*) \kernel/kallsyms.c +0
-0
kernel/kallsyms.c static unsigned long get_symbol_pos(unsigned long addr, · line 338
Quoted 8 lines
kernel/kallsyms.c +0
-5@@ -331,12 +331,6 @@ static unsigned long get_symbol_pos(unsigned long addr,unsigned long symbol_start =0 , symbol_end =0 ;unsigned long i, low, high, mid; -/* This kernel should never had been booted. */ -if (!IS_ENABLED (CONFIG_KALLSYMS_BASE_RELATIVE )) -BUG_ON (!kallsyms_addresses); -else -BUG_ON (!kallsyms_offsets);
Even previously with weak definitions, wouldn't these values always be true?
Quoted 45 lines
kernel/kallsyms.c +0
-1-/* Do a binary search on the sorted kallsyms_addresses array. */ low =0 ; high = kallsyms_num_syms;kernel/kallsyms_internal.h +9
-17@@ -8,24 +8,17 @@* These will be re-linked against their real values * during the second link stage. */ -extern const unsigned long kallsyms_addresses[]__weak ; -extern const int kallsyms_offsets[]__weak ; -extern const u8 kallsyms_names[]__weak ; +extern const unsigned long kallsyms_addresses[]; +extern const int kallsyms_offsets[]; +extern const u8 kallsyms_names[]; -/* -* Tell the compiler that the count isn't in the small data section if the arch -* has one (eg: FRV). -*/ -extern const unsigned int kallsyms_num_syms -__section(".rodata") __attribute__((weak)); - -extern const unsigned long kallsyms_relative_base -__section(".rodata") __attribute__((weak)); +extern const unsigned int kallsyms_num_syms; +extern const unsigned long kallsyms_relative_base; -extern const char kallsyms_token_table[]__weak ; -extern const u16 kallsyms_token_index[]__weak ; +extern const char kallsyms_token_table[]; +extern const u16 kallsyms_token_index[]; -extern const unsigned int kallsyms_markers[]__weak ; -extern const u8 kallsyms_seqs_of_names[]__weak ; +extern const unsigned int kallsyms_markers[]; +extern const u8 kallsyms_seqs_of_names[];#endif // LINUX_KALLSYMS_INTERNAL_H_ --2.39.2
Signature · 2 lines
Thanks, ~Nick Desaulniers
Quoted 49 lines · Nick Desaulniers
Quoted 23 lines · Ard Biesheuvel
kallsyms is a directory of all the symbols in the vmlinux binary, and so creating it is somewhat of a chicken-and-egg problem, as its non-zero size affects the layout of the binary, and therefore the values of the symbols.
For this reason, the kernel is linked more than once, and the first pass does not include any kallsyms data at all. For the linker to accept this, the symbol declarations describing the kallsyms metadata are emitted as having weak linkage, so they can remain unsatisfied. During the subsequent passes, the weak references are satisfied by the kallsyms metadata that was constructed based on information gathered from the preceding passes.
Weak references lead to somewhat worse codegen, because taking their address may need to produce NULL (if the reference was unsatisfied), and this is not usually supported by RIP or PC relative symbol references.
Given that these references are ultimately always satisfied in the final link, let's drop the weak annotation, and instead, provide fallback definitions in the linker script that are only emitted if an unsatisfied reference exists.
Quoted 7 lines
While at it, drop the FRV specific annotation that these symbols reside in .rodata - FRV is long gone.
Cc Arnd Bergmann <[email protected]>Cc Fangrui Song <[email protected]>Cc Nathan Chancellor <[email protected]>Cc Nick Desaulniers <[email protected]>Thanks for the patch. I did some quick boot tests of this with:
- x86 defconfig +CONFIG_KALLSYMS_SELFTEST=y
- x86 defconfig +CONFIG_KALLSYMS_SELFTEST=y+CONFIG_LTO_CLANG_THIN=y
- arm64 defconfig +CONFIG_KALLSYMS_SELFTEST=yCuriously, I only see:
[ 1.002200] kallsyms_selftest: startin the output (when grepping for
kallsyms_selftestas instructed by the help text forKALLSYMS_SELFTESTin init/Kconfig). But that happens regardless of this patch.I did not test backtraces or live patching (seems like kallsyms is related to those reading through the help texts in init/Kconfig), or measure for binary changes.
Quoted 7 lines
Based on my interpretation of
https://sourceware.org/binutils/docs/ld/PROVIDE.html, this LGTM.Reviewed-by Nick Desaulniers <[email protected]>
Thanks!
Quoted 36 lines
Quoted 36 lines
Cc Kees Cook <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>--- include/asm-generic/vmlinux.lds.h | 9 +++++++ kernel/kallsyms.c | 6 ----- kernel/kallsyms_internal.h | 25 +++++++------------- 3 files changed, 18 insertions(+), 22 deletions(-)include/asm-generic/vmlinux.lds.h +9
-0@@ -460,6 +460,15 @@*/ #define RO_DATA (align) \ . = ALIGN((align)); \ + PROVIDE(kallsyms_addresses = .); \ + PROVIDE(kallsyms_offsets = .); \ + PROVIDE(kallsyms_names = .); \ + PROVIDE(kallsyms_num_syms = .); \ + PROVIDE(kallsyms_relative_base = .); \ + PROVIDE(kallsyms_token_table = .); \ + PROVIDE(kallsyms_token_index = .); \ + PROVIDE(kallsyms_markers = .); \ + PROVIDE(kallsyms_seqs_of_names = .); \ .rodata : AT(ADDR(.rodata) - LOAD_OFFSET) { \ __start_rodata = .; \ *(.rodata) *(.rodata.*) \kernel/kallsyms.c +0
-0@@ -331,12 +331,6 @@ static unsigned long get_symbol_pos(unsigned long addr,unsigned long symbol_start =0 , symbol_end =0 ;unsigned long i, low, high, mid;
Quoted 8 lines
kernel/kallsyms.c static unsigned long get_symbol_pos(unsigned long addr, · line 338
Quoted 5 lines
kernel/kallsyms.c +0
-5-/* This kernel should never had been booted. */ -if (!IS_ENABLED (CONFIG_KALLSYMS_BASE_RELATIVE )) -BUG_ON (!kallsyms_addresses); -else -BUG_ON (!kallsyms_offsets);Even previously with weak definitions, wouldn't these values always be true?
To be pedantic: this patch does not deal with weak definitions at all. It only deals with weak *references*, which can remain unsatisfied.
In this particular case, one of these would remain unsatisfied, while the other one needs to be defined in order for kallsyms not to crash and burn as soon as it is used. After this change, both will always be non-NULL, hence the removal.
Quoted 50 lines
Quoted 43 lines
kernel/kallsyms.c +0
-1-/* Do a binary search on the sorted kallsyms_addresses array. */ low =0 ; high = kallsyms_num_syms;kernel/kallsyms_internal.h +9
-17@@ -8,24 +8,17 @@* These will be re-linked against their real values * during the second link stage. */ -extern const unsigned long kallsyms_addresses[]__weak ; -extern const int kallsyms_offsets[]__weak ; -extern const u8 kallsyms_names[]__weak ; +extern const unsigned long kallsyms_addresses[]; +extern const int kallsyms_offsets[]; +extern const u8 kallsyms_names[]; -/* -* Tell the compiler that the count isn't in the small data section if the arch -* has one (eg: FRV). -*/ -extern const unsigned int kallsyms_num_syms -__section(".rodata") __attribute__((weak)); - -extern const unsigned long kallsyms_relative_base -__section(".rodata") __attribute__((weak)); +extern const unsigned int kallsyms_num_syms; +extern const unsigned long kallsyms_relative_base; -extern const char kallsyms_token_table[]__weak ; -extern const u16 kallsyms_token_index[]__weak ; +extern const char kallsyms_token_table[]; +extern const u16 kallsyms_token_index[]; -extern const unsigned int kallsyms_markers[]__weak ; -extern const u8 kallsyms_seqs_of_names[]__weak ; +extern const unsigned int kallsyms_markers[]; +extern const u8 kallsyms_seqs_of_names[];#endif // LINUX_KALLSYMS_INTERNAL_H_ --Quoted 2 lines
2.39.2
--
Thanks,
~Nick Desaulniers
Quoted 44 lines · Ard Biesheuvel
kallsyms is a directory of all the symbols in the vmlinux binary, and so creating it is somewhat of a chicken-and-egg problem, as its non-zero size affects the layout of the binary, and therefore the values of the symbols.
For this reason, the kernel is linked more than once, and the first pass does not include any kallsyms data at all. For the linker to accept this, the symbol declarations describing the kallsyms metadata are emitted as having weak linkage, so they can remain unsatisfied. During the subsequent passes, the weak references are satisfied by the kallsyms metadata that was constructed based on information gathered from the preceding passes.
Weak references lead to somewhat worse codegen, because taking their address may need to produce NULL (if the reference was unsatisfied), and this is not usually supported by RIP or PC relative symbol references.
Given that these references are ultimately always satisfied in the final link, let's drop the weak annotation, and instead, provide fallback definitions in the linker script that are only emitted if an unsatisfied reference exists.
While at it, drop the FRV specific annotation that these symbols reside in .rodata - FRV is long gone.
Cc Arnd Bergmann <[email protected]>Cc Fangrui Song <[email protected]>Cc Nathan Chancellor <[email protected]>Cc Nick Desaulniers <[email protected]>Cc Kees Cook <[email protected]>Signed-off-by Ard Biesheuvel <[email protected]>--- include/asm-generic/vmlinux.lds.h | 9 +++++++ kernel/kallsyms.c | 6 ----- kernel/kallsyms_internal.h | 25 +++++++------------- 3 files changed, 18 insertions(+), 22 deletions(-)include/asm-generic/vmlinux.lds.h +0
-0@@ -460,6 +460,15 @@*/ #define RO_DATA (align) \
include/asm-generic/vmlinux.lds.h line 471
Quoted 10 lines
include/asm-generic/vmlinux.lds.h +9
-0. = ALIGN((align)); \ + PROVIDE(kallsyms_addresses = .); \ + PROVIDE(kallsyms_offsets = .); \ + PROVIDE(kallsyms_names = .); \ + PROVIDE(kallsyms_num_syms = .); \ + PROVIDE(kallsyms_relative_base = .); \ + PROVIDE(kallsyms_token_table = .); \ + PROVIDE(kallsyms_token_index = .); \ + PROVIDE(kallsyms_markers = .); \ + PROVIDE(kallsyms_seqs_of_names = .); \
Perhaps add comments here and in kernel/kallsyms_internal.h to correlate this list and the externs? "This list is needed because ..., see the externs in kallsyms_internal.h." or similar?
Otherwise, yeah, looks good.
Signature · 1 line
Kees Cook