| Message ID | 20241010122801.1321976-10-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:05PM +0200, Ard Biesheuvel wrote: > @@ -1394,8 +1396,12 @@ static struct reloc *insn_reloc(struct objtool_file *file, struct instruction *i > if (!file) > return NULL; > > - reloc = find_reloc_by_dest_range(file->elf, insn->sec, > - insn->offset, insn->len); > + do { > + /* Skip any R_*_NONE relocations */ > + reloc = find_reloc_by_dest_range(file->elf, insn->sec, > + offset++, len--); > + } while (len && reloc && reloc_type(reloc) == 0); s/0/R_NONE/
On Thu, Oct 10, 2024 at 02:28:05PM +0200, Ard Biesheuvel wrote: > +++ b/tools/objtool/arch/x86/special.c > @@ -115,30 +115,51 @@ struct reloc *arch_find_switch_table(struct objtool_file *file, > struct reloc *text_reloc, *rodata_reloc; > struct section *table_sec; > unsigned long table_offset; > + struct symbol *sym; > > /* look for a relocation which references .rodata */ > text_reloc = find_reloc_by_dest_range(file->elf, insn->sec, > insn->offset, insn->len); Hm, we can probably put insn_reloc() in check.h and use that here to take advantage of its caching for the no_reloc case. > + switch (text_reloc->sym->type) { > + case STT_OBJECT: > + sym = text_reloc->sym; > + break; > + case STT_SECTION: ^ extra whitespace
On Thu, 10 Oct 2024 at 22:15, Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > On Thu, Oct 10, 2024 at 02:28:05PM +0200, Ard Biesheuvel wrote: > > +++ b/tools/objtool/arch/x86/special.c > > @@ -115,30 +115,51 @@ struct reloc *arch_find_switch_table(struct objtool_file *file, > > struct reloc *text_reloc, *rodata_reloc; > > struct section *table_sec; > > unsigned long table_offset; > > + struct symbol *sym; > > > > /* look for a relocation which references .rodata */ > > text_reloc = find_reloc_by_dest_range(file->elf, insn->sec, > > insn->offset, insn->len); > > Hm, we can probably put insn_reloc() in check.h and use that here to > take advantage of its caching for the no_reloc case. > insn_reloc() filters out R_*_NONE relocations, for the reasons pointed out in the commit log.
On Fri, Oct 11, 2024 at 08:29:48AM +0200, Ard Biesheuvel wrote: > On Thu, 10 Oct 2024 at 22:15, Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > > On Thu, Oct 10, 2024 at 02:28:05PM +0200, Ard Biesheuvel wrote: > > > +++ b/tools/objtool/arch/x86/special.c > > > @@ -115,30 +115,51 @@ struct reloc *arch_find_switch_table(struct objtool_file *file, > > > struct reloc *text_reloc, *rodata_reloc; > > > struct section *table_sec; > > > unsigned long table_offset; > > > + struct symbol *sym; > > > > > > /* look for a relocation which references .rodata */ > > > text_reloc = find_reloc_by_dest_range(file->elf, insn->sec, > > > insn->offset, insn->len); > > > > Hm, we can probably put insn_reloc() in check.h and use that here to > > take advantage of its caching for the no_reloc case. > > > > insn_reloc() filters out R_*_NONE relocations, for the reasons pointed > out in the commit log. True, unless this becomes the fallback code for the generic "no annotations" case as I suggested elsewhere. Regardless we should be sure to write and read insn->no_reloc where we can to minimize duplicate reloc lookups.
diff --git a/tools/objtool/arch/x86/special.c b/tools/objtool/arch/x86/special.c index f8fb67636384..67c20623d7f7 100644 --- a/tools/objtool/arch/x86/special.c +++ b/tools/objtool/arch/x86/special.c @@ -115,30 +115,51 @@ struct reloc *arch_find_switch_table(struct objtool_file *file, struct reloc *text_reloc, *rodata_reloc; struct section *table_sec; unsigned long table_offset; + struct symbol *sym; /* look for a relocation which references .rodata */ text_reloc = find_reloc_by_dest_range(file->elf, insn->sec, insn->offset, insn->len); - if (!text_reloc || text_reloc->sym->type != STT_SECTION || - !text_reloc->sym->sec->rodata) + if (!text_reloc || !text_reloc->sym->sec->rodata) return NULL; - table_offset = reloc_addend(text_reloc); + /* + * If the indirect jump instruction itself is annotated with a + * R_X86_64_NONE relocation, it should point to the jump table + * in .rodata. In this case, the ELF symbol will give us the + * size of the table. Ignore other occurrences of R_X86_64_NONE. + */ + if (reloc_type(text_reloc) == R_X86_64_NONE && + insn->type != INSN_JUMP_DYNAMIC) + return NULL; + + table_offset = text_reloc->sym->offset + reloc_addend(text_reloc); table_sec = text_reloc->sym->sec; if (reloc_type(text_reloc) == R_X86_64_PC32) table_offset += 4; + switch (text_reloc->sym->type) { + case STT_OBJECT: + sym = text_reloc->sym; + break; + case STT_SECTION: + sym = find_symbol_containing(table_sec, table_offset); + break; + default: + return NULL; + } + /* * Make sure the .rodata address isn't associated with a - * symbol. GCC jump tables are anonymous data. + * symbol. Unannotated GCC jump tables are anonymous data. * * Also support C jump tables which are in the same format as * switch jump tables. For objtool to recognize them, they * need to be placed in the C_JUMP_TABLE_SECTION section. They * have symbols associated with them. */ - if (find_symbol_containing(table_sec, table_offset) && + if (reloc_type(text_reloc) != R_X86_64_NONE && sym && strcmp(table_sec->name, C_JUMP_TABLE_SECTION)) return NULL; @@ -151,6 +172,6 @@ struct reloc *arch_find_switch_table(struct objtool_file *file, if (!rodata_reloc) return NULL; - *table_size = 0; + *table_size = sym ? sym->len : 0; return rodata_reloc; } diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 5f711ac5b43d..6521c82880f0 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1386,6 +1386,8 @@ __weak const char *arch_nop_fentry_call(int len) static struct reloc *insn_reloc(struct objtool_file *file, struct instruction *insn) { + unsigned long offset = insn->offset; + unsigned int len = insn->len; struct reloc *reloc; if (insn->no_reloc) @@ -1394,8 +1396,12 @@ static struct reloc *insn_reloc(struct objtool_file *file, struct instruction *i if (!file) return NULL; - reloc = find_reloc_by_dest_range(file->elf, insn->sec, - insn->offset, insn->len); + do { + /* Skip any R_*_NONE relocations */ + reloc = find_reloc_by_dest_range(file->elf, insn->sec, + offset++, len--); + } while (len && reloc && reloc_type(reloc) == 0); + if (!reloc) { insn->no_reloc = 1; return NULL;