diff mbox series

[-tip,v8,05/13] x86/kprobes: Add UNWIND_HINT_FUNC on kretprobe_trampoline code

Message ID 162399996966.506599.810050095040575221.stgit@devnote2 (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series kprobes: Fix stacktrace with kretprobes on x86 | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Masami Hiramatsu (Google) June 18, 2021, 7:06 a.m. UTC 
From: Josh Poimboeuf <jpoimboe@redhat.com>

Add UNWIND_HINT_FUNC on kretporbe_trampoline code so that ORC
information is generated on the kretprobe_trampoline correctly.

Note that when the CONFIG_FRAME_POINTER=y, since the
kretprobe_trampoline skips updating frame pointer, the stack frame
of the kretprobe_trampoline seems non-standard. So this marks it
is STACK_FRAME_NON_STANDARD() and undefine UNWIND_HINT_FUNC.
Anyway, with the frame pointer, FP unwinder can unwind the stack
frame correctly without that hint.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Tested-by: Andrii Nakryik <andrii@kernel.org>
---
 Changes in v4:
  - Apply UNWIND_HINT_FUNC only if CONFIG_FRAME_POINTER=n.
---
 arch/x86/include/asm/unwind_hints.h |    5 +++++
 arch/x86/kernel/kprobes/core.c      |   17 +++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

Comments

Ingo Molnar July 5, 2021, 8:02 a.m. UTC | #1 
* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> From: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Add UNWIND_HINT_FUNC on kretporbe_trampoline code so that ORC
> information is generated on the kretprobe_trampoline correctly.

What is a 'kretporbe'?

> Note that when the CONFIG_FRAME_POINTER=y, since the
> kretprobe_trampoline skips updating frame pointer, the stack frame
> of the kretprobe_trampoline seems non-standard. So this marks it
> is STACK_FRAME_NON_STANDARD() and undefine UNWIND_HINT_FUNC.

What does 'marks it is' mean?

'undefine' UNWIND_HINT_FUNC?

Doesn't the patch do the exact opposite:

  > +#define UNWIND_HINT_FUNC \
  > +	UNWIND_HINT(ORC_REG_SP, 8, UNWIND_HINT_TYPE_FUNC, 0)

But it does undefine it in a specific spot:



> Anyway, with the frame pointer, FP unwinder can unwind the stack
> frame correctly without that hint.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Tested-by: Andrii Nakryik <andrii@kernel.org>

I have to say these changelogs are very careless.

> +#else
> +

In headers, in longer CPP blocks, please always mark the '#else' branch 
with what it is the else branch of.

See the output of:

   kepler:~/tip> git grep '#else' arch/x86/include/asm/ | head

> +#ifdef CONFIG_FRAME_POINTER
> +/*
> + * kretprobe_trampoline skips updating frame pointer. The frame pointer
> + * saved in trampoline_handler points to the real caller function's
> + * frame pointer. Thus the kretprobe_trampoline doesn't seems to have a
> + * standard stack frame with CONFIG_FRAME_POINTER=y.
> + * Let's mark it non-standard function. Anyway, FP unwinder can correctly
> + * unwind without the hint.

s/doesn't seems to have a standard stack frame
 /doesn't have a standard stack frame

There's nothing 'seems' about the situation - it's a non-standard function 
entry and stack frame situation, and the unwinder needs to know about it.

> +STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
> +#undef UNWIND_HINT_FUNC
> +#define UNWIND_HINT_FUNC
> +#endif
>  /*
>   * When a retprobed function returns, this code saves registers and
>   * calls trampoline_handler() runs, which calls the kretprobe's handler.
> @@ -1031,6 +1044,7 @@ asm(
>  	/* We don't bother saving the ss register */
>  #ifdef CONFIG_X86_64
>  	"	pushq %rsp\n"
> +	UNWIND_HINT_FUNC
>  	"	pushfq\n"
>  	SAVE_REGS_STRING
>  	"	movq %rsp, %rdi\n"
> @@ -1041,6 +1055,7 @@ asm(
>  	"	popfq\n"
>  #else
>  	"	pushl %esp\n"
> +	UNWIND_HINT_FUNC
>  	"	pushfl\n"
>  	SAVE_REGS_STRING
>  	"	movl %esp, %eax\n"

Why not provide an appropriate annotation method in <asm/unwind_hints.h>, 
so that other future code can use it too instead of reinventing the wheel?

Thanks,

	Ingo
Masami Hiramatsu (Google) July 9, 2021, 3:31 p.m. UTC | #2 
On Mon, 5 Jul 2021 10:02:47 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > From: Josh Poimboeuf <jpoimboe@redhat.com>
> > 
> > Add UNWIND_HINT_FUNC on kretporbe_trampoline code so that ORC
> > information is generated on the kretprobe_trampoline correctly.
> 
> What is a 'kretporbe'?

Oops, it's a typo.

> 
> > Note that when the CONFIG_FRAME_POINTER=y, since the
> > kretprobe_trampoline skips updating frame pointer, the stack frame
> > of the kretprobe_trampoline seems non-standard. So this marks it
> > is STACK_FRAME_NON_STANDARD() and undefine UNWIND_HINT_FUNC.
> 
> What does 'marks it is' mean?

Sorry, I meant, this marks the kretprobe_trampoline as non-standard
stack frame by STACK_FRAME_NON_STANDARD().

> 
> 'undefine' UNWIND_HINT_FUNC?
> 
> Doesn't the patch do the exact opposite:
> 
>   > +#define UNWIND_HINT_FUNC \
>   > +	UNWIND_HINT(ORC_REG_SP, 8, UNWIND_HINT_TYPE_FUNC, 0)
> 
> But it does undefine it in a specific spot:

Yes, if you think this is not correct way, what about the following?

#ifdef CONFIG_FRAME_POINTER
STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
#define KRETPROBE_UNWIND_HINT_FUNC
#else
#define KRETPROBE_UNWIND_HINT_FUNC	UNWIND_HINT_FUNC
#endif


> > Anyway, with the frame pointer, FP unwinder can unwind the stack
> > frame correctly without that hint.
> > 
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Tested-by: Andrii Nakryik <andrii@kernel.org>
> 
> I have to say these changelogs are very careless.

Sorry for inconvenience...

> 
> > +#else
> > +
> 
> In headers, in longer CPP blocks, please always mark the '#else' branch 
> with what it is the else branch of.

OK.

> 
> See the output of:
> 
>    kepler:~/tip> git grep '#else' arch/x86/include/asm/ | head

Thanks for the hint!

> 
> > +#ifdef CONFIG_FRAME_POINTER
> > +/*
> > + * kretprobe_trampoline skips updating frame pointer. The frame pointer
> > + * saved in trampoline_handler points to the real caller function's
> > + * frame pointer. Thus the kretprobe_trampoline doesn't seems to have a
> > + * standard stack frame with CONFIG_FRAME_POINTER=y.
> > + * Let's mark it non-standard function. Anyway, FP unwinder can correctly
> > + * unwind without the hint.
> 
> s/doesn't seems to have a standard stack frame
>  /doesn't have a standard stack frame
> 
> There's nothing 'seems' about the situation - it's a non-standard function 
> entry and stack frame situation, and the unwinder needs to know about it.

OK.

> 
> > +STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
> > +#undef UNWIND_HINT_FUNC
> > +#define UNWIND_HINT_FUNC
> > +#endif
> >  /*
> >   * When a retprobed function returns, this code saves registers and
> >   * calls trampoline_handler() runs, which calls the kretprobe's handler.
> > @@ -1031,6 +1044,7 @@ asm(
> >  	/* We don't bother saving the ss register */
> >  #ifdef CONFIG_X86_64
> >  	"	pushq %rsp\n"
> > +	UNWIND_HINT_FUNC
> >  	"	pushfq\n"
> >  	SAVE_REGS_STRING
> >  	"	movq %rsp, %rdi\n"
> > @@ -1041,6 +1055,7 @@ asm(
> >  	"	popfq\n"
> >  #else
> >  	"	pushl %esp\n"
> > +	UNWIND_HINT_FUNC
> >  	"	pushfl\n"
> >  	SAVE_REGS_STRING
> >  	"	movl %esp, %eax\n"
> 
> Why not provide an appropriate annotation method in <asm/unwind_hints.h>, 
> so that other future code can use it too instead of reinventing the wheel?

Would you mean we should define the UNWIND_HINT_FUNC as a macro
which depends on CONFIG_FRAME_POINTER, in <asm/unwind_hints.h>?

Josh, what would you think?

Thank you,
Masami Hiramatsu (Google) July 10, 2021, 1:41 a.m. UTC | #3 
Hi Ingo and Josh,

On Sat, 10 Jul 2021 00:31:40 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > > +STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
> > > +#undef UNWIND_HINT_FUNC
> > > +#define UNWIND_HINT_FUNC
> > > +#endif
> > >  /*
> > >   * When a retprobed function returns, this code saves registers and
> > >   * calls trampoline_handler() runs, which calls the kretprobe's handler.
> > > @@ -1031,6 +1044,7 @@ asm(
> > >  	/* We don't bother saving the ss register */
> > >  #ifdef CONFIG_X86_64
> > >  	"	pushq %rsp\n"
> > > +	UNWIND_HINT_FUNC
> > >  	"	pushfq\n"
> > >  	SAVE_REGS_STRING
> > >  	"	movq %rsp, %rdi\n"
> > > @@ -1041,6 +1055,7 @@ asm(
> > >  	"	popfq\n"
> > >  #else
> > >  	"	pushl %esp\n"
> > > +	UNWIND_HINT_FUNC
> > >  	"	pushfl\n"
> > >  	SAVE_REGS_STRING
> > >  	"	movl %esp, %eax\n"
> > 
> > Why not provide an appropriate annotation method in <asm/unwind_hints.h>, 
> > so that other future code can use it too instead of reinventing the wheel?

I think I got what you meant. Let me summarize the issue.

In case of CONFIG_FRAME_POINTER=n, it is OK just adding UNWIND_HINT_FUNC.

In case of CONFIG_FRAME_POINTER=y, without STACK_FRAME_NON_STANDARD(),
the objtool complains that a CALL instruction without the frame pointer.
---
  arch/x86/kernel/kprobes/core.o: warning: objtool: __kretprobe_trampoline()+0x25: call without frame pointer save/setup
---

If we just add STACK_FRAME_NON_STANDARD() with UNWIND_HINT_FUNC macro,
the objtool complains that non-standard function has unwind hint.
---
arch/x86/kernel/kprobes/core.o: warning: objtool: __kretprobe_trampoline()+0x1: BUG: why am I validating an ignored function?
---

Thus, add STACK_FRAME_NON_STANDARD() and undefine UNWIND_HINT_FUNC macro,
the objtool doesn't complain.

This means that the STACK_FRAME_NON_STANDARD() and UNWIND_HINT_FUNC macro
are mutually exclusive. However, those macros are used different way.
The STACK_FRAME_NON_STANDARD() will have the target symbol and the
UNWIND_HINT_FUNC needs to be embedded in the target code.
Thus we can not combine them in general.

If we can have something like UNWIND_HINT_FUNC_NO_FP, it may solve this
issue without ugly #ifdef and #undef.

Is that correct?

Maybe I can add UNWIND_HINT_TYPE_FUNC_NO_FP for UNWIND_HINT and make objtool
ignore the call without frame pointer. This makes an exception that the
kretprobe_trampoline will be noted in '.discard.unwind_hints' section
instead of '.discard.func_stack_frame_non_standard' section. 

Or another idea is to introduce ANNOTATE_NO_FP_FUNCTION_CALL with a new
'.discard.no_fp_function_calls' section.

What do you think these ideas?

Thank you,
Josh Poimboeuf July 10, 2021, 7:01 p.m. UTC | #4 
On Sat, Jul 10, 2021 at 10:41:04AM +0900, Masami Hiramatsu wrote:
> Hi Ingo and Josh,
> 
> On Sat, 10 Jul 2021 00:31:40 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > > > +STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
> > > > +#undef UNWIND_HINT_FUNC
> > > > +#define UNWIND_HINT_FUNC
> > > > +#endif
> > > >  /*
> > > >   * When a retprobed function returns, this code saves registers and
> > > >   * calls trampoline_handler() runs, which calls the kretprobe's handler.
> > > > @@ -1031,6 +1044,7 @@ asm(
> > > >  	/* We don't bother saving the ss register */
> > > >  #ifdef CONFIG_X86_64
> > > >  	"	pushq %rsp\n"
> > > > +	UNWIND_HINT_FUNC
> > > >  	"	pushfq\n"
> > > >  	SAVE_REGS_STRING
> > > >  	"	movq %rsp, %rdi\n"
> > > > @@ -1041,6 +1055,7 @@ asm(
> > > >  	"	popfq\n"
> > > >  #else
> > > >  	"	pushl %esp\n"
> > > > +	UNWIND_HINT_FUNC
> > > >  	"	pushfl\n"
> > > >  	SAVE_REGS_STRING
> > > >  	"	movl %esp, %eax\n"
> > > 
> > > Why not provide an appropriate annotation method in <asm/unwind_hints.h>, 
> > > so that other future code can use it too instead of reinventing the wheel?
> 
> I think I got what you meant. Let me summarize the issue.
> 
> In case of CONFIG_FRAME_POINTER=n, it is OK just adding UNWIND_HINT_FUNC.
> 
> In case of CONFIG_FRAME_POINTER=y, without STACK_FRAME_NON_STANDARD(),
> the objtool complains that a CALL instruction without the frame pointer.
> ---
>   arch/x86/kernel/kprobes/core.o: warning: objtool: __kretprobe_trampoline()+0x25: call without frame pointer save/setup
> ---
> 
> If we just add STACK_FRAME_NON_STANDARD() with UNWIND_HINT_FUNC macro,
> the objtool complains that non-standard function has unwind hint.
> ---
> arch/x86/kernel/kprobes/core.o: warning: objtool: __kretprobe_trampoline()+0x1: BUG: why am I validating an ignored function?

I'm thinking this latter warning indicates an objtool bug (as the BUG
warning claims).  If a function is ignored with
STACK_FRAME_NON_STANDARD() then objtool should probably also ignore its
hints.  Then we should be able to get rid of the #undef/#ifdef
UNWIND_HINT_FUNC silliness.

The other warning is correct and STACK_FRAME_NON_STANDARD() still needs
to be behind '#ifdef CONFIG_FRAME_POINTER' since the function is missing
a frame pointer.  So maybe we can make a STACK_FRAME_NON_STANDARD_FP()
or similar.

I'll post a few patches.
diff mbox series

Patch

diff --git a/arch/x86/include/asm/unwind_hints.h b/arch/x86/include/asm/unwind_hints.h
index 8e574c0afef8..8b33674288ea 100644
--- a/arch/x86/include/asm/unwind_hints.h
+++ b/arch/x86/include/asm/unwind_hints.h
@@ -52,6 +52,11 @@ 
 	UNWIND_HINT sp_reg=ORC_REG_SP sp_offset=8 type=UNWIND_HINT_TYPE_FUNC
 .endm
 
+#else
+
+#define UNWIND_HINT_FUNC \
+	UNWIND_HINT(ORC_REG_SP, 8, UNWIND_HINT_TYPE_FUNC, 0)
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* _ASM_X86_UNWIND_HINTS_H */
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 2dccb4347453..74f049b6e77f 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -1019,6 +1019,19 @@  int kprobe_int3_handler(struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(kprobe_int3_handler);
 
+#ifdef CONFIG_FRAME_POINTER
+/*
+ * kretprobe_trampoline skips updating frame pointer. The frame pointer
+ * saved in trampoline_handler points to the real caller function's
+ * frame pointer. Thus the kretprobe_trampoline doesn't seems to have a
+ * standard stack frame with CONFIG_FRAME_POINTER=y.
+ * Let's mark it non-standard function. Anyway, FP unwinder can correctly
+ * unwind without the hint.
+ */
+STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
+#undef UNWIND_HINT_FUNC
+#define UNWIND_HINT_FUNC
+#endif
 /*
  * When a retprobed function returns, this code saves registers and
  * calls trampoline_handler() runs, which calls the kretprobe's handler.
@@ -1031,6 +1044,7 @@  asm(
 	/* We don't bother saving the ss register */
 #ifdef CONFIG_X86_64
 	"	pushq %rsp\n"
+	UNWIND_HINT_FUNC
 	"	pushfq\n"
 	SAVE_REGS_STRING
 	"	movq %rsp, %rdi\n"
@@ -1041,6 +1055,7 @@  asm(
 	"	popfq\n"
 #else
 	"	pushl %esp\n"
+	UNWIND_HINT_FUNC
 	"	pushfl\n"
 	SAVE_REGS_STRING
 	"	movl %esp, %eax\n"
@@ -1054,8 +1069,6 @@  asm(
 	".size kretprobe_trampoline, .-kretprobe_trampoline\n"
 );
 NOKPROBE_SYMBOL(kretprobe_trampoline);
-STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
-
 
 /*
  * Called from kretprobe_trampoline