R_X86_64_REX_GOTPCRELX emitted in a kernel module when compiling with -fsanitize-coverage=trace-pc-guard

[ramosian-glider]

I am trying to bring support for -fsanitize-coverage=trace-pc-guard to the Linux kernel (draft patchset at ramosian-glider/linux#7)

The module looks as follows:

#include <linux/module.h>
MODULE_LICENSE("GPL");

static int __init testmod_init(void) {
    return 0;
}

module_init(testmod_init);

In the IR, the compiler declares two variables to reference the beginning and the end of the __sancov_guards section, @__start___sancov_guards and @__stop___sancov_guards:

...
$sancov.module_ctor_trace_pc_guard = comdat any 
...
@__sancov_gen_ = private global [1 x i32] zeroinitializer, section "__sancov_guards", comdat($init_module), align 4
@__start___sancov_guards = extern_weak hidden global i32 
@__stop___sancov_guards = extern_weak hidden global i32 
@llvm.used = appending global [2 x ptr] [ptr @sancov.module_ctor_trace_pc_guard, ptr @asan.module_ctor], section "llvm.metadata"
@llvm.compiler.used = appending global [3 x ptr] [ptr @__UNIQUE_ID___addressable_init_module564, ptr @__UNIQUE_ID_license563, ptr @__sancov_gen_], section "llvm.metadata"
@___asan_gen_module = private constant [17 x i8] c"kernel/testmod.c\00", align 1
@llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 2, ptr @sancov.module_ctor_trace_pc_guard, ptr @sancov.module_ctor_trace_pc_guard }]
...
declare void @__sanitizer_cov_trace_pc_guard_init(ptr, ptr)

; Function Attrs: fn_ret_thunk_extern nounwind
define internal void @sancov.module_ctor_trace_pc_guard() #1 comdat {
  call void @__asan_before_dynamic_init(i64 ptrtoint (ptr @___asan_gen_module to i64))
  call void @__sanitizer_cov_trace_pc_guard_init(ptr @__start___sancov_guards, ptr @__stop___sancov_guards)
  call void @__asan_after_dynamic_init()
  ret void 
}

(full IR: testmod.txt)

For the parameters of __sanitizer_cov_trace_pc_guard_init() the backend generates two R_X86_64_REX_GOTPCRELX in the object file:

0000000000000000 <sancov.module_ctor_trace_pc_guard>:
   0:	f3 0f 1e fa          	endbr64
   4:	48 c7 c7 00 00 00 00 	mov    $0x0,%rdi
			7: R_X86_64_32S	.rodata
   b:	e8 00 00 00 00       	call   10 <sancov.module_ctor_trace_pc_guard+0x10>
			c: R_X86_64_PLT32	__asan_before_dynamic_init-0x4
  10:	48 8b 3d 00 00 00 00 	mov    0x0(%rip),%rdi        # 17 <sancov.module_ctor_trace_pc_guard+0x17>
			13: R_X86_64_REX_GOTPCRELX	__start___sancov_guards-0x4
  17:	48 8b 35 00 00 00 00 	mov    0x0(%rip),%rsi        # 1e <sancov.module_ctor_trace_pc_guard+0x1e>
			1a: R_X86_64_REX_GOTPCRELX	__stop___sancov_guards-0x4
  1e:	e8 00 00 00 00       	call   23 <sancov.module_ctor_trace_pc_guard+0x23>
			1f: R_X86_64_PLT32	__sanitizer_cov_trace_pc_guard_init-0x4
  23:	e8 00 00 00 00       	call   28 <sancov.module_ctor_trace_pc_guard+0x28>
			24: R_X86_64_PLT32	__asan_after_dynamic_init-0x4
  28:	2e e9 00 00 00 00    	cs jmp 2e <init_module+0x1e>
			2a: R_X86_64_PLT32	__x86_return_thunk-0x4

, which remain in the resulting kernel module, making it unusable, as the kernel cannot process this relocation type:

$ objdump -r kernel/testmod.ko | grep PCREL
0000000000000013 R_X86_64_REX_GOTPCRELX  __start___sancov_guards-0x0000000000000004
000000000000001a R_X86_64_REX_GOTPCRELX  __stop___sancov_guards-0x0000000000000004

I am wondering if it is correct to declare the __start and __stop variables as extern_weak hidden global for the modules. Given that we want __sanitizer_cov_trace_pc_guard_init() in a module to be called for the contents of the __sancov_guards section of that module, not the vmlinux binary, shouldn't these variables be dso_local, or something alike?

[ramosian-glider]

@vitalybuka @dvyukov, @MaskRay @rnk for vis.

[rnk]

I found -Wa,-mrelax-relocations=yes/no which appears to control this. If you pass "no", you'd get the vanilla GOT relocation. Can the kernel handle that, or does it really need the dso_local PCREL LEA access pattern?

[MaskRay]

R_X86_64_REX_GOTPCRELX is R_X86_64_GOTPCREL with extra GOT optimization semantics (https://maskray.me/blog/2021-08-29-all-about-global-offset-table#got-optimization). If we ignore the optimization, then it can be handled the same way as R_X86_64_GOTPCREL . It should be straightforward to support R_X86_64_REX_GOTPCRELX in the kernel

% rg R_X86_64_GOTPCREL
tools/objtool/arch/x86/decode.c
95:     case R_X86_64_GOTPCREL:

arch/x86/include/asm/elf.h
57:#define R_X86_64_GOTPCREL    9       /* 32 bit signed pc relative

arch/x86/um/asm/elf.h
113:#define R_X86_64_GOTPCREL   9       /* 32 bit signed pc relative

arch/x86/tools/relocs.c
223:            REL_TYPE(R_X86_64_GOTPCREL),

[ramosian-glider]

As far as I can tell, the kernel cannot handle vanilla GOT relocations either.

The vmlinux linker script explicitly prohibits nonempty .got sections. For modules there are no explicit restrictions, but the module relocation code doesn't support R_X86_64_GOTPCREL.

[ramosian-glider]

Based on the above, I assume -mcmodel=kernel should probably avoid GOT relocations, but I don't see this mode being documented anywhere.

[ramosian-glider]

For the following code (godbolt link: https://godbolt.org/z/T61Yhda5r)

extern __attribute__((weak, visibility("hidden"))) int __start___sancov_guards;

void foo(char *);

int bar(int num) {
    foo(__start___sancov_guards);
    return num * num;
}

Clang defines __start___sancov_guards as @__start___sancov_guards = extern_weak hidden global i32, align 4 (same as sancov instrumentation does). That variable then turns into a GOTPCREL relocation in the assembly code.

When the same code is compiled with -fno-PIE, that same variable is defined as @__start___sancov_guards = extern_weak dso_local hidden global i32, align 4.

Because the kernel is using -fno-PIE to compile its object files, I guess sancov instrumentation should be emitting an extern_weak (EDIT: dso_local) at least in the presence of this flag.

(-mcmodel=kernel doesn't really seem to affect the code generation in this case)

[MaskRay]

The GOT-generating relocation decision is tied to dso_local. I recall the assertion ASSERT(SIZEOF(.got) == 0, "Unexpected GOT entries detected!"), which appears exclusively in the Linux's x86 port.
For other architectures, there are multiple ways to end up with a .got section (like PPC64’s .toc, which is extremely hard to eliminate).
(For example, it’s generally better to use GOT for an undefined weak symbol, even though many -fno-pic ports generate a PC-relative/absolute relocation. If the symbols ends up undefined, and the kernel load address is something different, it would be annoying.)

It seems the kernel’s x86 port prefers a zero-sized .got, but it might be better to relax this constraint (at least for specific instrumentation setups) and include support for GOT relocations instead.
It’s likely not ideal for compiler maintenance, nor a good use of a compiler engineer’s time, to adjust instrumentation passes for these issues—when they could be sidestepped effortlessly by having the kernel support GOT relocations.

[ramosian-glider]

This is insightful, thanks! I will look discussing it with upstream kernel folks (cc @kees who added the x86 assertions about .got)

However I looked at my dummy module with two GOTPCREL relocations, and there's no .got section, so even adding support for R_X86_64_GOTPCREL in arch/x86/kernel/modules.c will be insufficient.

[ramosian-glider]

Correction: the dummy module above had two R_X86_64_REX_GOTPCRELX relocations, which do not require a .got section and can be relocated.

ramosian-glider/linux@dd1522a should take care of handling them.

[ramosian-glider]

@ardbiesheuvel FYI

[ardbiesheuvel]

The GOT-generating relocation decision is tied to dso_local. I recall the assertion ASSERT(SIZEOF(.got) == 0, "Unexpected GOT entries detected!"), which appears exclusively in the Linux's x86 port. For other architectures, there are multiple ways to end up with a .got section (like PPC64’s .toc, which is extremely hard to eliminate). (For example, it’s generally better to use GOT for an undefined weak symbol, even though many -fno-pic ports generate a PC-relative/absolute relocation. If the symbols ends up undefined, and the kernel load address is something different, it would be annoying.)

It seems the kernel’s x86 port prefers a zero-sized .got, but it might be better to relax this constraint (at least for specific instrumentation setups) and include support for GOT relocations instead. It’s likely not ideal for compiler maintenance, nor a good use of a compiler engineer’s time, to adjust instrumentation passes for these issues—when they could be sidestepped effortlessly by having the kernel support GOT relocations.

Whether or not the kernel prefers a zero-sized GOT is irrelevant in this case - vmlinux is a fully linked binary, and so any R_X86_64_REX_GOTPCRELX based references will be relaxed into RIP-relative LEA instructions.

This issue is about kernel modules. Kernel modules are not DSOs but partially linked objects, and all the logic to load them at runtime is based on static relocations. This means that the kernel will need to implement its own relaxation logic and/or GOT slot allocation logic in order to deal with GOTPCREL based relocations.

Implementing this only for a diagnostic feature that insists on using a GOT based reference in non-PIC code is not a great use of our time, and complicates the module loader unjustifiably.

Could someone explain why these references are weak to begin with? Does the sanitizer still work when those symbols are missing?

[ramosian-glider]

Could someone explain why these references are weak to begin with? Does the sanitizer still work when those symbols are missing?

They were made weak in https://reviews.llvm.org/D98903 to avoid errors in the presence of --gc-sections.

In fact the coverage implementation that I propose in https://groups.google.com/g/kasan-dev/c/LC83evZ5568 does not require the compiler to emit sancov.module_ctor_trace_pc_guard() at all, so we could probably introduce a flag to omit it.
But I am not convinced that restricting -fsanitize-coverage=trace-pc-guard in the kernel to only support the existing use case is the right thing to do.