Messages in this thread | | | Subject | Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET | From | Julien Thierry <> | Date | Wed, 1 Apr 2020 16:43:35 +0100 |
| |
Hi Peter,
On 3/31/20 11:27 PM, Peter Zijlstra wrote: > On Tue, Mar 31, 2020 at 04:20:40PM -0500, Josh Poimboeuf wrote: >> On Tue, Mar 31, 2020 at 04:17:58PM -0500, Josh Poimboeuf wrote: >>>> I'm not against adding a second/separate hint for this. In fact, I >>>> almost considered teaching objtool how to interpret the whole IRET frame >>>> so that we can do it without hints. It's just that that's too much code >>>> for this one case. >>>> >>>> HINT_IRET_SELF ? >>> >>> Despite my earlier complaint about stack size knowledge, we could just >>> forget the hint and make "iretq in C code" equivalent to "reduce stack >>> size by arch_exception_stack_size()" and keep going. There's >>> file->c_file which tells you it's a C file. >> >> Or maybe "iretq in an STT_FUNC" is better since this pattern could >> presumably happen in a callable asm function. > > Like so then? > > --- > Subject: objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET > From: Peter Zijlstra <peterz@infradead.org> > Date: Tue, 31 Mar 2020 13:16:52 +0200 > > This replaces the SAVE/RESTORE hints with a RET_OFFSET hint that > applies to any instruction that terminates a function, like: RETURN > and sibling calls. It allows the stack-frame to be off by @sp_offset, > ie. it allows stuffing the return stack. > > 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. Then employ the RET_OFFSET hint to > the tail-call exit that has the direct-call (orig_eax) stuffed on the > return stack. > > For sync_core() we teach objtool that an IRET inside an STT_FUNC > simply consumes the exception stack and continues. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > arch/x86/include/asm/orc_types.h | 9 ++- > arch/x86/include/asm/processor.h | 2 > 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 | 9 ++- > tools/objtool/Makefile | 2 > tools/objtool/arch.h | 3 + > tools/objtool/arch/x86/decode.c | 5 +- > tools/objtool/check.c | 80 ++++++++++----------------------- > tools/objtool/check.h | 4 + > 11 files changed, 74 insertions(+), 91 deletions(-) >
[snip]
> --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -1246,13 +1246,8 @@ static int read_unwind_hints(struct objt > > 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_OFFSET) { > + insn->ret_offset = hint->sp_offset; > continue; > } > > @@ -1416,20 +1411,26 @@ static bool is_fentry_call(struct instru > 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) > { > + u8 ret_offset = insn->ret_offset; > 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) > + if (state->cfa.base != initial_func_cfi.cfa.base || state->drap) > + return true; > + > + if (state->cfa.offset != initial_func_cfi.cfa.offset && > + !(ret_offset && state->cfa.offset == initial_func_cfi.cfa.offset + ret_offset))
Isn't that the same thing as "state->cfa.offset != initial_func_cfi.cfa.offset + ret_offset" ?
> + return true; > + > + if (state->stack_size != initial_func_cfi.cfa.offset + ret_offset) > return true; > > - for (i = 0; i < CFI_NUM_REGS; i++) > + 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) > return true; > + } > > return false; > } > @@ -1971,7 +1972,7 @@ static int validate_call(struct instruct > > 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; > @@ -2000,7 +2001,7 @@ static int validate_return(struct symbol > return 1; > } > > - if (func && has_modified_stack_frame(state)) { > + if (func && has_modified_stack_frame(insn, state)) { > WARN_FUNC("return with modified stack frame", > insn->sec, insn->offset); > return 1; > @@ -2063,47 +2064,9 @@ static int validate_branch(struct objtoo > return 0; > } > > - if (insn->hint) { > - if (insn->restore) { > - struct instruction *save_insn, *i; > - > - i = insn; > - save_insn = NULL; > - sym_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; > @@ -2185,6 +2148,13 @@ static int validate_branch(struct objtoo > > break; > > + case INSN_EXCEPTION_RETURN: > + if (func) { > + state.stack_size -= arch_exception_frame_size; > + break;
Why break instead of returning? Shouldn't an exception return mark the end of a branch (whether inside or outside a function) ?
Here it seems it will continue to the next instruction which might have been unreachable.
> + } > + > + /* fallthrough */
What is the purpose of the fallthrough here? If the exception return was in a function, it carried on to the next instruction, so it won't use the WARN_FUNC(). So, if I'm looking at the right version of the code only the "return 0;" will be used. And, unless my previous comment is wrong, I'd argue that we should return both for func and !func.
> case INSN_CONTEXT_SWITCH: > if (func && (!next_insn || !next_insn->hint)) { > WARN_FUNC("unsupported instruction in callable function", > --- a/tools/objtool/check.h > +++ b/tools/objtool/check.h > @@ -33,9 +33,11 @@ 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; > bool retpoline_safe; > u8 visited; > + u8 ret_offset; > struct symbol *call_dest; > struct instruction *jump_dest; > struct instruction *first_jump_src; > >
Cheers,
-- Julien Thierry
| |