diff mbox series

[v3,8/9] ARM: kprobes: Make a frame pointer on __kretprobe_trampoline

Message ID 163477772593.264901.7405996794526239017.stgit@devnote2 (mailing list archive)
State New, archived
Headers show
Series kprobes: Make KUnit and add stacktrace on kretprobe tests | expand

Commit Message

Masami Hiramatsu (Google) Oct. 21, 2021, 12:55 a.m. UTC
Currently kretprobe on ARM just fills r0-r11 of pt_regs, but
that is not enough for the stacktrace. Moreover, from the user
kretprobe handler, stacktrace needs a frame pointer on the
__kretprobe_trampoline.

This adds a frame pointer on __kretprobe_trampoline for both gcc
and clang case. Those have different frame pointer so we need
different but similar stack on pt_regs.

Gcc makes the frame pointer (fp) to point the 'pc' address of
the {fp, ip (=sp), lr, pc}, this means {r11, r13, r14, r15}.
Thus if we save the r11 (fp) on pt_regs->r12, we can make this
set on the end of pt_regs.

On the other hand, Clang makes the frame pointer to point the
'fp' address of {fp, lr} on stack. Since the next to the
pt_regs->lr is pt_regs->sp, I reused the pair of pt_regs->fp
and pt_regs->ip.
So this stores the 'lr' on pt_regs->ip and make the fp to point
pt_regs->fp.

For both cases, saves __kretprobe_trampoline address to
pt_regs->lr, so that the stack tracer can identify this frame
pointer has been made by the __kretprobe_trampoline.

Note that if the CONFIG_FRAME_POINTER is not set, this keeps
fp as is.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---
 Changes in v3:
  - Avoid using !sp when storing sp itself.
  - Unify stmdb register list.
  - Keep the current assembly code when CONFIG_FRAME_POINTER=n.
---
 arch/arm/probes/kprobes/core.c |   28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

Comments

Arnd Bergmann Dec. 3, 2021, 8:37 p.m. UTC | #1
On Thu, Oct 21, 2021 at 2:55 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Currently kretprobe on ARM just fills r0-r11 of pt_regs, but
> that is not enough for the stacktrace. Moreover, from the user
> kretprobe handler, stacktrace needs a frame pointer on the
> __kretprobe_trampoline.
>
> This adds a frame pointer on __kretprobe_trampoline for both gcc
> and clang case. Those have different frame pointer so we need
> different but similar stack on pt_regs.
>
> Gcc makes the frame pointer (fp) to point the 'pc' address of
> the {fp, ip (=sp), lr, pc}, this means {r11, r13, r14, r15}.
> Thus if we save the r11 (fp) on pt_regs->r12, we can make this
> set on the end of pt_regs.
>
> On the other hand, Clang makes the frame pointer to point the
> 'fp' address of {fp, lr} on stack. Since the next to the
> pt_regs->lr is pt_regs->sp, I reused the pair of pt_regs->fp
> and pt_regs->ip.
> So this stores the 'lr' on pt_regs->ip and make the fp to point
> pt_regs->fp.
>
> For both cases, saves __kretprobe_trampoline address to
> pt_regs->lr, so that the stack tracer can identify this frame
> pointer has been made by the __kretprobe_trampoline.
>
> Note that if the CONFIG_FRAME_POINTER is not set, this keeps
> fp as is.
>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

This causes a regression that I see in randconfig builds with gcc-11:

/tmp/ccovvQNw.s: Assembler messages:
/tmp/ccovvQNw.s:32: Error: invalid literal constant: pool needs to be closer
make[5]: *** [/git/arm-soc/scripts/Makefile.build:288:
arch/arm/probes/kprobes/core.o] Error 1

I have two randconfigs that reproduce it locally, here is a .config
[1] and assembly
output[2] for reference. I have not done any further analysis, but
maybe you have
an idea.

        Arnd

[1] https://pastebin.com/y4rkH8qX
[2] https://pastebin.com/9mEVU8Rd
Ard Biesheuvel Dec. 4, 2021, 8:45 a.m. UTC | #2
On Fri, 3 Dec 2021 at 21:38, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Oct 21, 2021 at 2:55 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Currently kretprobe on ARM just fills r0-r11 of pt_regs, but
> > that is not enough for the stacktrace. Moreover, from the user
> > kretprobe handler, stacktrace needs a frame pointer on the
> > __kretprobe_trampoline.
> >
> > This adds a frame pointer on __kretprobe_trampoline for both gcc
> > and clang case. Those have different frame pointer so we need
> > different but similar stack on pt_regs.
> >
> > Gcc makes the frame pointer (fp) to point the 'pc' address of
> > the {fp, ip (=sp), lr, pc}, this means {r11, r13, r14, r15}.
> > Thus if we save the r11 (fp) on pt_regs->r12, we can make this
> > set on the end of pt_regs.
> >
> > On the other hand, Clang makes the frame pointer to point the
> > 'fp' address of {fp, lr} on stack. Since the next to the
> > pt_regs->lr is pt_regs->sp, I reused the pair of pt_regs->fp
> > and pt_regs->ip.
> > So this stores the 'lr' on pt_regs->ip and make the fp to point
> > pt_regs->fp.
> >
> > For both cases, saves __kretprobe_trampoline address to
> > pt_regs->lr, so that the stack tracer can identify this frame
> > pointer has been made by the __kretprobe_trampoline.
> >
> > Note that if the CONFIG_FRAME_POINTER is not set, this keeps
> > fp as is.
> >
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
>
> This causes a regression that I see in randconfig builds with gcc-11:
>
> /tmp/ccovvQNw.s: Assembler messages:
> /tmp/ccovvQNw.s:32: Error: invalid literal constant: pool needs to be closer
> make[5]: *** [/git/arm-soc/scripts/Makefile.build:288:
> arch/arm/probes/kprobes/core.o] Error 1
>
> I have two randconfigs that reproduce it locally, here is a .config
> [1] and assembly
> output[2] for reference. I have not done any further analysis, but
> maybe you have
> an idea.

Does this help?

diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 9090c3a74dcc..88999ed2cfc4 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -408,6 +408,7 @@ void __naked __kprobes __kretprobe_trampoline(void)
 #else
                "mov    pc, lr                  \n\t"
 #endif
+               ".ltorg                         \n\t"
                : : : "memory");
 }
Arnd Bergmann Dec. 4, 2021, 12:08 p.m. UTC | #3
On Sat, Dec 4, 2021 at 9:45 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Does this help?

Yes, this fixes it, thanks for the quick help!

       Arnd
Masami Hiramatsu (Google) Dec. 8, 2021, 12:26 p.m. UTC | #4
On Sat, 4 Dec 2021 13:08:46 +0100
Arnd Bergmann <arnd@arndb.de> wrote:

> On Sat, Dec 4, 2021 at 9:45 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > Does this help?
> 
> Yes, this fixes it, thanks for the quick help!

Thanks Ard and Arnd!
BTW, would you know what kconfig item warns this issue, or is it only
with gcc-11?
I would like to build the same environment.

Thank you,


> 
>        Arnd
diff mbox series

Patch

diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 95f23b47ba27..4848404ba51b 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -368,16 +368,36 @@  int __kprobes kprobe_exceptions_notify(struct notifier_block *self,
 /*
  * When a retprobed function returns, trampoline_handler() is called,
  * calling the kretprobe's handler. We construct a struct pt_regs to
- * give a view of registers r0-r11 to the user return-handler.  This is
- * not a complete pt_regs structure, but that should be plenty sufficient
- * for kretprobe handlers which should normally be interested in r0 only
- * anyway.
+ * give a view of registers r0-r11, sp, lr, and pc to the user
+ * return-handler. This is not a complete pt_regs structure, but that
+ * should be enough for stacktrace from the return handler with or
+ * without pt_regs.
  */
 void __naked __kprobes __kretprobe_trampoline(void)
 {
 	__asm__ __volatile__ (
+#ifdef CONFIG_FRAME_POINTER
+		"ldr	lr, =__kretprobe_trampoline	\n\t"
+	/* __kretprobe_trampoline makes a framepointer on pt_regs. */
+#ifdef CONFIG_CC_IS_CLANG
+		"stmdb	sp, {sp, lr, pc}	\n\t"
+		"sub	sp, sp, #12		\n\t"
+		/* In clang case, pt_regs->ip = lr. */
+		"stmdb	sp!, {r0 - r11, lr}	\n\t"
+		/* fp points regs->r11 (fp) */
+		"add	fp, sp,	#44		\n\t"
+#else /* !CONFIG_CC_IS_CLANG */
+		/* In gcc case, pt_regs->ip = fp. */
+		"stmdb	sp, {fp, sp, lr, pc}	\n\t"
 		"sub	sp, sp, #16		\n\t"
 		"stmdb	sp!, {r0 - r11}		\n\t"
+		/* fp points regs->r15 (pc) */
+		"add	fp, sp, #60		\n\t"
+#endif /* CONFIG_CC_IS_CLANG */
+#else /* !CONFIG_FRAME_POINTER */
+		"sub	sp, sp, #16		\n\t"
+		"stmdb	sp!, {r0 - r11}		\n\t"
+#endif /* CONFIG_FRAME_POINTER */
 		"mov	r0, sp			\n\t"
 		"bl	trampoline_handler	\n\t"
 		"mov	lr, r0			\n\t"