[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 at transsion.com>
> > > Signed-off-by: chenqiwu <qiwu.chen at 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