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

[Ard Biesheuvel]

On Fri, 5 Feb 2021 at 19:00, Nick Desaulniers <ndesaulniers at google.com> wrote:
>
> On Fri, Feb 5, 2021 at 12:52 AM Ard Biesheuvel <ardb at 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 at google.com>
> > Cc: Nathan Chancellor <nathan at kernel.org>
> > Cc: Guillaume Tucker <guillaume.tucker at collabora.com>
> > Link: https://lore.kernel.org/linux-arm-kernel/6c65bcef-d4e7-25fa-43cf-2c435bb61bb9@collabora.com/
> > Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
>
>
> Reviewed-by: Nick Desaulniers <ndesaulniers at google.com>
> Tested-by: Nick Desaulniers <ndesaulniers at 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 at kernel.org
> Fixes: 429f7a062e3b ("ARM: decompressor: fix BSS size calculation")
> Reported-by: Guillaume Tucker <guillaume.tucker at collabora.com>
> Reported-by: "kernelci.org bot" <bot at kernelci.org>
>

Thanks. I'll add these tags and drop this patch into the patch system

> Tests run:
>
...
>
> + 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?
>

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.


> > ---
> >  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.
>

Agreed.

> >  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