[RESEND v3] arm64: Add USER_STACKTRACE support

[RESEND v3] arm64: Add USER_STACKTRACE support

[chenqiwu]

Currently, userstacktrace is unsupported for ftrace and uprobe
tracers on arm64. This patch uses the perf_callchain_user() code
as blueprint to implement the arch_stack_walk_user() which add
userstacktrace support on arm64.
Meanwhile, we can use arch_stack_walk_user() to simplify the
implementation of perf_callchain_user().
This patch is tested pass with ftrace, uprobe and perf tracers
profiling userstacktrace cases.

changes in v3:
  - update perf_callchain_user() to use arch_stack_walk_user()
    and delete the redundant code as Mark's suggestion in v2.
  - update the commit message.

Tested-by: chenqiwu <qiwu.chen@transsion.com>
Signed-off-by: chenqiwu <qiwu.chen@transsion.com>
---
 arch/arm64/Kconfig                 |   1 +
 arch/arm64/kernel/perf_callchain.c | 118 +---------------------------
 arch/arm64/kernel/stacktrace.c     | 120 +++++++++++++++++++++++++++++
 3 files changed, 125 insertions(+), 114 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7b071a00425d..4c5066f88dd2 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -255,6 +255,7 @@ config ARM64
 	select TRACE_IRQFLAGS_SUPPORT
 	select TRACE_IRQFLAGS_NMI_SUPPORT
 	select HAVE_SOFTIRQ_ON_OWN_STACK
+	select USER_STACKTRACE_SUPPORT
 	help
 	  ARM 64-bit (AArch64) Linux support.
 
diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
index 6d157f32187b..e8ed5673f481 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -10,94 +10,12 @@
 
 #include <asm/pointer_auth.h>
 
-struct frame_tail {
-	struct frame_tail	__user *fp;
-	unsigned long		lr;
-} __attribute__((packed));
-
-/*
- * Get the return address for a single stackframe and return a pointer to the
- * next frame tail.
- */
-static struct frame_tail __user *
-user_backtrace(struct frame_tail __user *tail,
-	       struct perf_callchain_entry_ctx *entry)
-{
-	struct frame_tail buftail;
-	unsigned long err;
-	unsigned long lr;
-
-	/* Also check accessibility of one struct frame_tail beyond */
-	if (!access_ok(tail, sizeof(buftail)))
-		return NULL;
-
-	pagefault_disable();
-	err = __copy_from_user_inatomic(&buftail, tail, sizeof(buftail));
-	pagefault_enable();
-
-	if (err)
-		return NULL;
-
-	lr = ptrauth_strip_user_insn_pac(buftail.lr);
-
-	perf_callchain_store(entry, lr);
-
-	/*
-	 * Frame pointers should strictly progress back up the stack
-	 * (towards higher addresses).
-	 */
-	if (tail >= buftail.fp)
-		return NULL;
-
-	return buftail.fp;
-}
-
-#ifdef CONFIG_COMPAT
-/*
- * The registers we're interested in are at the end of the variable
- * length saved register structure. The fp points at the end of this
- * structure so the address of this struct is:
- * (struct compat_frame_tail *)(xxx->fp)-1
- *
- * This code has been adapted from the ARM OProfile support.
- */
-struct compat_frame_tail {
-	compat_uptr_t	fp; /* a (struct compat_frame_tail *) in compat mode */
-	u32		sp;
-	u32		lr;
-} __attribute__((packed));
-
-static struct compat_frame_tail __user *
-compat_user_backtrace(struct compat_frame_tail __user *tail,
-		      struct perf_callchain_entry_ctx *entry)
+static bool callchain_trace(void *data, unsigned long pc)
 {
-	struct compat_frame_tail buftail;
-	unsigned long err;
-
-	/* Also check accessibility of one struct frame_tail beyond */
-	if (!access_ok(tail, sizeof(buftail)))
-		return NULL;
-
-	pagefault_disable();
-	err = __copy_from_user_inatomic(&buftail, tail, sizeof(buftail));
-	pagefault_enable();
-
-	if (err)
-		return NULL;
-
-	perf_callchain_store(entry, buftail.lr);
-
-	/*
-	 * Frame pointers should strictly progress back up the stack
-	 * (towards higher addresses).
-	 */
-	if (tail + 1 >= (struct compat_frame_tail __user *)
-			compat_ptr(buftail.fp))
-		return NULL;
+	struct perf_callchain_entry_ctx *entry = data;
 
-	return (struct compat_frame_tail __user *)compat_ptr(buftail.fp) - 1;
+	return perf_callchain_store(entry, pc) == 0;
 }
-#endif /* CONFIG_COMPAT */
 
 void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
 			 struct pt_regs *regs)
@@ -107,35 +25,7 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
 		return;
 	}
 
-	perf_callchain_store(entry, regs->pc);
-
-	if (!compat_user_mode(regs)) {
-		/* AARCH64 mode */
-		struct frame_tail __user *tail;
-
-		tail = (struct frame_tail __user *)regs->regs[29];
-
-		while (entry->nr < entry->max_stack &&
-		       tail && !((unsigned long)tail & 0x7))
-			tail = user_backtrace(tail, entry);
-	} else {
-#ifdef CONFIG_COMPAT
-		/* AARCH32 compat mode */
-		struct compat_frame_tail __user *tail;
-
-		tail = (struct compat_frame_tail __user *)regs->compat_fp - 1;
-
-		while ((entry->nr < entry->max_stack) &&
-			tail && !((unsigned long)tail & 0x3))
-			tail = compat_user_backtrace(tail, entry);
-#endif
-	}
-}
-
-static bool callchain_trace(void *data, unsigned long pc)
-{
-	struct perf_callchain_entry_ctx *entry = data;
-	return perf_callchain_store(entry, pc) == 0;
+	arch_stack_walk_user(callchain_trace, entry, regs);
 }
 
 void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 17f66a74c745..7f9ab5a37096 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -240,3 +240,123 @@ void show_stack(struct task_struct *tsk, unsigned long *sp, const char *loglvl)
 	dump_backtrace(NULL, tsk, loglvl);
 	barrier();
 }
+
+/*
+ * The struct defined for userspace stack frame in AARCH64 mode.
+ */
+struct frame_tail {
+	struct frame_tail	__user *fp;
+	unsigned long		lr;
+} __attribute__((packed));
+
+/*
+ * Get the return address for a single stackframe and return a pointer to the
+ * next frame tail.
+ */
+static struct frame_tail __user *
+unwind_user_frame(struct frame_tail __user *tail, void *cookie,
+	       stack_trace_consume_fn consume_entry)
+{
+	struct frame_tail buftail;
+	unsigned long err;
+	unsigned long lr;
+
+	/* Also check accessibility of one struct frame_tail beyond */
+	if (!access_ok(tail, sizeof(buftail)))
+		return NULL;
+
+	pagefault_disable();
+	err = __copy_from_user_inatomic(&buftail, tail, sizeof(buftail));
+	pagefault_enable();
+
+	if (err)
+		return NULL;
+
+	lr = ptrauth_strip_user_insn_pac(buftail.lr);
+
+	if (!consume_entry(cookie, lr))
+		return NULL;
+
+	/*
+	 * Frame pointers should strictly progress back up the stack
+	 * (towards higher addresses).
+	 */
+	if (tail >= buftail.fp)
+		return NULL;
+
+	return buftail.fp;
+}
+
+#ifdef CONFIG_COMPAT
+/*
+ * The registers we're interested in are at the end of the variable
+ * length saved register structure. The fp points at the end of this
+ * structure so the address of this struct is:
+ * (struct compat_frame_tail *)(xxx->fp)-1
+ *
+ * This code has been adapted from the ARM OProfile support.
+ */
+struct compat_frame_tail {
+	compat_uptr_t	fp; /* a (struct compat_frame_tail *) in compat mode */
+	u32		sp;
+	u32		lr;
+} __attribute__((packed));
+
+static struct compat_frame_tail __user *
+unwind_compat_user_frame(struct compat_frame_tail __user *tail, void *cookie,
+				stack_trace_consume_fn consume_entry)
+{
+	struct compat_frame_tail buftail;
+	unsigned long err;
+
+	/* Also check accessibility of one struct frame_tail beyond */
+	if (!access_ok(tail, sizeof(buftail)))
+		return NULL;
+
+	pagefault_disable();
+	err = __copy_from_user_inatomic(&buftail, tail, sizeof(buftail));
+	pagefault_enable();
+
+	if (err)
+		return NULL;
+
+	if (!consume_entry(cookie, buftail.lr))
+		return NULL;
+
+	/*
+	 * Frame pointers should strictly progress back up the stack
+	 * (towards higher addresses).
+	 */
+	if (tail + 1 >= (struct compat_frame_tail __user *)
+			compat_ptr(buftail.fp))
+		return NULL;
+
+	return (struct compat_frame_tail __user *)compat_ptr(buftail.fp) - 1;
+}
+#endif /* CONFIG_COMPAT */
+
+
+void arch_stack_walk_user(stack_trace_consume_fn consume_entry, void *cookie,
+					const struct pt_regs *regs)
+{
+	if (!consume_entry(cookie, regs->pc))
+		return;
+
+	if (!compat_user_mode(regs)) {
+		/* AARCH64 mode */
+		struct frame_tail __user *tail;
+
+		tail = (struct frame_tail __user *)regs->regs[29];
+		while (tail && !((unsigned long)tail & 0x7))
+			tail = unwind_user_frame(tail, cookie, consume_entry);
+	} else {
+#ifdef CONFIG_COMPAT
+		/* AARCH32 compat mode */
+		struct compat_frame_tail __user *tail;
+
+		tail = (struct compat_frame_tail __user *)regs->compat_fp - 1;
+		while (tail && !((unsigned long)tail & 0x3))
+			tail = unwind_compat_user_frame(tail, cookie, consume_entry);
+#endif
+	}
+}
-- 
2.25.1

Re: [RESEND v3] arm64: Add USER_STACKTRACE support

[Will Deacon]

On Tue, Dec 19, 2023 at 10:22:29AM +0800, chenqiwu wrote:
> Currently, userstacktrace is unsupported for ftrace and uprobe
> tracers on arm64. This patch uses the perf_callchain_user() code
> as blueprint to implement the arch_stack_walk_user() which add
> userstacktrace support on arm64.
> Meanwhile, we can use arch_stack_walk_user() to simplify the
> implementation of perf_callchain_user().
> This patch is tested pass with ftrace, uprobe and perf tracers
> profiling userstacktrace cases.
> 
> changes in v3:
>   - update perf_callchain_user() to use arch_stack_walk_user()
>     and delete the redundant code as Mark's suggestion in v2.
>   - update the commit message.
> 
> Tested-by: chenqiwu <qiwu.chen@transsion.com>
> Signed-off-by: chenqiwu <qiwu.chen@transsion.com>
> ---
>  arch/arm64/Kconfig                 |   1 +
>  arch/arm64/kernel/perf_callchain.c | 118 +---------------------------
>  arch/arm64/kernel/stacktrace.c     | 120 +++++++++++++++++++++++++++++
>  3 files changed, 125 insertions(+), 114 deletions(-)

This mostly looks good to me, with one potential issue:

> @@ -107,35 +25,7 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
>  		return;
>  	}
>  
> -	perf_callchain_store(entry, regs->pc);
> -
> -	if (!compat_user_mode(regs)) {
> -		/* AARCH64 mode */
> -		struct frame_tail __user *tail;
> -
> -		tail = (struct frame_tail __user *)regs->regs[29];
> -
> -		while (entry->nr < entry->max_stack &&

The old code is checking entry->nr against entry->max_stack here...

> +void arch_stack_walk_user(stack_trace_consume_fn consume_entry, void *cookie,
> +					const struct pt_regs *regs)
> +{
> +	if (!consume_entry(cookie, regs->pc))
> +		return;
> +
> +	if (!compat_user_mode(regs)) {
> +		/* AARCH64 mode */
> +		struct frame_tail __user *tail;
> +
> +		tail = (struct frame_tail __user *)regs->regs[29];
> +		while (tail && !((unsigned long)tail & 0x7))
> +			tail = unwind_user_frame(tail, cookie, consume_entry);

... but it looks like you've dropped that with the rework. Why is that ok?

Will

Re: [RESEND v3] arm64: Add USER_STACKTRACE support

[chenqiwu]

On Fri, Apr 19, 2024 at 02:09:21PM +0100, Will Deacon wrote:
> On Tue, Dec 19, 2023 at 10:22:29AM +0800, chenqiwu wrote:
> > Currently, userstacktrace is unsupported for ftrace and uprobe
> > tracers on arm64. This patch uses the perf_callchain_user() code
> > as blueprint to implement the arch_stack_walk_user() which add
> > userstacktrace support on arm64.
> > Meanwhile, we can use arch_stack_walk_user() to simplify the
> > implementation of perf_callchain_user().
> > This patch is tested pass with ftrace, uprobe and perf tracers
> > profiling userstacktrace cases.
> > 
> > changes in v3:
> >   - update perf_callchain_user() to use arch_stack_walk_user()
> >     and delete the redundant code as Mark's suggestion in v2.
> >   - update the commit message.
> > 
> > Tested-by: chenqiwu <qiwu.chen@transsion.com>
> > Signed-off-by: chenqiwu <qiwu.chen@transsion.com>
> > ---
> >  arch/arm64/Kconfig                 |   1 +
> >  arch/arm64/kernel/perf_callchain.c | 118 +---------------------------
> >  arch/arm64/kernel/stacktrace.c     | 120 +++++++++++++++++++++++++++++
> >  3 files changed, 125 insertions(+), 114 deletions(-)
> 
> This mostly looks good to me, with one potential issue:
> 
> > @@ -107,35 +25,7 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
> >  		return;
> >  	}
> >  
> > -	perf_callchain_store(entry, regs->pc);
> > -
> > -	if (!compat_user_mode(regs)) {
> > -		/* AARCH64 mode */
> > -		struct frame_tail __user *tail;
> > -
> > -		tail = (struct frame_tail __user *)regs->regs[29];
> > -
> > -		while (entry->nr < entry->max_stack &&
> 
> The old code is checking entry->nr against entry->max_stack here...
> 
> > +void arch_stack_walk_user(stack_trace_consume_fn consume_entry, void *cookie,
> > +					const struct pt_regs *regs)
> > +{
> > +	if (!consume_entry(cookie, regs->pc))
> > +		return;
> > +
> > +	if (!compat_user_mode(regs)) {
> > +		/* AARCH64 mode */
> > +		struct frame_tail __user *tail;
> > +
> > +		tail = (struct frame_tail __user *)regs->regs[29];
> > +		while (tail && !((unsigned long)tail & 0x7))
> > +			tail = unwind_user_frame(tail, cookie, consume_entry);
> 
> ... but it looks like you've dropped that with the rework. Why is that ok?
> 
It's no necessary to check entry->nr in arch_stack_walk_user(), because the caller function
stack_trace_save_user() registers the consume_entry callback for saving user stack traces into
a storage array, checking entry->nr against entry->max_stack is put into stack_trace_consume_entry().
Qiwu

Re: [RESEND v3] arm64: Add USER_STACKTRACE support

[Will Deacon]

On Wed, Apr 24, 2024 at 10:11:35PM +0800, chenqiwu wrote:
> On Fri, Apr 19, 2024 at 02:09:21PM +0100, Will Deacon wrote:
> > On Tue, Dec 19, 2023 at 10:22:29AM +0800, chenqiwu wrote:
> > > Currently, userstacktrace is unsupported for ftrace and uprobe
> > > tracers on arm64. This patch uses the perf_callchain_user() code
> > > as blueprint to implement the arch_stack_walk_user() which add
> > > userstacktrace support on arm64.
> > > Meanwhile, we can use arch_stack_walk_user() to simplify the
> > > implementation of perf_callchain_user().
> > > This patch is tested pass with ftrace, uprobe and perf tracers
> > > profiling userstacktrace cases.
> > > 
> > > changes in v3:
> > >   - update perf_callchain_user() to use arch_stack_walk_user()
> > >     and delete the redundant code as Mark's suggestion in v2.
> > >   - update the commit message.
> > > 
> > > Tested-by: chenqiwu <qiwu.chen@transsion.com>
> > > Signed-off-by: chenqiwu <qiwu.chen@transsion.com>
> > > ---
> > >  arch/arm64/Kconfig                 |   1 +
> > >  arch/arm64/kernel/perf_callchain.c | 118 +---------------------------
> > >  arch/arm64/kernel/stacktrace.c     | 120 +++++++++++++++++++++++++++++
> > >  3 files changed, 125 insertions(+), 114 deletions(-)
> > 
> > This mostly looks good to me, with one potential issue:
> > 
> > > @@ -107,35 +25,7 @@ void perf_callchain_user(struct perf_callchain_entry_ctx *entry,
> > >  		return;
> > >  	}
> > >  
> > > -	perf_callchain_store(entry, regs->pc);
> > > -
> > > -	if (!compat_user_mode(regs)) {
> > > -		/* AARCH64 mode */
> > > -		struct frame_tail __user *tail;
> > > -
> > > -		tail = (struct frame_tail __user *)regs->regs[29];
> > > -
> > > -		while (entry->nr < entry->max_stack &&
> > 
> > The old code is checking entry->nr against entry->max_stack here...
> > 
> > > +void arch_stack_walk_user(stack_trace_consume_fn consume_entry, void *cookie,
> > > +					const struct pt_regs *regs)
> > > +{
> > > +	if (!consume_entry(cookie, regs->pc))
> > > +		return;
> > > +
> > > +	if (!compat_user_mode(regs)) {
> > > +		/* AARCH64 mode */
> > > +		struct frame_tail __user *tail;
> > > +
> > > +		tail = (struct frame_tail __user *)regs->regs[29];
> > > +		while (tail && !((unsigned long)tail & 0x7))
> > > +			tail = unwind_user_frame(tail, cookie, consume_entry);
> > 
> > ... but it looks like you've dropped that with the rework. Why is that ok?
> > 
> It's no necessary to check entry->nr in arch_stack_walk_user(), because
> the caller function stack_trace_save_user() registers the consume_entry
> callback for saving user stack traces into a storage array, checking
> entry->nr against entry->max_stack is put into
> stack_trace_consume_entry().

Gotcha, and in the case of perf that same checking is done by
perf_callchain_store() for which we now check the return value.

Will

Re: [RESEND v3] arm64: Add USER_STACKTRACE support

[Will Deacon]

On Tue, 19 Dec 2023 10:22:29 +0800, chenqiwu wrote:
> Currently, userstacktrace is unsupported for ftrace and uprobe
> tracers on arm64. This patch uses the perf_callchain_user() code
> as blueprint to implement the arch_stack_walk_user() which add
> userstacktrace support on arm64.
> Meanwhile, we can use arch_stack_walk_user() to simplify the
> implementation of perf_callchain_user().
> This patch is tested pass with ftrace, uprobe and perf tracers
> profiling userstacktrace cases.
> 
> [...]

Applied to will (for-next/perf), thanks!

[1/1] arm64: Add USER_STACKTRACE support
      https://git.kernel.org/will/c/410e471f8746

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev