Messages in this thread Patch in this message | | | Date | Thu, 26 Mar 2020 20:57:18 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case |
| |
On Thu, Mar 26, 2020 at 04:49:38PM +0100, Peter Zijlstra wrote: > > The 'insn == first' check isn't ideal, but at least it works (I think?). > > It works, yes, for exactly this one case.
How's this? Ignore the ignore_cfi bits, that's a 'failed' experiment.
--- arch/x86/include/asm/orc_types.h | 2 + arch/x86/include/asm/processor.h | 6 +- arch/x86/include/asm/unwind_hints.h | 4 ++ tools/arch/x86/include/asm/orc_types.h | 2 + tools/objtool/check.c | 109 +++++++++++++++++++-------------- tools/objtool/check.h | 3 +- 6 files changed, 75 insertions(+), 51 deletions(-)
diff --git a/arch/x86/include/asm/orc_types.h b/arch/x86/include/asm/orc_types.h index 6e060907c163..82b5c685341a 100644 --- a/arch/x86/include/asm/orc_types.h +++ b/arch/x86/include/asm/orc_types.h @@ -60,6 +60,8 @@ #define ORC_TYPE_REGS_IRET 2 #define UNWIND_HINT_TYPE_SAVE 3 #define UNWIND_HINT_TYPE_RESTORE 4 +#define UNWIND_HINT_TYPE_IGNORE 5 +#define UNWIND_HINT_TYPE_IRET_CONT 6
#ifndef __ASSEMBLY__ /* diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index 94789db550df..45c74cbc0a83 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -728,8 +728,8 @@ static inline void sync_core(void) unsigned int tmp;
asm volatile ( - UNWIND_HINT_SAVE "mov %%ss, %0\n\t" + UNWIND_HINT_SAVE "pushq %q0\n\t" "pushq %%rsp\n\t" "addq $8, (%%rsp)\n\t" @@ -737,9 +737,9 @@ static inline void sync_core(void) "mov %%cs, %0\n\t" "pushq %q0\n\t" "pushq $1f\n\t" + UNWIND_HINT_IRET_CONT "iretq\n\t" - UNWIND_HINT_RESTORE - "1:" + "1:\n\t" : "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory"); #endif } diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h index f5e2eb12cb71..d8a07749c323 100644 --- a/arch/x86/include/asm/unwind_hints.h +++ b/arch/x86/include/asm/unwind_hints.h @@ -112,6 +112,10 @@
#define UNWIND_HINT_RESTORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RESTORE, 0)
+#define UNWIND_HINT_IGNORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_IGNORE, 0) + +#define UNWIND_HINT_IRET_CONT UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_IRET_CONT, 0) + #endif /* __ASSEMBLY__ */
#endif /* _ASM_X86_UNWIND_HINTS_H */ diff --git a/tools/arch/x86/include/asm/orc_types.h b/tools/arch/x86/include/asm/orc_types.h index 6e060907c163..82b5c685341a 100644 --- a/tools/arch/x86/include/asm/orc_types.h +++ b/tools/arch/x86/include/asm/orc_types.h @@ -60,6 +60,8 @@ #define ORC_TYPE_REGS_IRET 2 #define UNWIND_HINT_TYPE_SAVE 3 #define UNWIND_HINT_TYPE_RESTORE 4 +#define UNWIND_HINT_TYPE_IGNORE 5 +#define UNWIND_HINT_TYPE_IRET_CONT 6
#ifndef __ASSEMBLY__ /* diff --git a/tools/objtool/check.c b/tools/objtool/check.c index e637a4a38d2a..03bac6cb313c 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1259,14 +1259,25 @@ static int read_unwind_hints(struct objtool_file *file)
cfa = &insn->state.cfa;
- if (hint->type == UNWIND_HINT_TYPE_SAVE) { + switch (hint->type) { + case UNWIND_HINT_TYPE_SAVE: insn->save = true; continue;
- } else if (hint->type == UNWIND_HINT_TYPE_RESTORE) { + case UNWIND_HINT_TYPE_RESTORE: insn->restore = true; - insn->hint = true; continue; + + case UNWIND_HINT_TYPE_IGNORE: + insn->ignore_cfi = true; + continue; + + case UNWIND_HINT_TYPE_IRET_CONT: + insn->iret_cont = true; + continue; + + default: + break; }
insn->hint = true; @@ -1558,6 +1569,9 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state) struct cfi_reg *cfa = &state->cfa; struct cfi_reg *regs = state->regs;
+ if (insn->ignore_cfi) + return 0; + /* stack operations don't make sense with an undefined CFA */ if (cfa->base == CFI_UNDEFINED) { if (insn->func) { @@ -1993,6 +2007,37 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st return validate_call(insn, state); }
+static int insn_hint_restore(struct objtool_file *file, struct section *sec, + struct symbol *func, struct instruction *insn, + struct insn_state *state) +{ + struct instruction *save_insn, *i; + + i = insn; + save_insn = NULL; + func_for_each_insn_continue_reverse(file, func, i) { + if (i->save) { + save_insn = i; + break; + } + } + + if (!save_insn) { + WARN_FUNC("no corresponding CFI save for CFI restore", + sec, insn->offset); + return 1; + } + + if (!save_insn->visited) { + WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo", + sec, insn->offset); + return 1; + } + + *state = save_insn->state; + return 0; +} + /* * Follow the branch starting at the given instruction, and recursively follow * any other branches (jumps). Meanwhile, track the frame pointer state at @@ -2000,15 +2045,14 @@ static int validate_sibling_call(struct instruction *insn, struct insn_state *st * tools/objtool/Documentation/stack-validation.txt. */ static int validate_branch(struct objtool_file *file, struct symbol *func, - struct instruction *first, struct insn_state state) + struct instruction *insn, struct insn_state state) { + struct instruction *next_insn; struct alternative *alt; - struct instruction *insn, *next_insn; struct section *sec; u8 visited; int ret;
- insn = first; sec = insn->sec;
if (insn->alt_group && list_empty(&insn->alts)) { @@ -2034,7 +2078,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
visited = 1 << state.uaccess; if (insn->visited) { - if (!insn->hint && !insn_state_match(insn, &state)) + if ((!insn->hint && !insn->restore) && !insn_state_match(insn, &state)) return 1;
if (insn->visited & visited) @@ -2042,47 +2086,12 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, }
if (insn->hint) { - if (insn->restore) { - struct instruction *save_insn, *i; - - i = insn; - save_insn = NULL; - func_for_each_insn_continue_reverse(file, func, i) { - if (i->save) { - save_insn = i; - break; - } - } - - if (!save_insn) { - WARN_FUNC("no corresponding CFI save for CFI restore", - sec, insn->offset); - return 1; - } - - if (!save_insn->visited) { - /* - * Oops, no state to copy yet. - * Hopefully we can reach this - * instruction from another branch - * after the save insn has been - * visited. - */ - if (insn == first) - return 0; - - WARN_FUNC("objtool isn't smart enough to handle this CFI save/restore combo", - sec, insn->offset); - return 1; - } - - insn->state = save_insn->state; - } - state = insn->state; - - } else + } else { + if (insn->restore) + insn_hint_restore(file, sec, func, insn, &state); insn->state = state; + }
insn->visited |= visited;
@@ -2191,11 +2200,17 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, break;
case INSN_CONTEXT_SWITCH: - if (func && (!next_insn || !next_insn->hint)) { + if (insn->iret_cont) { + insn_hint_restore(file, sec, func, insn, &state); + break; + } + + if (func) { WARN_FUNC("unsupported instruction in callable function", sec, insn->offset); return 1; } + return 0;
case INSN_STACK: diff --git a/tools/objtool/check.h b/tools/objtool/check.h index 6d875ca6fce0..f2b6172e119b 100644 --- a/tools/objtool/check.h +++ b/tools/objtool/check.h @@ -33,7 +33,8 @@ struct instruction { unsigned int len; enum insn_type type; unsigned long immediate; - bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts; + bool alt_group, dead_end, ignore, ignore_alts; + bool hint, save, restore, ignore_cfi, iret_cont; bool retpoline_safe; u8 visited; struct symbol *call_dest;
| |