Re: [PATCH] x86/boot: avoid relaxable symbols with Clang

[Nick Desaulniers]

On Fri, Aug 7, 2020 at 2:29 PM Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote:
>
> On Fri, Aug 07, 2020 at 12:41:00PM -0700, Nick Desaulniers wrote:
> > A recent change to a default value of configuration variable
> > (ENABLE_X86_RELAX_RELOCATIONS OFF -> ON) in LLVM now causes Clang's
> > integrated assembler to emit R_X86_64_GOTPCRELX/R_X86_64_REX_GOTPCRELX
> > relocations. LLD will relax instructions with these relocations based on
> > whether the image is being linked as position independent or not.  When
> > not, then LLD will relax these instructions to use absolute addressing
> > mode (R_RELAX_GOT_PC_NOPIC). This causes kernels built with Clang
> > and linked with LLD to fail to boot.
>
> It could also cause kernels compiled with gcc and linked with LLD to
> fail in the same way, no? The gcc/gas combination will generate the
> relaxed relocations from I think gas-2.26 onward. Although the only
> troublesome symbol in the case of gcc/gas is trampoline_32bit_src,
> referenced from pgtable_64.c (gcc doesn't use a GOTPC reloc for _pgtable
> etc).

Thanks for taking a look, and the feedback. I appreciate it!

$ gcc --version | head -n 1
gcc (Debian 9.3.0-11) 9.3.0
$ make -j71 clean defconfig bzImage
$ llvm-readelf -r arch/x86/boot/compressed/*.o | grep -e
R_X86_64_GOTPCRELX -e R_X86_64_REX_GOTPCRELX
0000000000000114  000000120000002a R_X86_64_REX_GOTPCRELX
0000000000000000 trampoline_32bit_src - 4
$ llvm-readelf -r arch/x86/boot/compressed/vmlinux | grep -e
R_X86_64_GOTPCRELX -e R_X86_64_REX_GOTPCRELX
$

So it looks like yes.  I guess then we'd need to add a check for
CONFIG_LD_IS_LLD and CONFIG_CC_IS_GCC and binutils version is 2.26+?
I don't mind adding support for that combination, but I'd like to skip
it in this patch for the sake of backporting something small to stable
to get our CI green ASAP, since CONFIG_LD_IS_LLD probably doesn't
exist for those stable branches, which will complicate the backport of
such a patch.  So I'd do it in a follow up patch if we're cool with
that?

> I'm a bit surprised you were able to boot with just _pgtable fixed
> (looking at the CBL issue), there are quite a few more GOTPC relocs with
> clang -- maybe LLD isn't doing all the optimizations it could yet.

I am, too.  I didn't specify which symbol was problematic or put this
flag on just one object file, because it's likely that there's an
issue with multiple symbols in multiple object files, though it's just
_pgtable that causes observable boot failures.

> This potential issue was mentioned [0] in one of the earlier threads
> (see last paragraph).
>
> [0] https://lore.kernel.org/lkml/20200526191411.GA2380966@xxxxxxxxxxxxxxxxxx/

Oh, indeed.

> > Also, the LLVM commit notes that these relocation types aren't supported
> > until binutils 2.26. Since we support binutils 2.23+, avoid the
> > relocations regardless of linker.
>
> Note that the GNU assembler won't support the option to disable the
> relaxations until 2.26, when they were added.
>
> However, it turns out that clang always uses the integrated assembler
> for the decompressor (and the EFI stub) because the no-integrated-as
> option gets dropped when building these pieces, due to redefinition of
> KBUILD_CFLAGS. You might want to mention this in the commit log or a

That's why I was careful to note in the commit message that it was
Clang's integrated assembler (assembler job) vs Clang (compiler job)
itself that was producing these.  May I add precisely:

```
Note that the GNU assembler won't support the option to disable the
relaxations until 2.26, when they were added.

However, it turns out that clang always uses the integrated assembler
for the decompressor (and the EFI stub) because the no-integrated-as
option gets dropped when building these pieces, due to redefinition of
KBUILD_CFLAGS.
```
with your suggested-by tag for a v2?

> comment to explain why using the option unconditionally is safe. It
> might need to be made conditional if the CFLAGS ever gets fixed to
> maintain no-integrated-as.
>
> Thanks.



-- 
Thanks,
~Nick Desaulniers