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) <pet...@infradead.org> > ---
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