Messages in this thread | | | Subject | Re: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET | From | Julien Thierry <> | Date | Thu, 2 Apr 2020 09:29:20 +0100 |
| |
On 4/2/20 9:17 AM, Peter Zijlstra wrote: > On Thu, Apr 02, 2020 at 09:50:36AM +0200, Peter Zijlstra wrote: >> On Thu, Apr 02, 2020 at 07:41:46AM +0100, Julien Thierry wrote: > >>> Also, instead of adding a special "arch_exception_frame_size", I could >>> suggest: >>> - Picking this patch [1] from a completely arbitrary source >>> - Getting rid of INSN_STACK type, any instruction could then include stack >>> ops on top of their existing semantics, they can just have an empty list if >>> they don't touch SP/BP >>> - x86 decoder adds a stack_op to the iret to modify the stack pointer by the >>> right amount >> >> That's not the worst idea, lemme try that. > > Something like so then? >
Yes, you could even remove INSN_STACK from insn_type and just always call handle_insn_ops() before the switch statement on insn->type. If the list is empty it does nothing. This way you wouldn't need to call it for the INSN_EXCEPTION_RETURN case, and any type of instructions could use stack_ops.
And the other suggestion is my other email was that you don't even need to add INSN_EXCEPTION_RETURN. You can keep IRET as INSN_CONTEXT_SWITCH by default and x86 decoder lookups the symbol conaining an iret. If it's a function symbol, it can just set the type to INSN_OTHER so that it caries on to the next instruction after having handled the stack_op.
And everything fits under tools/objtool/arch/x86 :) .
Or is it too far-fetch'd?
> --- a/arch/x86/include/asm/processor.h > +++ b/arch/x86/include/asm/processor.h > @@ -738,7 +738,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" > @@ -748,7 +747,6 @@ static inline void sync_core(void) > "pushq %q0\n\t" > "pushq $1f\n\t" > "iretq\n\t" > - UNWIND_HINT_RESTORE > "1:" > : "=&r" (tmp), ASM_CALL_CONSTRAINT : : "cc", "memory"); > #endif > --- 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, > --- a/tools/objtool/arch/x86/decode.c > +++ b/tools/objtool/arch/x86/decode.c > @@ -435,9 +435,19 @@ int arch_decode_instruction(struct elf * > *type = INSN_RETURN; > break; > > + case 0xcf: /* iret */ > + *type = INSN_EXCEPTION_RETURN; > + > + /* add $40, %rsp */ > + op->src.type = OP_SRC_ADD; > + op->src.reg = CFI_SP; > + op->src.offset = 5*8; > + op->dest.type = OP_DEST_REG; > + op->dest.reg = CFI_SP; > + break; > + > case 0xca: /* retf */ > case 0xcb: /* retf */ > - case 0xcf: /* iret */ > *type = INSN_CONTEXT_SWITCH; > break; > > @@ -483,7 +493,7 @@ int arch_decode_instruction(struct elf * > > *immediate = insn.immediate.nbytes ? insn.immediate.value : 0; > > - if (*type == INSN_STACK) > + if (*type == INSN_STACK || *type == INSN_EXCEPTION_RETURN) > list_add_tail(&op->list, ops_list); > else > free(op); > --- a/tools/objtool/check.c > +++ b/tools/objtool/check.c > @@ -2224,6 +2224,20 @@ static int validate_branch(struct objtoo > > break; > > + case INSN_EXCEPTION_RETURN: > + if (handle_insn_ops(insn, &state)) > + return 1; > + > + /* > + * This handles x86's sync_core() case, where we use an > + * IRET to self. All 'normal' IRET instructions are in > + * STT_NOTYPE entry symbols. > + */ > + if (func) > + break; > + > + return 0; > + > case INSN_CONTEXT_SWITCH: > if (func && (!next_insn || !next_insn->hint)) { > WARN_FUNC("unsupported instruction in callable function", >
Cheers,
-- Julien Thierry
| |