Messages in this thread Patch in this message | | | Date | Mon, 30 Mar 2020 19:02:00 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH v4 01/13] objtool: Remove CFI save/restore special case |
| |
On Thu, Mar 26, 2020 at 08:00:01PM -0500, Josh Poimboeuf wrote: > On Thu, Mar 26, 2020 at 08:57:18PM +0100, Peter Zijlstra wrote: > > 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. > > It still seems complex to me. > > What do you think about this?
Perhaps, but I still don't really like it. After much thinking I have yet another proposal, see below.
> If we store save_insn in the state when > we see insn->save, the restore logic becomes a lot easier.
Yes, that does help.
> Then if we > get a restore without a save, we can just ignore the restore hint in > that path. Later, when we see the restore insn again from the save > path, we can then compare the insn state with the saved state to make > sure they match.
That basically removes the error.
> This assumes no crazy save/restore scenarios.
This.
> It also means that the > restore path has to be reachable from the save path, for which I had to > make a change to make IRETQ *not* a dead end if there's a restore hint > immediately after it.
Right, I did that too. But that then makes me wonder why you needed that change to validate_unwind_hints(), because if the restore hint makes it continue, you'll not have unreachable code after it.
--- Subject: objtool: Implement RET_TAIL hint
This replaces the SAVE/RESTORE hints with a RET_TAIL hint that applies to:
- regular RETURN and sibling calls (which are also function exists) it allows the stack-frame to be off by one word, ie. it allows a return-tail-call.
- EXCEPTION_RETURN (a new INSN_type that splits IRET out of CONTEXT_SWITCH) and here it denotes a return to self by having it consume arch_exception_frame_size bytes off the stack and continuing.
Apply this hint to ftrace_64.S and sync_core(), the two existing users of the SAVE/RESTORE hints.
For ftrace_64.S we split the return path and make sure the ftrace_epilogue call is seen as a sibling/tail-call turning it into it's own function.
By splitting the return path every instruction has a unique stack setup and ORC can generate correct unwinds (XXX check if/how the ftrace trampolines map into the ORC). Then employ the RET_TAIL hint to the tail-call exit that has the direct-call (orig_eax) return-tail-call on.
For sync_core() annotate the IRET with RET_TAIL to mark it as a control-flow NOP that consumes the exception frame.
XXX this adds a tail-call to ftrace_caller() XXX compile tested only
Possibly-signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> --- arch/x86/include/asm/orc_types.h | 3 +- arch/x86/include/asm/processor.h | 3 +- arch/x86/include/asm/unwind_hints.h | 12 ++---- arch/x86/kernel/ftrace.c | 12 +++++- arch/x86/kernel/ftrace_64.S | 27 +++++-------- tools/arch/x86/include/asm/orc_types.h | 3 +- tools/objtool/Makefile | 2 +- tools/objtool/arch.h | 3 ++ tools/objtool/arch/x86/decode.c | 5 ++- tools/objtool/check.c | 74 ++++++++++------------------------ tools/objtool/check.h | 3 +- 11 files changed, 59 insertions(+), 88 deletions(-)
diff --git a/arch/x86/include/asm/orc_types.h b/arch/x86/include/asm/orc_types.h index 6e060907c163..49536fdd29ce 100644 --- a/arch/x86/include/asm/orc_types.h +++ b/arch/x86/include/asm/orc_types.h @@ -58,8 +58,7 @@ #define ORC_TYPE_CALL 0 #define ORC_TYPE_REGS 1 #define ORC_TYPE_REGS_IRET 2 -#define UNWIND_HINT_TYPE_SAVE 3 -#define UNWIND_HINT_TYPE_RESTORE 4 +#define UNWIND_HINT_TYPE_RET_TAIL 3 #ifndef __ASSEMBLY__ /* diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index 94789db550df..cb09ee18cbab 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -728,7 +728,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,8 +736,8 @@ static inline void sync_core(void) "mov %%cs, %0\n\t" "pushq %q0\n\t" "pushq $1f\n\t" + UNWIND_HINT_RET_TAIL "iretq\n\t" - UNWIND_HINT_RESTORE "1:" : "=&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..64ec9c9a6674 100644 --- a/arch/x86/include/asm/unwind_hints.h +++ b/arch/x86/include/asm/unwind_hints.h @@ -86,12 +86,8 @@ UNWIND_HINT sp_offset=\sp_offset .endm -.macro UNWIND_HINT_SAVE - UNWIND_HINT type=UNWIND_HINT_TYPE_SAVE -.endm - -.macro UNWIND_HINT_RESTORE - UNWIND_HINT type=UNWIND_HINT_TYPE_RESTORE +.macro UNWIND_HINT_RET_TAIL + UNWIND_HINT type=UNWIND_HINT_TYPE_RET_TAIL .endm #else /* !__ASSEMBLY__ */ @@ -108,9 +104,7 @@ ".balign 4 \n\t" \ ".popsection\n\t" -#define UNWIND_HINT_SAVE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_SAVE, 0) - -#define UNWIND_HINT_RESTORE UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RESTORE, 0) +#define UNWIND_HINT_RET_TAIL UNWIND_HINT(0, 0, UNWIND_HINT_TYPE_RET_TAIL, 0) #endif /* __ASSEMBLY__ */ diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c index 37a0aeaf89e7..3fda2ee7bf71 100644 --- a/arch/x86/kernel/ftrace.c +++ b/arch/x86/kernel/ftrace.c @@ -282,7 +282,8 @@ static inline void tramp_free(void *tramp) { } /* Defined as markers to the end of the ftrace default trampolines */ extern void ftrace_regs_caller_end(void); -extern void ftrace_epilogue(void); +extern void ftrace_regs_caller_ret(void); +extern void ftrace_caller_end(void); extern void ftrace_caller_op_ptr(void); extern void ftrace_regs_caller_op_ptr(void); @@ -334,7 +335,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size) call_offset = (unsigned long)ftrace_regs_call; } else { start_offset = (unsigned long)ftrace_caller; - end_offset = (unsigned long)ftrace_epilogue; + end_offset = (unsigned long)ftrace_caller_end; op_offset = (unsigned long)ftrace_caller_op_ptr; call_offset = (unsigned long)ftrace_call; } @@ -366,6 +367,13 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size) if (WARN_ON(ret < 0)) goto fail; + if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) { + ip = ftrace_regs_caller_ret; + ret = probe_kernel_read(ip, (void *)retq, RET_SIZE); + if (WARN_ON(ret < 0)) + goto fail; + } + /* * The address of the ftrace_ops that is used for this trampoline * is stored at the end of the trampoline. This will be used to diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S index 369e61faacfe..28834d715817 100644 --- a/arch/x86/kernel/ftrace_64.S +++ b/arch/x86/kernel/ftrace_64.S @@ -157,8 +157,12 @@ SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL) * think twice before adding any new code or changing the * layout here. */ -SYM_INNER_LABEL(ftrace_epilogue, SYM_L_GLOBAL) +SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL) + jmp ftrace_epilogue +SYM_FUNC_END(ftrace_caller); + +SYM_FUNC_START(ftrace_epilogue) #ifdef CONFIG_FUNCTION_GRAPH_TRACER SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) jmp ftrace_stub @@ -170,14 +174,12 @@ SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) */ SYM_INNER_LABEL_ALIGN(ftrace_stub, SYM_L_WEAK) retq -SYM_FUNC_END(ftrace_caller) +SYM_FUNC_END(ftrace_epilogue) SYM_FUNC_START(ftrace_regs_caller) /* Save the current flags before any operations that can change them */ pushfq - UNWIND_HINT_SAVE - /* added 8 bytes to save flags */ save_mcount_regs 8 /* save_mcount_regs fills in first two parameters */ @@ -244,20 +246,14 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL) movq %rax, MCOUNT_REG_SIZE(%rsp) restore_mcount_regs 8 + /* Restore flags */ + popfq - jmp 2f +SYM_INNER_LABEL(ftrace_regs_caller_ret, SYM_L_GLOBAL); + UNWIND_HINT_RET_TAIL + jmp ftrace_epilogue 1: restore_mcount_regs - - -2: - /* - * The stack layout is nondetermistic here, depending on which path was - * taken. This confuses objtool and ORC, rightfully so. For now, - * pretend the stack always looks like the non-direct case. - */ - UNWIND_HINT_RESTORE - /* Restore flags */ popfq @@ -268,7 +264,6 @@ SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL) * to the return. */ SYM_INNER_LABEL(ftrace_regs_caller_end, SYM_L_GLOBAL) - jmp ftrace_epilogue SYM_FUNC_END(ftrace_regs_caller) diff --git a/tools/arch/x86/include/asm/orc_types.h b/tools/arch/x86/include/asm/orc_types.h index 6e060907c163..49536fdd29ce 100644 --- a/tools/arch/x86/include/asm/orc_types.h +++ b/tools/arch/x86/include/asm/orc_types.h @@ -58,8 +58,7 @@ #define ORC_TYPE_CALL 0 #define ORC_TYPE_REGS 1 #define ORC_TYPE_REGS_IRET 2 -#define UNWIND_HINT_TYPE_SAVE 3 -#define UNWIND_HINT_TYPE_RESTORE 4 +#define UNWIND_HINT_TYPE_RET_TAIL 3 #ifndef __ASSEMBLY__ /* diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile index ee08aeff30a1..ebbf92a8836f 100644 --- a/tools/objtool/Makefile +++ b/tools/objtool/Makefile @@ -31,7 +31,7 @@ INCLUDES := -I$(srctree)/tools/include \ -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \ -I$(srctree)/tools/arch/$(SRCARCH)/include WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed -CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS) +CFLAGS := -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -ggdb3 $(INCLUDES) $(LIBELF_FLAGS) LDFLAGS += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS) # Allow old libelf to be used: diff --git a/tools/objtool/arch.h b/tools/objtool/arch.h index ced3765c4f44..6d04a683f8cc 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, @@ -73,6 +74,8 @@ int arch_decode_instruction(struct elf *elf, struct section *sec, unsigned int *len, enum insn_type *type, unsigned long *immediate, struct stack_op *op); +static const int arch_exception_frame_size = 5*8; + bool arch_callee_saved_reg(unsigned char reg); #endif /* _ARCH_H */ diff --git a/tools/objtool/arch/x86/decode.c b/tools/objtool/arch/x86/decode.c index a62e032863a8..1f9dd97e4a50 100644 --- a/tools/objtool/arch/x86/decode.c +++ b/tools/objtool/arch/x86/decode.c @@ -431,10 +431,13 @@ int arch_decode_instruction(struct elf *elf, struct section *sec, case 0xca: /* retf */ case 0xcb: /* retf */ - case 0xcf: /* iret */ *type = INSN_CONTEXT_SWITCH; break; + case 0xcf: /* iret */ + *type = INSN_EXCEPTION_RETURN; + break; + case 0xe8: *type = INSN_CALL; break; diff --git a/tools/objtool/check.c b/tools/objtool/check.c index e637a4a38d2a..6eb9ae48c94f 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1259,13 +1259,8 @@ static int read_unwind_hints(struct objtool_file *file) cfa = &insn->state.cfa; - if (hint->type == UNWIND_HINT_TYPE_SAVE) { - insn->save = true; - continue; - - } else if (hint->type == UNWIND_HINT_TYPE_RESTORE) { - insn->restore = true; - insn->hint = true; + if (hint->type == UNWIND_HINT_TYPE_RET_TAIL) { + insn->ret_tail = true; continue; } @@ -1429,16 +1424,22 @@ static bool is_fentry_call(struct instruction *insn) return false; } -static bool has_modified_stack_frame(struct insn_state *state) +static bool has_modified_stack_frame(struct instruction *insn, struct insn_state *state) { int i; if (state->cfa.base != initial_func_cfi.cfa.base || - state->cfa.offset != initial_func_cfi.cfa.offset || - state->stack_size != initial_func_cfi.cfa.offset || state->drap) return true; + if (state->cfa.offset != initial_func_cfi.cfa.offset || + !(insn->ret_tail && state->cfa.offset == initial_func_cfi.cfa.offset + 8)) + + + if (state->stack_size != initial_func_cfi.cfa.offset && + !(insn->ret_tail && state->stack_size == initial_func_cfi.cfa.offset + 8)) + return true; + for (i = 0; i < CFI_NUM_REGS; i++) if (state->regs[i].base != initial_func_cfi.regs[i].base || state->regs[i].offset != initial_func_cfi.regs[i].offset) @@ -1984,7 +1985,7 @@ static int validate_call(struct instruction *insn, struct insn_state *state) static int validate_sibling_call(struct instruction *insn, struct insn_state *state) { - if (has_modified_stack_frame(state)) { + if (has_modified_stack_frame(insn, state)) { WARN_FUNC("sibling call from callable instruction with modified stack frame", insn->sec, insn->offset); return 1; @@ -2041,47 +2042,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, return 0; } - 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; - } - + if (insn->hint) state = insn->state; - - } else + else insn->state = state; insn->visited |= visited; @@ -2123,7 +2086,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, return 1; } - if (func && has_modified_stack_frame(&state)) { + if (func && has_modified_stack_frame(insn, &state)) { WARN_FUNC("return with modified stack frame", sec, insn->offset); return 1; @@ -2190,6 +2153,13 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, break; + case INSN_EXCEPTION_RETURN: + if (insn->ret_tail) { + state.stack_size -= arch_exception_frame_size; + break; + } + + /* fallthrough */ case INSN_CONTEXT_SWITCH: if (func && (!next_insn || !next_insn->hint)) { WARN_FUNC("unsupported instruction in callable function", diff --git a/tools/objtool/check.h b/tools/objtool/check.h index 6d875ca6fce0..138533ac23e2 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, ret_tail; bool retpoline_safe; u8 visited; struct symbol *call_dest;
| |