Re: [RFC][PATCH 3/3] x86/entry, ORC: Teach objtool/unwind_orc about stack irq swizzles
From: Peter Zijlstra
Date: Thu May 07 2020 - 13:38:19 EST
On Thu, May 07, 2020 at 06:10:23PM +0200, Peter Zijlstra wrote:
> Thomas would very much like objtool to understand and generate correct
> ORC unwind information for the minimal stack swizzle sequence:
>
> mov %rsp, (%[ts])
> mov %[ts], %rsp
> ...
> pop %rsp
>
> This sequence works for the fp and guess unwinders -- all they need is
> that top-of-stack link set up by the first instruction.
>
> The previous entry_64.S code worked with "UNWIND_HINT_REGS indirect=1"
> hints to inform the unwinder about the stack-swizzle, but because
> we've now already entered C, we can no longer point to a REGS. In
> fact, due to being in C we don't even have a reliable sp_offset to
> anything.
>
> None of the existing UNWIND_HINT() functionality is quite sufficient
> to generate the right thing, but SP_INDIRECT is still the closest, so
> extend it.
>
> When SP_INDIRECT is combined with .end=1 (which is otherwise unused,
> except for sp_reg == UNDEFINED):
>
> - change it from (sp+sp_offset) to (sp)+sp_offset
> - have objtool preserve sp_offset from the previous state
> - change "pop %rsp" handling to restore the CFI state from before the
> hint.
>
> NOTES:
>
> - We now have an instruction with stackops and a hint; make hint take
> precedence over stackops.
>
> - Due to the reverse search in "pop %rsp" we must
> fill_alternative_cfi() before validate_branch().
>
> - This all isn't really pretty, but it works and gets Thomas the code
> sequence he wants.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
Much simpler, also works.
---
--- a/arch/x86/include/asm/irq_stack.h
+++ b/arch/x86/include/asm/irq_stack.h
@@ -23,6 +23,7 @@ do { \
__this_cpu_add(irq_count, 1); \
asm volatile( \
"movq %%rsp, (%[ts]) \n" \
+ UNWIND_HINT_INDIRECT \
"movq %[ts], %%rsp \n" \
ASM_INSTR_BEGIN \
_asm " \n" \
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -95,6 +95,47 @@
UNWIND_HINT type=UNWIND_HINT_TYPE_RET_OFFSET sp_offset=\sp_offset
.endm
+#else
+
+#define UNWIND_HINT(sp_reg, sp_offset, type, end) \
+ "987: \n\t" \
+ ".pushsection .discard.unwind_hints\n\t" \
+ /* struct unwind_hint */ \
+ ".long 987b - .\n\t" \
+ ".short " __stringify(sp_offset) "\n\t" \
+ ".byte " __stringify(sp_reg) "\n\t" \
+ ".byte " __stringify(type) "\n\t" \
+ ".byte " __stringify(end) "\n\t" \
+ ".balign 4 \n\t" \
+ ".popsection\n\t"
+
+/*
+ * Stack swizzling vs objtool/ORC:
+ *
+ * The canonical way of swizzling stack is:
+ *
+ * 1: mov %%rsp, (%[ts])
+ * 2: mov %[ts], %%rsp
+ * ...
+ * 3: pop %%rsp
+ *
+ * Where:
+ *
+ * 1 - places a pointer to the previous stack at the top of the new stack;
+ * also see the unwinders.
+ *
+ * 2 - switches to the new stack, but to avoid hitting the CFA_UNDEFINED case,
+ * we need to tell objtool the stack pointer can be found at (%%rsp),
+ * UNWIND_HINT_INDIRECT does so.
+ *
+ * 3 - restores the previous stack by popping the value stored by 1 into %%rsp,
+ * ....
+ *
+ * See arch/x86/include/asm/irq_stack.h
+ */
+#define UNWIND_HINT_INDIRECT \
+ UNWIND_HINT(ORC_REG_SP_INDIRECT, 0, ORC_TYPE_CALL, 0)
+
#endif /* __ASSEMBLY__ */
#endif /* _ASM_X86_UNWIND_HINTS_H */
--- a/arch/x86/kernel/unwind_orc.c
+++ b/arch/x86/kernel/unwind_orc.c
@@ -435,12 +435,12 @@ bool unwind_next_frame(struct unwind_sta
break;
case ORC_REG_SP_INDIRECT:
- sp = state->sp + orc->sp_offset;
+ sp = state->sp;
indirect = true;
break;
case ORC_REG_BP_INDIRECT:
- sp = state->bp + orc->sp_offset;
+ sp = state->bp;
indirect = true;
break;
@@ -489,6 +489,8 @@ bool unwind_next_frame(struct unwind_sta
if (indirect) {
if (!deref_stack_reg(state, sp, &sp))
goto err;
+
+ sp += orc->sp_offset;
}
/* Find IP, SP and possibly regs: */
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1720,8 +1720,7 @@ static void restore_reg(struct cfi_state
* 41 5d pop %r13
* c3 retq
*/
-static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi,
- struct stack_op *op)
+static int update_cfi_state(struct instruction *insn, struct cfi_state *cfi, struct stack_op *op)
{
struct cfi_reg *cfa = &cfi->cfa;
struct cfi_reg *regs = cfi->regs;
@@ -1898,6 +1897,13 @@ static int update_cfi_state(struct instr
case OP_SRC_POP:
case OP_SRC_POPF:
+ if (op->dest.reg == CFI_SP && cfa->base == CFI_SP_INDIRECT) {
+
+ /* pop %rsp from a stack swizzle */
+ cfa->base = CFI_SP;
+ break;
+ }
+
if (!cfi->drap && op->dest.reg == cfa->base) {
/* pop %rbp */
@@ -2085,9 +2091,11 @@ static int handle_insn_ops(struct instru
return -1;
}
- res = update_cfi_state(insn, &state->cfi, op);
- if (res)
- return res;
+ if (!insn->hint) {
+ res = update_cfi_state(insn, &state->cfi, op);
+ if (res)
+ return res;
+ }
if (op->dest.type == OP_DEST_PUSHF) {
if (!state->uaccess_stack) {
@@ -2319,16 +2327,25 @@ static int validate_branch(struct objtoo
if (state.noinstr)
state.instr += insn->instr;
- if (insn->hint)
- state.cfi = insn->cfi;
- else
+ if (insn->hint) {
+ if (insn->cfi.cfa.base == CFI_SP_INDIRECT) {
+ state.cfi.cfa.base = CFI_SP_INDIRECT;
+ insn->cfi = state.cfi;
+ } else {
+ state.cfi = insn->cfi;
+ }
+ } else {
insn->cfi = state.cfi;
+ }
insn->visited |= visited;
if (!insn->ignore_alts && !list_empty(&insn->alts)) {
bool skip_orig = false;
+ if (insn->alt_group)
+ fill_alternative_cfi(file, insn);
+
list_for_each_entry(alt, &insn->alts, list) {
if (alt->skip_orig)
skip_orig = true;
@@ -2341,9 +2358,6 @@ static int validate_branch(struct objtoo
}
}
- if (insn->alt_group)
- fill_alternative_cfi(file, insn);
-
if (skip_orig)
return 0;
}
--- a/tools/objtool/orc_dump.c
+++ b/tools/objtool/orc_dump.c
@@ -50,9 +50,9 @@ static const char *orc_type_name(unsigne
static void print_reg(unsigned int reg, int offset)
{
if (reg == ORC_REG_BP_INDIRECT)
- printf("(bp%+d)", offset);
+ printf("(bp)%+d", offset);
else if (reg == ORC_REG_SP_INDIRECT)
- printf("(sp%+d)", offset);
+ printf("(sp)%+d", offset);
else if (reg == ORC_REG_UNDEFINED)
printf("(und)");
else