From b746046238bb99b8f703c79f6d95357428fb6476 Mon Sep 17 00:00:00 2001 From: Peter Zijlstra Date: Thu, 2 Apr 2020 10:15:51 +0200 Subject: [PATCH] objtool: Better handle IRET Teach objtool a little more about IRET so that we can avoid using the SAVE/RESTORE annotation. In particular, make the weird corner case in insn->restore go away. The purpose of that corner case is to deal with the fact that UNWIND_HINT_RESTORE lands on the instruction after IRET, but that instruction can end up being outside the basic block, consider: if (cond) sync_core() foo(); Then the hint will land on foo(), and we'll encounter the restore hint without ever having seen the save hint. By teaching objtool about the arch specific exception frame size, and assuming that any IRET in an STT_FUNC symbol is an exception frame sized POP, we can remove the use of save/restore hints for this code. Signed-off-by: Peter Zijlstra (Intel) Reviewed-by: Miroslav Benes Reviewed-by: Alexandre Chartre Acked-by: Josh Poimboeuf Link: https://lkml.kernel.org/r/20200416115118.631224674@infradead.org Signed-off-by: Ingo Molnar --- arch/x86/include/asm/processor.h | 2 -- tools/objtool/arch.h | 1 + tools/objtool/arch/x86/decode.c | 14 ++++++++++++-- tools/objtool/check.c | 29 ++++++++++++++++------------- 4 files changed, 29 insertions(+), 17 deletions(-) diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index 3bcf27caf6c9..3eeaaeb75638 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -727,7 +727,6 @@ static inline void sync_core(void) unsigned int tmp; asm volatile ( - UNWIND_HINT_SAVE "mov %%ss, %0\n\t" "pushq %q0\n\t" "pushq %%rsp\n\t" @@ -737,7 +736,6 @@ static inline void sync_core(void) "pushq %q0\n\t" "pushq $1f\n\t" "iretq\n\t" - UNWIND_HINT_RESTORE "1:" : "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory"); #endif diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h index f9883c431949..55396dfe0d07 100644 --- a/tools/objtool/arch.h +++ b/tools/objtool/arch.h @@ -19,6 +19,7 @@ enum insn_type { INSN_CALL, INSN_CALL_DYNAMIC, INSN_RETURN, + INSN_EXCEPTION_RETURN, INSN_CONTEXT_SWITCH, INSN_STACK, INSN_BUG, diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c index 199b4084a13c..32736383ead1 100644 --- a/tools/objtool/arch/x86/decode.c +++ b/tools/objtool/arch/x86/decode.c @@ -446,9 +446,19 @@ int arch_decode_instruction(struct elf *elf, struct section *sec, *type = INSN_RETURN; break; + case 0xcf: /* iret */ + *type = INSN_EXCEPTION_RETURN; + + /* add $40, %rsp */ + op->src.type = OP_SRC_ADD; + op->src.reg = CFI_SP; + op->src.offset = 5*8; + op->dest.type = OP_DEST_REG; + op->dest.reg = CFI_SP; + break; + case 0xca: /* retf */ case 0xcb: /* retf */ - case 0xcf: /* iret */ *type = INSN_CONTEXT_SWITCH; break; @@ -494,7 +504,7 @@ int arch_decode_instruction(struct elf *elf, struct section *sec, *immediate = insn.immediate.nbytes ? insn.immediate.value : 0; - if (*type == INSN_STACK) + if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN) list_add_tail(&op->list, ops_list); else free(op); diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 9e854fd128d4..781b3a3c2ba6 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -2065,15 +2065,14 @@ static int validate_return(struct symbol *func, struct instruction *insn, struct * 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 alternative *alt; - struct instruction *insn, *next_insn; + struct instruction *next_insn; struct section *sec; u8 visited; int ret; - insn = first; sec = insn->sec; if (insn->alt_group && list_empty(&insn->alts)) { @@ -2126,16 +2125,6 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, } 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; @@ -2228,6 +2217,20 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, break; + case INSN_EXCEPTION_RETURN: + if (handle_insn_ops(insn, &state)) + return 1; + + /* + * This handles x86's sync_core() case, where we use an + * IRET to self. All 'normal' IRET instructions are in + * STT_NOTYPE entry symbols. + */ + if (func) + break; + + return 0; + case INSN_CONTEXT_SWITCH: if (func && (!next_insn || !next_insn->hint)) { WARN_FUNC("unsupported instruction in callable function", -- 2.20.1