lkml.org 
[lkml]   [2020]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] objtool,ftrace: Implement UNWIND_HINT_RET_OFFSET
From
Date


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

\
 
 \ /
  Last update: 2020-04-02 10:30    [W:0.075 / U:0.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site