x86/speculation, objtool: Use absolute relocations for annotations

  .discard.retpoline_safe sections do not have the SHF_ALLOC flag.  These
sections referencing text sections' STT_SECTION symbols with PC-relative
relocations like R_386_PC32 [0] is conceptually not suitable.  Newer
LLD will report warnings for REL relocations even for relocatable links
[1].

    ld.lld: warning: vmlinux.a(drivers/i2c/busses/i2c-i801.o):(.discard.retpoline_safe+0x120): has non-ABS relocation R_386_PC32 against symbol ''

Switch to absolute relocations instead, which indicate link-time
addresses.  In a relocatable link, these addresses are also output
section offsets, used by checks in tools/objtool/check.c.  When linking
vmlinux, these .discard.* sections will be discarded, therefore it is
not a problem that R_X86_64_32 cannot represent a kernel address.

Alternatively, we could set the SHF_ALLOC flag for .discard.* sections,
but I think non-SHF_ALLOC for sections to be discarded makes more sense.

Note: if we decide to never support REL architectures (e.g. arm, i386),
we can utilize R_*_NONE relocations (.reloc ., BFD_RELOC_NONE, sym),
making .discard.* sections zero-sized.  That said, the section content
waste is 4 bytes per entry, much smaller than sizeof(Elf{32,64}_Rel).

[0] commit 1c0c1faf5692 ("objtool: Use relative pointers for annotations")

Link: https://github.com/ClangBuiltLinux/linux/issues/1937 [1]
Signed-off-by: Fangrui Song <maskray@google.com>
---
 arch/x86/include/asm/alternative.h   |  4 ++--
 arch/x86/include/asm/nospec-branch.h |  4 ++--
 include/linux/objtool.h              | 10 +++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)
  

On Tue, Sep 19, 2023 at 05:17:28PM -0700, Fangrui Song wrote:
> .discard.retpoline_safe sections do not have the SHF_ALLOC flag.  These
> sections referencing text sections' STT_SECTION symbols with PC-relative
> relocations like R_386_PC32 [0] is conceptually not suitable.  Newer
> LLD will report warnings for REL relocations even for relocatable links
> [1].
> 
>     ld.lld: warning: vmlinux.a(drivers/i2c/busses/i2c-i801.o):(.discard.retpoline_safe+0x120): has non-ABS relocation R_386_PC32 against symbol ''

What, why ?!? Please explain more.

> Switch to absolute relocations instead, which indicate link-time
> addresses.  In a relocatable link, these addresses are also output
> section offsets, used by checks in tools/objtool/check.c.  When linking
> vmlinux, these .discard.* sections will be discarded, therefore it is
> not a problem that R_X86_64_32 cannot represent a kernel address.
> 
> Alternatively, we could set the SHF_ALLOC flag for .discard.* sections,
> but I think non-SHF_ALLOC for sections to be discarded makes more sense.
> 
> Note: if we decide to never support REL architectures (e.g. arm, i386),

We have explicit support for REL (as opposed to RELA) architectures, so
I don't think we can do that.
  

On Thu, Sep 21, 2023 at 12:26 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Sep 19, 2023 at 05:17:28PM -0700, Fangrui Song wrote:
> > .discard.retpoline_safe sections do not have the SHF_ALLOC flag.  These
> > sections referencing text sections' STT_SECTION symbols with PC-relative
> > relocations like R_386_PC32 [0] is conceptually not suitable.  Newer
> > LLD will report warnings for REL relocations even for relocatable links
> > [1].
> >
> >     ld.lld: warning: vmlinux.a(drivers/i2c/busses/i2c-i801.o):(.discard.retpoline_safe+0x120): has non-ABS relocation R_386_PC32 against symbol ''
>
> What, why ?!? Please explain more.

This can be read as a pedantic warning from the linker.

A location relocated by an R_386_PC32 relocation in
.discard.retpoline_safe records an offset from the current location
(non-allocable) to an text symbol.
This offset is conceptually not suitable: in the ELF object file
format's model, the non-SHF_ALLOC section is not part of the memory
image, so
we cannot say that the offset from the non-memory thing to a text
symbol is a fixed value.

> > Switch to absolute relocations instead, which indicate link-time
> > addresses.  In a relocatable link, these addresses are also output
> > section offsets, used by checks in tools/objtool/check.c.  When linking
> > vmlinux, these .discard.* sections will be discarded, therefore it is
> > not a problem that R_X86_64_32 cannot represent a kernel address.
> >
> > Alternatively, we could set the SHF_ALLOC flag for .discard.* sections,
> > but I think non-SHF_ALLOC for sections to be discarded makes more sense.
> >
> > Note: if we decide to never support REL architectures (e.g. arm, i386),
>
> We have explicit support for REL (as opposed to RELA) architectures, so
> I don't think we can do that.
>
  

On Thu, Sep 21, 2023 at 12:58:13AM -0700, Fangrui Song wrote:
> On Thu, Sep 21, 2023 at 12:26 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Tue, Sep 19, 2023 at 05:17:28PM -0700, Fangrui Song wrote:
> > > .discard.retpoline_safe sections do not have the SHF_ALLOC flag.  These
> > > sections referencing text sections' STT_SECTION symbols with PC-relative
> > > relocations like R_386_PC32 [0] is conceptually not suitable.  Newer
> > > LLD will report warnings for REL relocations even for relocatable links
> > > [1].
> > >
> > >     ld.lld: warning: vmlinux.a(drivers/i2c/busses/i2c-i801.o):(.discard.retpoline_safe+0x120): has non-ABS relocation R_386_PC32 against symbol ''
> >
> > What, why ?!? Please explain more.
> 
> This can be read as a pedantic warning from the linker.
> 
> A location relocated by an R_386_PC32 relocation in
> .discard.retpoline_safe records an offset from the current location
> (non-allocable) to an text symbol.
> This offset is conceptually not suitable: in the ELF object file
> format's model, the non-SHF_ALLOC section is not part of the memory
> image, so
> we cannot say that the offset from the non-memory thing to a text
> symbol is a fixed value.

Bah, so why has this worked at all then? Clearly the linkers aren't very
strict about things.

Anyway, I think what we want is to just mark the section SHF_ALLOC. The
reason is that one of the plans we have is to collapse all the different
annotations into a single section and then have something like:

	struct objtoo_annotation {
		s32 location;
		u32 type;
	}

So that we can easily extend the annotations and don't need to add
yet-another-section-reader-function to objtool.

This is just one of the things we've not gotten around to yet. But as
is, we have:

	.discard.unreachable
	.discard.reachable
	.discard.func_stack_frame_non_standard
	.discard.ignore_alts
	.discard.unwind_hints
	.discard.noendbr
	.discard.retpoline_safe
	.discard.instr_end
	.discard.instr_begin
	.discard.validate_unret
	.discard.intra_function_calls

And with the exception of unwind_hints, they're all just trivial
location things.

The very last thing we need is yet more of that.

If we were to use absolute things, we get 12 byte entries and while that
probably wouldn't spell the end of the world, why make thing larger than
they have to be.

After all, its not like any of this actually survives the final link.
  

On Thu, Sep 21, 2023 at 8:35 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Sep 21, 2023 at 12:58:13AM -0700, Fangrui Song wrote:
> > On Thu, Sep 21, 2023 at 12:26 AM Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Tue, Sep 19, 2023 at 05:17:28PM -0700, Fangrui Song wrote:
> > > > .discard.retpoline_safe sections do not have the SHF_ALLOC flag.  These
> > > > sections referencing text sections' STT_SECTION symbols with PC-relative
> > > > relocations like R_386_PC32 [0] is conceptually not suitable.  Newer
> > > > LLD will report warnings for REL relocations even for relocatable links
> > > > [1].
> > > >
> > > >     ld.lld: warning: vmlinux.a(drivers/i2c/busses/i2c-i801.o):(.discard.retpoline_safe+0x120): has non-ABS relocation R_386_PC32 against symbol ''
> > >
> > > What, why ?!? Please explain more.
> >
> > This can be read as a pedantic warning from the linker.
> >
> > A location relocated by an R_386_PC32 relocation in
> > .discard.retpoline_safe records an offset from the current location
> > (non-allocable) to an text symbol.
> > This offset is conceptually not suitable: in the ELF object file
> > format's model, the non-SHF_ALLOC section is not part of the memory
> > image, so
> > we cannot say that the offset from the non-memory thing to a text
> > symbol is a fixed value.
>
> Bah, so why has this worked at all then? Clearly the linkers aren't very
> strict about things.

GNU ld isn't very strict, but LLD has had a warning for
non-relocatable links for a long time
(https://github.com/llvm/llvm-project/blob/main/lld/test/ELF/non-abs-reloc.s).
LLD just did not report warnings for relocatable links.

> Anyway, I think what we want is to just mark the section SHF_ALLOC. The
> reason is that one of the plans we have is to collapse all the different
> annotations into a single section and then have something like:
>
>         struct objtoo_annotation {
>                 s32 location;
>                 u32 type;
>         }
>
> So that we can easily extend the annotations and don't need to add
> yet-another-section-reader-function to objtool.
>
> This is just one of the things we've not gotten around to yet. But as
> is, we have:
>
>         .discard.unreachable
>         .discard.reachable
>         .discard.func_stack_frame_non_standard
>         .discard.ignore_alts
>         .discard.unwind_hints
>         .discard.noendbr
>         .discard.retpoline_safe
>         .discard.instr_end
>         .discard.instr_begin
>         .discard.validate_unret
>         .discard.intra_function_calls
>
> And with the exception of unwind_hints, they're all just trivial
> location things.
>
> The very last thing we need is yet more of that.

If these sections are guaranteed to be discarded (*(.discard.*) in
scripts/module.lds.S and include/asm-generic/vmlinux.lds.h),
using non-SHF_ALLOC sections isn't a bad choice.
The intention is actually clearer.

> If we were to use absolute things, we get 12 byte entries and while that
> probably wouldn't spell the end of the world, why make thing larger than
> they have to be.
>
> After all, its not like any of this actually survives the final link.

I do not see why absolute things need 12 byte entries.
We can freely use `.long .text.foo` even in ELFCLASS64 object files.
There is no risk of overflow (the ultimate link .text.foo may have an
address of 0xffffffff........) since the section will be discarded.

Referencing SHF_ALLOC sections with absolute relocations in
non-SHF_ALLOC sections has well-defined semantics, as used by .debug_*
sections.
  

On Thu, Sep 21, 2023 at 09:26:43AM -0700, Fangrui Song wrote:

> I do not see why absolute things need 12 byte entries.
> We can freely use `.long .text.foo` even in ELFCLASS64 object files.
> There is no risk of overflow (the ultimate link .text.foo may have an
> address of 0xffffffff........) since the section will be discarded.

And you're sure no toolchain is going to be clever and tell me that the
absolute relocation will want to be 8 bytes and does not fit in the 4
bytes allotted?

Because clearly that is something some clever assembler is going to
complain about any day now.
  

On Thu, Sep 21, 2023 at 10:19 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Sep 21, 2023 at 09:26:43AM -0700, Fangrui Song wrote:
>
> > I do not see why absolute things need 12 byte entries.
> > We can freely use `.long .text.foo` even in ELFCLASS64 object files.
> > There is no risk of overflow (the ultimate link .text.foo may have an
> > address of 0xffffffff........) since the section will be discarded.
>
> And you're sure no toolchain is going to be clever and tell me that the
> absolute relocation will want to be 8 bytes and does not fit in the 4
> bytes allotted?
> Because clearly that is something some clever assembler is going to
> complain about any day now.

Well, only if the clever assembler doesn't support 32-bit absolute
relocation for a 64-bit architecture.
I don't know such an architecture. In addition, as long as the
architecture intends to support DWARF32, it has to support 32-bit
absolute relocations for a 64-bit architecture.

Of course, I cannot predict new toolchains for new architectures from
doing insensible thing, but
DWARF32 support and other metadata section uses are pretty strong
arguments for them to add a 32-bit absolute relocation type.

Note: some .discard.* sections before commit 1c0c1faf5692 ("objtool:
Use relative pointers for annotations") used
absolute relocations.
  

On Thu, Sep 21, 2023 at 10:36:27AM -0700, Fangrui Song wrote:

> Well, only if the clever assembler doesn't support 32-bit absolute
> relocation for a 64-bit architecture.
> I don't know such an architecture. In addition, as long as the
> architecture intends to support DWARF32, it has to support 32-bit
> absolute relocations for a 64-bit architecture.

Ooh... my bad. For some reason I thought that absolute meant native word
size. But you already mentioned R_X86_64_32 (and I failed to check) and
that is indeed an absolute (S+A) relocation of 32bit (dword) size.

And apparently we also have R_X64_64_16 and R_X86_64_8, which would even
allow something like:

#define OBJTOOL_ANNOTATE(type)				\
	"999:\n\t"					\
	".pushsection .discard.objtool_annotate\n\t"	\
	".byte 999b\n\t"				\
	".byte " __stringify(type) "\n\t"		\
	".popsection\n\t"

And since we only read the relocation and don't care for the actual
result that would actually work just fine.

Anyway, thanks for bearing with me.

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
  

On Thu, Sep 21, 2023 at 09:22:53PM +0200, Peter Zijlstra wrote:
> On Thu, Sep 21, 2023 at 10:36:27AM -0700, Fangrui Song wrote:
> 
> > Well, only if the clever assembler doesn't support 32-bit absolute
> > relocation for a 64-bit architecture.
> > I don't know such an architecture. In addition, as long as the
> > architecture intends to support DWARF32, it has to support 32-bit
> > absolute relocations for a 64-bit architecture.
> 
> Ooh... my bad. For some reason I thought that absolute meant native word
> size. But you already mentioned R_X86_64_32 (and I failed to check) and
> that is indeed an absolute (S+A) relocation of 32bit (dword) size.
> 
> And apparently we also have R_X64_64_16 and R_X86_64_8, which would even
> allow something like:

Hurm, just checked PPC/PPC64 and ARM64 and they only do 16bit (and up)
absolute relocations, not the single byte form.

So if I want to keep this portable, I suppose I shouldn't go smaller.
  

On Thu, Sep 21, 2023 at 12:22 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Sep 21, 2023 at 10:36:27AM -0700, Fangrui Song wrote:
>
> > Well, only if the clever assembler doesn't support 32-bit absolute
> > relocation for a 64-bit architecture.
> > I don't know such an architecture. In addition, as long as the
> > architecture intends to support DWARF32, it has to support 32-bit
> > absolute relocations for a 64-bit architecture.
>
> Ooh... my bad. For some reason I thought that absolute meant native word
> size. But you already mentioned R_X86_64_32 (and I failed to check) and
> that is indeed an absolute (S+A) relocation of 32bit (dword) size.
>
> And apparently we also have R_X64_64_16 and R_X86_64_8, which would even
> allow something like:
>
> #define OBJTOOL_ANNOTATE(type)                          \
>         "999:\n\t"                                      \
>         ".pushsection .discard.objtool_annotate\n\t"    \
>         ".byte 999b\n\t"                                \
>         ".byte " __stringify(type) "\n\t"               \
>         ".popsection\n\t"
>
> And since we only read the relocation and don't care for the actual
> result that would actually work just fine.
>
> Anyway, thanks for bearing with me.
>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Thanks!  I assume that the patch can be picked up without me doing anything :)

Yes, x86 also supports 1-byte and 2-byte absolute relocation types,
which are missing from some other architectures.
If we want to optimize for RELA, we can use R_*_NONE by utilizing
`.reloc ., BFD_RELOC_NONE, .text.foo` (as mentioned in the commit
message).
(binutils 2.26 https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=740bdc67c057ee8012327420848eb134e1db4211;
I added the support to most LLVM targets in 2021).

However, this is just 4 byte per entry difference compared with the
large sizeof(Elf{32,64}_Rel), so not worth the trouble:)
  

On Thu, Sep 21, 2023 at 01:07:09PM -0700, Fangrui Song wrote:
> On Thu, Sep 21, 2023 at 12:22 PM Peter Zijlstra <peterz@infradead.org> wrote:

> > Anyway, thanks for bearing with me.
> >
> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> 
> Thanks!  I assume that the patch can be picked up without me doing anything :)

Yep, we'll get it sorted. Thanks!