Re: [PATCH] ARM: decompressor: fix BSS size calculation for LLVM ld.lld

[Nick Desaulniers <ndesaulniers@google.com>]

On Fri, Feb 5, 2021 at 12:52 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> 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
>

$ readelf -s arch/arm/boot/compressed/vmlinux | grep bss_size

>   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.
>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Guillaume Tucker <guillaume.tucker@collabora.com>
> Link: https://lore.kernel.org/linux-arm-kernel/6c65bcef-d4e7-25fa-43cf-2c435bb61bb9@collabora.com/
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>


Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>

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:

Cc: stable@kernel.org
Fixes: 429f7a062e3b ("ARM: decompressor: fix BSS size calculation")
Reported-by: Guillaume Tucker <guillaume.tucker@collabora.com>
Reported-by: "kernelci.org bot" <bot@kernelci.org>

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?

> ---
>  arch/arm/boot/compressed/Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> index fb521efcc6c2..54307db7854d 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -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.

>  LDFLAGS_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
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel