Re: [PATCH 2/2] riscv: stacktrace: make walk_stackframe() more robust

[Björn Töpel]

Puranjay Mohan <puranjay12@xxxxxxxxx> writes:

> Currently walk_stackframe() provides only a cookie and the PC to the
> consume_entry function. This doesn't allow the implementation of
> advanced stack walkers that need access to SP and FP as well.
>
> Change walk_stackframe to provide a struct unwind_state to the
> consume_entry function. This unwind_state has all information that is
> available to walk_stackframe. The information provided to the callback
> will not always be live/useful, the callback would be aware of the
> different configurations the information in unwind_state can be.
>
> For example: if CONFIG_FRAME_POINTER is not available, unwind_state->fp
> will always be zero.
>
> This commit doesn't make any functional changes.
>
> Signed-off-by: Puranjay Mohan <puranjay12@xxxxxxxxx>
> ---
>  arch/riscv/kernel/stacktrace.c | 55 ++++++++++++++++++++++++++++++----
>  1 file changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
> index e28f7b2e4b6a6..92c41c87b267b 100644
> --- a/arch/riscv/kernel/stacktrace.c
> +++ b/arch/riscv/kernel/stacktrace.c
> @@ -14,15 +14,26 @@
>  
>  #include <asm/stacktrace.h>
>  
> +struct unwind_state {
> +	unsigned long fp;
> +	unsigned long sp;
> +	unsigned long pc;
> +	struct pt_regs *regs;
> +	struct task_struct *task;
> +};
> +
> +typedef bool (*unwind_consume_fn)(void *cookie, const struct unwind_state *state);
> +
>  #ifdef CONFIG_FRAME_POINTER
>  
>  extern asmlinkage void ret_from_exception(void);
>  
>  static __always_inline void
>  walk_stackframe(struct task_struct *task, struct pt_regs *regs,
> -		bool (*fn)(void *, unsigned long), void *arg)
> +		unwind_consume_fn fn, void *arg)
>  {
>  	unsigned long fp, sp, pc;
> +	struct unwind_state state;
>  	int level = 0;
>  
>  	if (regs) {
> @@ -40,12 +51,17 @@ walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  		sp = task->thread.sp;
>  		pc = task->thread.ra;
>  	}
> +	state.task = task;
> +	state.regs = regs;
>  
>  	for (;;) {
>  		unsigned long low, high;
>  		struct stackframe *frame;
>  
> -		if (unlikely(!__kernel_text_address(pc) || (level++ >= 0 && !fn(arg, pc))))
> +		state.sp = sp;
> +		state.fp = fp;
> +		state.pc = pc;
> +		if (unlikely(!__kernel_text_address(pc) || (level++ >= 0 && !fn(arg, &state))))
>  			break;
>  
>  		/* Validate frame pointer */
> @@ -64,7 +80,10 @@ walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  			pc = ftrace_graph_ret_addr(current, NULL, frame->ra,
>  						   &frame->ra);
>  			if (pc == (unsigned long)ret_from_exception) {
> -				if (unlikely(!__kernel_text_address(pc) || !fn(arg, pc)))
> +				state.sp = sp;
> +				state.fp = fp;
> +				state.pc = pc;
> +				if (unlikely(!__kernel_text_address(pc) || !fn(arg, &state)))
>  					break;
>  
>  				pc = ((struct pt_regs *)sp)->epc;
> @@ -79,9 +98,10 @@ walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  
>  static __always_inline void
>  walk_stackframe(struct task_struct *task, struct pt_regs *regs,
> -		bool (*fn)(void *, unsigned long), void *arg)
> +		unwind_consume_fn fn, void *arg)
>  {
>  	unsigned long sp, pc;
> +	struct unwind_state state;
>  	unsigned long *ksp;
>  
>  	if (regs) {
> @@ -99,9 +119,14 @@ walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  	if (unlikely(sp & 0x7))
>  		return;
>  
> +	state.task = task;
> +	state.regs = regs;
> +	state.sp = sp;
> +	state.fp = 0;
>  	ksp = (unsigned long *)sp;
>  	while (!kstack_end(ksp)) {
> -		if (__kernel_text_address(pc) && unlikely(!fn(arg, pc)))
> +		state.pc = pc;
> +		if (__kernel_text_address(pc) && unlikely(!fn(arg, &state)))
>  			break;
>  		pc = READ_ONCE_NOCHECK(*ksp++) - 0x4;
>  	}
> @@ -109,10 +134,28 @@ walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  
>  #endif /* CONFIG_FRAME_POINTER */
>  
> +struct unwind_consume_entry_data {
> +	stack_trace_consume_fn consume_entry;
> +	void *cookie;
> +};
> +
> +static __always_inline bool
> +arch_unwind_consume_entry(void *cookie, const struct unwind_state *state)

Same comment as patch 1.


Björn