From 6d01f28bc08814abc7b8f8973cc71195537f4c80 Mon Sep 17 00:00:00 2001 From: Josh Poimboeuf Date: Thu, 28 Jul 2016 19:14:58 -0500 Subject: [PATCH] objtool: Support new GCC 6 switch jump table pattern This fixes some false positive objtool warnings seen with gcc 6.1.1: kernel/trace/ring_buffer.o: warning: objtool: ring_buffer_read_page()+0x36c: sibling call from callable instruction with changed frame pointer arch/x86/kernel/reboot.o: warning: objtool: native_machine_emergency_restart()+0x139: sibling call from callable instruction with changed frame pointer lib/xz/xz_dec_stream.o: warning: objtool: xz_dec_run()+0xc2: sibling call from callable instruction with changed frame pointer With GCC 6, a new code pattern is sometimes used to access a switch statement jump table in .rodata, which objtool doesn't yet recognize: mov [rodata addr],%reg1 ... some instructions ... jmpq *(%reg1,%reg2,8) Add support for detecting that pattern. The detection code is rather crude, but it's still effective at weeding out false positives and catching real warnings. It can be refined later once objtool starts reading DWARF CFI. Reported-by: Linus Torvalds Signed-off-by: Josh Poimboeuf Cc: Andy Lutomirski Cc: Borislav Petkov Cc: Brian Gerst Cc: Denys Vlasenko Cc: H. Peter Anvin Cc: Peter Zijlstra Cc: Thomas Gleixner Link: http://lkml.kernel.org/r/b8c9503b4ad8c8a827cc5400db4c1b40a3ea07bc.1469751119.git.jpoimboe@redhat.com Signed-off-by: Ingo Molnar --- tools/objtool/builtin-check.c | 140 +++++++++++++++++++++------------- 1 file changed, 88 insertions(+), 52 deletions(-) diff --git a/tools/objtool/builtin-check.c b/tools/objtool/builtin-check.c index 17fa7fc34fdf..bd09d0effef8 100644 --- a/tools/objtool/builtin-check.c +++ b/tools/objtool/builtin-check.c @@ -107,6 +107,12 @@ static struct instruction *next_insn_same_sec(struct objtool_file *file, insn->offset < func->offset + func->len; \ insn = list_next_entry(insn, list)) +#define func_for_each_insn_continue_reverse(file, func, insn) \ + for (insn = list_prev_entry(insn, list); \ + &insn->list != &file->insn_list && \ + insn->sec == func->sec && insn->offset >= func->offset; \ + insn = list_prev_entry(insn, list)) + #define sec_for_each_insn_from(file, insn) \ for (; insn; insn = next_insn_same_sec(file, insn)) @@ -664,65 +670,95 @@ static int add_switch_table(struct objtool_file *file, struct symbol *func, return 0; } -static int add_func_switch_tables(struct objtool_file *file, - struct symbol *func) +/* + * find_switch_table() - Given a dynamic jump, find the switch jump table in + * .rodata associated with it. + * + * There are 3 basic patterns: + * + * 1. jmpq *[rodata addr](,%reg,8) + * + * This is the most common case by far. It jumps to an address in a simple + * jump table which is stored in .rodata. + * + * 2. jmpq *[rodata addr](%rip) + * + * This is caused by a rare GCC quirk, currently only seen in three driver + * functions in the kernel, only with certain obscure non-distro configs. + * + * As part of an optimization, GCC makes a copy of an existing switch jump + * table, modifies it, and then hard-codes the jump (albeit with an indirect + * jump) to use a single entry in the table. The rest of the jump table and + * some of its jump targets remain as dead code. + * + * In such a case we can just crudely ignore all unreachable instruction + * warnings for the entire object file. Ideally we would just ignore them + * for the function, but that would require redesigning the code quite a + * bit. And honestly that's just not worth doing: unreachable instruction + * warnings are of questionable value anyway, and this is such a rare issue. + * + * 3. mov [rodata addr],%reg1 + * ... some instructions ... + * jmpq *(%reg1,%reg2,8) + * + * This is a fairly uncommon pattern which is new for GCC 6. As of this + * writing, there are 11 occurrences of it in the allmodconfig kernel. + * + * TODO: Once we have DWARF CFI and smarter instruction decoding logic, + * ensure the same register is used in the mov and jump instructions. + */ +static struct rela *find_switch_table(struct objtool_file *file, + struct symbol *func, + struct instruction *insn) { - struct instruction *insn, *prev_jump; - struct rela *text_rela, *rodata_rela, *prev_rela = NULL; - int ret; + struct rela *text_rela, *rodata_rela; - prev_jump = NULL; + text_rela = find_rela_by_dest_range(insn->sec, insn->offset, insn->len); + if (text_rela && text_rela->sym == file->rodata->sym) { + /* case 1 */ + rodata_rela = find_rela_by_dest(file->rodata, + text_rela->addend); + if (rodata_rela) + return rodata_rela; - func_for_each_insn(file, func, insn) { - if (insn->type != INSN_JUMP_DYNAMIC) - continue; + /* case 2 */ + rodata_rela = find_rela_by_dest(file->rodata, + text_rela->addend + 4); + if (!rodata_rela) + return NULL; + file->ignore_unreachables = true; + return rodata_rela; + } + + /* case 3 */ + func_for_each_insn_continue_reverse(file, func, insn) { + if (insn->type == INSN_JUMP_UNCONDITIONAL || + insn->type == INSN_JUMP_DYNAMIC) + break; text_rela = find_rela_by_dest_range(insn->sec, insn->offset, insn->len); - if (!text_rela || text_rela->sym != file->rodata->sym) - continue; + if (text_rela && text_rela->sym == file->rodata->sym) + return find_rela_by_dest(file->rodata, + text_rela->addend); + } - /* common case: jmpq *[addr](,%rax,8) */ - rodata_rela = find_rela_by_dest(file->rodata, - text_rela->addend); + return NULL; +} - /* - * rare case: jmpq *[addr](%rip) - * - * This check is for a rare gcc quirk, currently only seen in - * three driver functions in the kernel, only with certain - * obscure non-distro configs. - * - * As part of an optimization, gcc makes a copy of an existing - * switch jump table, modifies it, and then hard-codes the jump - * (albeit with an indirect jump) to use a single entry in the - * table. The rest of the jump table and some of its jump - * targets remain as dead code. - * - * In such a case we can just crudely ignore all unreachable - * instruction warnings for the entire object file. Ideally we - * would just ignore them for the function, but that would - * require redesigning the code quite a bit. And honestly - * that's just not worth doing: unreachable instruction - * warnings are of questionable value anyway, and this is such - * a rare issue. - * - * kbuild reports: - * - https://lkml.kernel.org/r/201603231906.LWcVUpxm%25fengguang.wu@intel.com - * - https://lkml.kernel.org/r/201603271114.K9i45biy%25fengguang.wu@intel.com - * - https://lkml.kernel.org/r/201603291058.zuJ6ben1%25fengguang.wu@intel.com - * - * gcc bug: - * - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70604 - */ - if (!rodata_rela) { - rodata_rela = find_rela_by_dest(file->rodata, - text_rela->addend + 4); - if (rodata_rela) - file->ignore_unreachables = true; - } +static int add_func_switch_tables(struct objtool_file *file, + struct symbol *func) +{ + struct instruction *insn, *prev_jump = NULL; + struct rela *rela, *prev_rela = NULL; + int ret; - if (!rodata_rela) + func_for_each_insn(file, func, insn) { + if (insn->type != INSN_JUMP_DYNAMIC) + continue; + + rela = find_switch_table(file, func, insn); + if (!rela) continue; /* @@ -732,13 +768,13 @@ static int add_func_switch_tables(struct objtool_file *file, */ if (prev_jump) { ret = add_switch_table(file, func, prev_jump, prev_rela, - rodata_rela); + rela); if (ret) return ret; } prev_jump = insn; - prev_rela = rodata_rela; + prev_rela = rela; } if (prev_jump) { -- GitLab