From: Masami Hiramatsu <mhiramat@kernel.org> To: "Russell King (Oracle)" <linux@armlinux.org.uk> Cc: Steven Rostedt <rostedt@goodmis.org>, "Naveen N . Rao" <naveen.n.rao@linux.vnet.ibm.com>, Ananth N Mavinakayanahalli <ananth@linux.ibm.com>, Ingo Molnar <mingo@kernel.org>, linux-kernel@vger.kernel.org, Sven Schnelle <svens@linux.ibm.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Nathan Chancellor <nathan@kernel.org>, Nick Desaulniers <ndesaulniers@google.com>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 08/10] ARM: kprobes: Make a frame pointer on __kretprobe_trampoline Date: Mon, 18 Oct 2021 14:55:36 +0900 [thread overview] Message-ID: <20211018145536.3cb042e574560fa4a00aa575@kernel.org> (raw) In-Reply-To: <YWtBDXJQRLN2T1c6@shell.armlinux.org.uk> On Sat, 16 Oct 2021 22:15:57 +0100 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Fri, Oct 15, 2021 at 09:51:56PM +0900, Masami Hiramatsu 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> > > --- > > arch/arm/probes/kprobes/core.c | 29 ++++++++++++++++++++++++----- > > 1 file changed, 24 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c > > index 95f23b47ba27..7cbd65a22769 100644 > > --- a/arch/arm/probes/kprobes/core.c > > +++ b/arch/arm/probes/kprobes/core.c > > @@ -368,16 +368,35 @@ 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__ ( > > - "sub sp, sp, #16 \n\t" > > + "ldr lr, =__kretprobe_trampoline \n\t" > > + "stmdb sp!, {sp, lr, pc} \n\t" > > I think you really do not want to do that. Yes, I just wants to save the {sp, lr, pc} to mimic the framepointer. > > From DDI0406C: > > "ARM deprecates the use of instructions with the base register in the > list and ! specified. If the base register is not the lowest-numbered > register in the list, such an instruction stores an UNKNOWN value for > the base register." > > However, it doesn't say what value is stored if the base register is > the lowest-numbered register in the list. The pseudocode given shows > that it is the original value. However, DDI0100E: > > "Operand restrictions > If <Rn> is specified as <registers> and base register writeback is > specified: > • If <Rn> is the lowest-numbered register specified in > <register_list>, the original value of <Rn> is stored. > • Otherwise, the stored value of <Rn> is UNPREDICTABLE." > > So I guess it might be okay... but it seems a bit dodgy to rely on > this behaviour. Oh, OK. I just tested it on qemu-arm so maybe it was wrong. > > > +#ifdef CONFIG_FRAME_POINTER > > + /* __kretprobe_trampoline makes a framepointer on pt_regs. */ > > +#ifdef CONFIG_CC_IS_CLANG > > + /* In clang case, pt_regs->ip = lr. */ > > + "stmdb sp!, {lr} \n\t" > > "stmdb sp!, {r0 - r11} \n\t" > > This can be simplified to: > "stmdb sp!, {r0 - r11, lr} \n\t" > > Also, note the value we store for "fp" is __kretprobe_trampoline. Oh, I thought 'r11' is 'fp' from arch/arm/include/uapi/asm/ptrace.h ... #define ARM_ip uregs[12] #define ARM_fp uregs[11] #define ARM_r10 uregs[10] ... Is that fp? or ip? > > > + /* 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} \n\t" > > + "stmdb sp!, {r0 - r11} \n\t" > > This can be simplified to: > "stmdb sp!, {r0 - r12} \n\t" > > since fp is r12. Ditto. The arch/arm/include/uapi/asm/ptrace.h seems to say 'r12' is 'ip'. Thank you, > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! -- Masami Hiramatsu <mhiramat@kernel.org>
WARNING: multiple messages have this Message-ID (diff)
From: Masami Hiramatsu <mhiramat@kernel.org> To: "Russell King (Oracle)" <linux@armlinux.org.uk> Cc: Steven Rostedt <rostedt@goodmis.org>, "Naveen N . Rao" <naveen.n.rao@linux.vnet.ibm.com>, Ananth N Mavinakayanahalli <ananth@linux.ibm.com>, Ingo Molnar <mingo@kernel.org>, linux-kernel@vger.kernel.org, Sven Schnelle <svens@linux.ibm.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Nathan Chancellor <nathan@kernel.org>, Nick Desaulniers <ndesaulniers@google.com>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 08/10] ARM: kprobes: Make a frame pointer on __kretprobe_trampoline Date: Mon, 18 Oct 2021 14:55:36 +0900 [thread overview] Message-ID: <20211018145536.3cb042e574560fa4a00aa575@kernel.org> (raw) In-Reply-To: <YWtBDXJQRLN2T1c6@shell.armlinux.org.uk> On Sat, 16 Oct 2021 22:15:57 +0100 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Fri, Oct 15, 2021 at 09:51:56PM +0900, Masami Hiramatsu 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> > > --- > > arch/arm/probes/kprobes/core.c | 29 ++++++++++++++++++++++++----- > > 1 file changed, 24 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c > > index 95f23b47ba27..7cbd65a22769 100644 > > --- a/arch/arm/probes/kprobes/core.c > > +++ b/arch/arm/probes/kprobes/core.c > > @@ -368,16 +368,35 @@ 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__ ( > > - "sub sp, sp, #16 \n\t" > > + "ldr lr, =__kretprobe_trampoline \n\t" > > + "stmdb sp!, {sp, lr, pc} \n\t" > > I think you really do not want to do that. Yes, I just wants to save the {sp, lr, pc} to mimic the framepointer. > > From DDI0406C: > > "ARM deprecates the use of instructions with the base register in the > list and ! specified. If the base register is not the lowest-numbered > register in the list, such an instruction stores an UNKNOWN value for > the base register." > > However, it doesn't say what value is stored if the base register is > the lowest-numbered register in the list. The pseudocode given shows > that it is the original value. However, DDI0100E: > > "Operand restrictions > If <Rn> is specified as <registers> and base register writeback is > specified: > • If <Rn> is the lowest-numbered register specified in > <register_list>, the original value of <Rn> is stored. > • Otherwise, the stored value of <Rn> is UNPREDICTABLE." > > So I guess it might be okay... but it seems a bit dodgy to rely on > this behaviour. Oh, OK. I just tested it on qemu-arm so maybe it was wrong. > > > +#ifdef CONFIG_FRAME_POINTER > > + /* __kretprobe_trampoline makes a framepointer on pt_regs. */ > > +#ifdef CONFIG_CC_IS_CLANG > > + /* In clang case, pt_regs->ip = lr. */ > > + "stmdb sp!, {lr} \n\t" > > "stmdb sp!, {r0 - r11} \n\t" > > This can be simplified to: > "stmdb sp!, {r0 - r11, lr} \n\t" > > Also, note the value we store for "fp" is __kretprobe_trampoline. Oh, I thought 'r11' is 'fp' from arch/arm/include/uapi/asm/ptrace.h ... #define ARM_ip uregs[12] #define ARM_fp uregs[11] #define ARM_r10 uregs[10] ... Is that fp? or ip? > > > + /* 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} \n\t" > > + "stmdb sp!, {r0 - r11} \n\t" > > This can be simplified to: > "stmdb sp!, {r0 - r12} \n\t" > > since fp is r12. Ditto. The arch/arm/include/uapi/asm/ptrace.h seems to say 'r12' is 'ip'. Thank you, > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! -- Masami Hiramatsu <mhiramat@kernel.org> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-10-18 5:55 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-15 12:50 [PATCH 00/10] kprobes: Make KUnit and add stacktrace on kretprobe tests Masami Hiramatsu 2021-10-15 12:50 ` Masami Hiramatsu 2021-10-15 12:50 ` [PATCH 01/10] kprobes: convert tests to kunit Masami Hiramatsu 2021-10-15 12:50 ` Masami Hiramatsu 2021-10-15 12:51 ` [PATCH 02/10] kprobes: Add a test case for stacktrace from kretprobe handler Masami Hiramatsu 2021-10-15 12:51 ` Masami Hiramatsu 2021-10-15 12:51 ` [PATCH 03/10] x86/unwind: Compile kretprobe fixup code only if CONFIG_KRETPROBES=y Masami Hiramatsu 2021-10-15 12:51 ` Masami Hiramatsu 2021-10-15 13:10 ` Masami Hiramatsu 2021-10-15 13:10 ` Masami Hiramatsu 2021-10-15 12:51 ` [PATCH 04/10] arm64: kprobes: Record frame pointer with kretprobe instance Masami Hiramatsu 2021-10-15 12:51 ` Masami Hiramatsu 2021-10-15 12:51 ` [PATCH 05/10] arm64: kprobes: Make a frame pointer on __kretprobe_trampoline Masami Hiramatsu 2021-10-15 12:51 ` Masami Hiramatsu 2021-10-15 12:51 ` [PATCH 06/10] arm64: Recover kretprobe modified return address in stacktrace Masami Hiramatsu 2021-10-15 12:51 ` Masami Hiramatsu 2021-10-15 12:51 ` [PATCH 07/10] ARM: clang: Do not rely on lr register for stacktrace Masami Hiramatsu 2021-10-15 12:51 ` Masami Hiramatsu 2021-10-15 12:51 ` [PATCH 08/10] ARM: kprobes: Make a frame pointer on __kretprobe_trampoline Masami Hiramatsu 2021-10-15 12:51 ` Masami Hiramatsu 2021-10-16 10:37 ` kernel test robot 2021-10-16 10:37 ` kernel test robot 2021-10-16 21:15 ` Russell King (Oracle) 2021-10-16 21:15 ` Russell King (Oracle) 2021-10-18 5:55 ` Masami Hiramatsu [this message] 2021-10-18 5:55 ` Masami Hiramatsu 2021-10-15 12:52 ` [PATCH 09/10] ARM: Recover kretprobe modified return address in stacktrace Masami Hiramatsu 2021-10-15 12:52 ` Masami Hiramatsu 2021-10-15 12:52 ` [PATCH 10/10] [RFC] arm64: kprobes: Detect error of kretprobe return address fixup Masami Hiramatsu 2021-10-15 12:52 ` Masami Hiramatsu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20211018145536.3cb042e574560fa4a00aa575@kernel.org \ --to=mhiramat@kernel.org \ --cc=ananth@linux.ibm.com \ --cc=catalin.marinas@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=mingo@kernel.org \ --cc=nathan@kernel.org \ --cc=naveen.n.rao@linux.vnet.ibm.com \ --cc=ndesaulniers@google.com \ --cc=rostedt@goodmis.org \ --cc=svens@linux.ibm.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.