On Tue, Oct 13, 2020 at 12:42:30PM +0100, Mark Rutland wrote:
> On Mon, Oct 12, 2020 at 06:26:05PM +0100, Mark Brown wrote:

> > +/*
> > + * This function returns an error if it detects any unreliable features of the
> > + * stack.  Otherwise it guarantees that the stack trace is reliable.
> > + *
> > + * If the task is not 'current', the caller *must* ensure the task is inactive.
> > + */

> Is the caller responsible for pinning a non-current task's stack? e.g.
> in dump_backtrace() we do that with try_get_task_stack(). If so, it
> might be worth making the comment say "the task is inactive and its
> stack is pinned".

Yes, this is done in the generic code by stack_trace_save_reliable()
which should be the only user of this function.  TBH we should probably
move this comment to the prototype in the header rather than duplicating
it in each architecture as is currently done, I was being a bit lazy
there.

> > +int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
> > +			     void *cookie, struct task_struct *task)
> > +{
> > +	struct stackframe frame;
> > +
> > +	if (task == current)
> > +		start_backtrace(&frame,
> > +				(unsigned long)__builtin_frame_address(0),
> > +				(unsigned long)arch_stack_walk_reliable);
> > +	else
> > +		start_backtrace(&frame, thread_saved_fp(task),
> > +				thread_saved_pc(task));
> > +

> Codestyle nit: as these spread over multiple lines the if-else clauses
> should have braces.

Usage appears to be a bit varied there for single statements :/