Re: [RFC PATCH] kallsyms: Avoid weak references for kallsyms symbols

[Kees Cook <keescook@chromium.org>]

On Thu, May 04, 2023 at 07:43:20PM +0200, Ard Biesheuvel wrote:
> 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 <arnd@arndb.de>
> Cc: Fangrui Song <maskray@google.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  include/asm-generic/vmlinux.lds.h |  9 +++++++
>  kernel/kallsyms.c                 |  6 -----
>  kernel/kallsyms_internal.h        | 25 +++++++-------------
>  3 files changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index d1f57e4868ed341d..dd42c0fcad2b519f 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -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 = .);				\

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.

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook