Skip to content

Commit

Permalink
objtool: Better handle IRET
Browse files Browse the repository at this point in the history
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>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Reviewed-by: Alexandre Chartre <alexandre.chartre@oracle.com>
Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://lkml.kernel.org/r/20200416115118.631224674@infradead.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
  • Loading branch information
Peter Zijlstra authored and Ingo Molnar committed Apr 22, 2020
1 parent 65ea47d commit b746046
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 17 deletions.
2 changes: 0 additions & 2 deletions arch/x86/include/asm/processor.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions tools/objtool/arch.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ enum insn_type {
INSN_CALL,
INSN_CALL_DYNAMIC,
INSN_RETURN,
INSN_EXCEPTION_RETURN,
INSN_CONTEXT_SWITCH,
INSN_STACK,
INSN_BUG,
Expand Down
14 changes: 12 additions & 2 deletions tools/objtool/arch/x86/decode.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down
29 changes: 16 additions & 13 deletions tools/objtool/check.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit b746046

Please sign in to comment.