Re: [PATCH v5 02/17] objtool: Better handle IRET

From: Miroslav Benes
Date: Fri Apr 17 2020 - 07:29:37 EST


On Thu, 16 Apr 2020, Peter Zijlstra wrote:

> 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) <peterz@xxxxxxxxxxxxx>
> ---
> 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(-)
>
> --- 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
> --- 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,
> --- a/tools/objtool/arch/x86/decode.c
> +++ b/tools/objtool/arch/x86/decode.c
> @@ -435,9 +435,19 @@ int arch_decode_instruction(struct elf *
> *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;
>
> @@ -483,7 +493,7 @@ int arch_decode_instruction(struct elf *
>
> *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);
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2080,15 +2080,14 @@ static int validate_return(struct symbol
> * 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)) {
> @@ -2141,16 +2140,6 @@ static int validate_branch(struct objtoo
> }
>
> 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;
> @@ -2243,6 +2232,20 @@ static int validate_branch(struct objtoo
>
> 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",

It looks really simple.

Have you tried Julien's proposal about removing INSN_STACK altogether,
move the x86 to arch/x86/ and call handle_insn_ops() unconditionally, or
have you just postponed it? As I said, I think it could be better in the
long term, but the above looks good for now as well.

Miroslav