| Message ID | 20231219022229.10230-1-qiwu.chen@transsion.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,v3] arm64: Add USER_STACKTRACE support | expand |
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
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
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
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,
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 + } +}
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(-)