Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
From: Julien Thierry
Date: Wed Apr 01 2020 - 11:43:44 EST
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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
---
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