| Message ID | 20241010122801.1321976-8-ardb+git@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve objtool jump table handling | expand |
On Thu, Oct 10, 2024 at 02:28:03PM +0200, Ard Biesheuvel wrote: > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > index 3cb3e9b5ad0b..7f7981a93535 100644 > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -2101,6 +2101,8 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn, > { > struct symbol *pfunc = insn_func(insn)->pfunc; > struct reloc *table = insn_jump_table(insn); > + unsigned int rtype = reloc_type(table); > + bool pcrel = rtype == R_X86_64_PC32; R_DATA32 or R_TEXT32 please, the budding cross arch stuff has their own names for all that.
On Thu, 10 Oct 2024 at 15:26, Peter Zijlstra <peterz@infradead.org> wrote: > > On Thu, Oct 10, 2024 at 02:28:03PM +0200, Ard Biesheuvel wrote: > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > > index 3cb3e9b5ad0b..7f7981a93535 100644 > > --- a/tools/objtool/check.c > > +++ b/tools/objtool/check.c > > @@ -2101,6 +2101,8 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn, > > { > > struct symbol *pfunc = insn_func(insn)->pfunc; > > struct reloc *table = insn_jump_table(insn); > > + unsigned int rtype = reloc_type(table); > > + bool pcrel = rtype == R_X86_64_PC32; > > R_DATA32 or R_TEXT32 please, the budding cross arch stuff has their own > names for all that. > #define R_DATA32 R_X86_64_PC32 #define R_DATA64 R_X86_64_PC32 #define R_TEXT32 R_X86_64_PC32 #define R_TEXT64 R_X86_64_PC32 Clear as mud. I'd guess we need the '64' variant here, but I'm not sure which one to use for a .rodata relocation pointing to .text. Any hints?
On Thu, Oct 10, 2024 at 03:59:43PM +0200, Ard Biesheuvel wrote: > On Thu, 10 Oct 2024 at 15:26, Peter Zijlstra <peterz@infradead.org> wrote: > > > > On Thu, Oct 10, 2024 at 02:28:03PM +0200, Ard Biesheuvel wrote: > > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > > > index 3cb3e9b5ad0b..7f7981a93535 100644 > > > --- a/tools/objtool/check.c > > > +++ b/tools/objtool/check.c > > > @@ -2101,6 +2101,8 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn, > > > { > > > struct symbol *pfunc = insn_func(insn)->pfunc; > > > struct reloc *table = insn_jump_table(insn); > > > + unsigned int rtype = reloc_type(table); > > > + bool pcrel = rtype == R_X86_64_PC32; > > > > R_DATA32 or R_TEXT32 please, the budding cross arch stuff has their own > > names for all that. > > > > #define R_DATA32 R_X86_64_PC32 > #define R_DATA64 R_X86_64_PC32 > #define R_TEXT32 R_X86_64_PC32 > #define R_TEXT64 R_X86_64_PC32 > > Clear as mud. > > I'd guess we need the '64' variant here, but I'm not sure which one to > use for a .rodata relocation pointing to .text. Any hints? Josh? do R_{DATA,TEXT}64 want to be _PC64 ?
On Thu, Oct 10, 2024 at 04:07:45PM +0200, Peter Zijlstra wrote: > On Thu, Oct 10, 2024 at 03:59:43PM +0200, Ard Biesheuvel wrote: > > On Thu, 10 Oct 2024 at 15:26, Peter Zijlstra <peterz@infradead.org> wrote: > > > > > > On Thu, Oct 10, 2024 at 02:28:03PM +0200, Ard Biesheuvel wrote: > > > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c > > > > index 3cb3e9b5ad0b..7f7981a93535 100644 > > > > --- a/tools/objtool/check.c > > > > +++ b/tools/objtool/check.c > > > > @@ -2101,6 +2101,8 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn, > > > > { > > > > struct symbol *pfunc = insn_func(insn)->pfunc; > > > > struct reloc *table = insn_jump_table(insn); > > > > + unsigned int rtype = reloc_type(table); > > > > + bool pcrel = rtype == R_X86_64_PC32; > > > > > > R_DATA32 or R_TEXT32 please, the budding cross arch stuff has their own > > > names for all that. > > > > > > > #define R_DATA32 R_X86_64_PC32 > > #define R_DATA64 R_X86_64_PC32 > > #define R_TEXT32 R_X86_64_PC32 > > #define R_TEXT64 R_X86_64_PC32 > > > > Clear as mud. Yep... :-/ > > > > I'd guess we need the '64' variant here, but I'm not sure which one to > > use for a .rodata relocation pointing to .text. Any hints? This should be R_TEXT64. > Josh? do R_{DATA,TEXT}64 want to be _PC64 ? Actually, R_{DATA,TEXT}32 should be R_386_PC32.
diff --git a/tools/objtool/arch/x86/special.c b/tools/objtool/arch/x86/special.c index 4ea0f9815fda..415e4d035e53 100644 --- a/tools/objtool/arch/x86/special.c +++ b/tools/objtool/arch/x86/special.c @@ -150,13 +150,5 @@ struct reloc *arch_find_switch_table(struct objtool_file *file, if (!rodata_reloc) return NULL; - /* - * Use of RIP-relative switch jumps is quite rare, and - * indicates a rare GCC quirk/bug which can leave dead - * code behind. - */ - if (reloc_type(text_reloc) == R_X86_64_PC32) - file->ignore_unreachables = true; - return rodata_reloc; } diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 3cb3e9b5ad0b..7f7981a93535 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -2101,6 +2101,8 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn, { struct symbol *pfunc = insn_func(insn)->pfunc; struct reloc *table = insn_jump_table(insn); + unsigned int rtype = reloc_type(table); + bool pcrel = rtype == R_X86_64_PC32; struct instruction *dest_insn; unsigned int prev_offset = 0; struct reloc *reloc = table; @@ -2111,13 +2113,18 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn, * instruction. */ for_each_reloc_from(table->sec, reloc) { + unsigned long addend = reloc_addend(reloc); /* Check for the end of the table: */ if (reloc != table && reloc == next_table) break; + /* Each entry in the jump table should use the same relocation type */ + if (reloc_type(reloc) != rtype) + break; + /* Make sure the table entries are consecutive: */ - if (prev_offset && reloc_offset(reloc) != prev_offset + 8) + if (prev_offset && reloc_offset(reloc) != prev_offset + (pcrel ? 4 : 8)) break; /* Detect function pointers from contiguous objects: */ @@ -2125,7 +2132,15 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn, reloc_addend(reloc) == pfunc->offset) break; - dest_insn = find_insn(file, reloc->sym->sec, reloc_addend(reloc)); + /* + * Place-relative jump tables carry offsets relative to the + * start of the jump table, not to the entry itself. So correct + * the addend for the location of the entry in the table. + */ + if (pcrel) + addend -= reloc_offset(reloc) - reloc_offset(table); + + dest_insn = find_insn(file, reloc->sym->sec, addend); if (!dest_insn) break; @@ -2133,6 +2148,9 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn, if (!insn_func(dest_insn) || insn_func(dest_insn)->pfunc != pfunc) break; + if (pcrel) + reloc->sym_offset = addend; + alt = malloc(sizeof(*alt)); if (!alt) { WARN("malloc failed"); @@ -4536,7 +4554,7 @@ static int validate_ibt_data_reloc(struct objtool_file *file, struct instruction *dest; dest = find_insn(file, reloc->sym->sec, - reloc->sym->offset + reloc_addend(reloc)); + reloc->sym->offset + reloc_sym_offset(reloc)); if (!dest) return 0; diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h index d7e815c2fd15..f4a6307f4c08 100644 --- a/tools/objtool/include/objtool/elf.h +++ b/tools/objtool/include/objtool/elf.h @@ -78,6 +78,7 @@ struct reloc { struct section *sec; struct symbol *sym; struct reloc *sym_next_reloc; + s64 sym_offset; }; struct elf { @@ -251,6 +252,11 @@ static inline s64 reloc_addend(struct reloc *reloc) return __get_reloc_field(reloc, r_addend); } +static inline s64 reloc_sym_offset(struct reloc *reloc) +{ + return reloc->sym_offset ?: reloc_addend(reloc); +} + static inline void set_reloc_addend(struct elf *elf, struct reloc *reloc, s64 addend) { __set_reloc_field(reloc, r_addend, addend);