lkml.org 
[lkml]   [2020]   [Apr]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v5 02/17] objtool: Better handle IRET
From
Date

On 4/16/20 1:47 PM, 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@infradead.org>
> ---
> 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;

Do we need to update the stack state for normal IRET? This wasn't done before.
So shouldn't this better be:

case INSN_EXCEPTION_RETURN:
if (!func)
return 0;

if (handle_insn_ops(insn, &state))
return 1;

break;

> +
> + /*
> + * 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;

Is it worth checking that func->name is effectively "sync_core"?

alex.

> +
> + return 0;
> +
> case INSN_CONTEXT_SWITCH:
> if (func && (!next_insn || !next_insn->hint)) {
> WARN_FUNC("unsupported instruction in callable function",
>
>

\
 
 \ /
  Last update: 2020-04-17 19:34    [W:0.207 / U:0.700 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site