[PATCH 0/2] riscv: ftrace: make stack walk more robust.

[PATCH 0/2] riscv: ftrace: make stack walk more robust.

[Puranjay Mohan]

The current stack walker in riscv implemented in walk_stackframe() provides
the PC to a callback function when it unwinds the stacks. This doesn't
allow implementing stack walkers that need access to more information like
the frame pointer, etc.

This series makes walk_stackframe() provide a unwinde_state structure to
callback functions. This structure has all the information that
walk_stackframe() can provide.

Currently, there are four users of walk_stackframe(): return_address(),
perf_callchain_kernel(), dump_backtrace(), and __get_wchan(). All of these
have been converted to use arch_stack_walk() rather than calling
walk_stackframe() directly.

We need this to implement arch_bpf_stack_walk() that provides a callback
that needs the FP, SP, and PC. This will be needed for implementing BFP
exceptions for RISCV.

There are no functional changes in this series.

I have tested this by crashing the kernel and looking at the stack trace
with and without CONFIG_FRAME_POINTER

Puranjay Mohan (2):
  riscv: stacktrace: use arch_stack_walk() in place of walk_stackframe
  riscv: stacktrace: make walk_stackframe() more robust

 arch/riscv/include/asm/stacktrace.h |  2 -
 arch/riscv/kernel/perf_callchain.c  |  2 +-
 arch/riscv/kernel/stacktrace.c      | 75 +++++++++++++++++++++++------
 3 files changed, 61 insertions(+), 18 deletions(-)

-- 
2.40.1

[PATCH 1/2] riscv: stacktrace: use arch_stack_walk() in place of walk_stackframe

[Puranjay Mohan]

Currently, dump_backtrace(), __get_wchan(), and perf_callchain_kernel()
directly call walk_stackframe(). Make then call arch_stack_walk() which
is a wrapper around walk_stackframe() and make walk_stackframe() static.

This allows making changes to walk_stackframe() without disturbing the
users of arch_stack_walk() which is the exposed function.

Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
 arch/riscv/include/asm/stacktrace.h |  2 --
 arch/riscv/kernel/perf_callchain.c  |  2 +-
 arch/riscv/kernel/stacktrace.c      | 26 ++++++++++++++------------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/riscv/include/asm/stacktrace.h b/arch/riscv/include/asm/stacktrace.h
index b1495a7e06ce6..32213e37c379f 100644
--- a/arch/riscv/include/asm/stacktrace.h
+++ b/arch/riscv/include/asm/stacktrace.h
@@ -11,8 +11,6 @@ struct stackframe {
 	unsigned long ra;
 };
 
-extern void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
-				    bool (*fn)(void *, unsigned long), void *arg);
 extern void dump_backtrace(struct pt_regs *regs, struct task_struct *task,
 			   const char *loglvl);
 
diff --git a/arch/riscv/kernel/perf_callchain.c b/arch/riscv/kernel/perf_callchain.c
index 3348a61de7d99..c023e0b1eb814 100644
--- a/arch/riscv/kernel/perf_callchain.c
+++ b/arch/riscv/kernel/perf_callchain.c
@@ -74,5 +74,5 @@ static bool fill_callchain(void *entry, unsigned long pc)
 void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
 			   struct pt_regs *regs)
 {
-	walk_stackframe(NULL, regs, fill_callchain, entry);
+	arch_stack_walk(fill_callchain, entry, NULL, regs);
 }
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index 64a9c093aef93..e28f7b2e4b6a6 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -18,8 +18,9 @@
 
 extern asmlinkage void ret_from_exception(void);
 
-void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
-			     bool (*fn)(void *, unsigned long), void *arg)
+static __always_inline void
+walk_stackframe(struct task_struct *task, struct pt_regs *regs,
+		bool (*fn)(void *, unsigned long), void *arg)
 {
 	unsigned long fp, sp, pc;
 	int level = 0;
@@ -76,8 +77,9 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
 
 #else /* !CONFIG_FRAME_POINTER */
 
-void notrace walk_stackframe(struct task_struct *task,
-	struct pt_regs *regs, bool (*fn)(void *, unsigned long), void *arg)
+static __always_inline void
+walk_stackframe(struct task_struct *task, struct pt_regs *regs,
+		bool (*fn)(void *, unsigned long), void *arg)
 {
 	unsigned long sp, pc;
 	unsigned long *ksp;
@@ -107,6 +109,12 @@ void notrace walk_stackframe(struct task_struct *task,
 
 #endif /* CONFIG_FRAME_POINTER */
 
+noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
+				      struct task_struct *task, struct pt_regs *regs)
+{
+	walk_stackframe(task, regs, consume_entry, cookie);
+}
+
 static bool print_trace_address(void *arg, unsigned long pc)
 {
 	const char *loglvl = arg;
@@ -118,7 +126,7 @@ static bool print_trace_address(void *arg, unsigned long pc)
 noinline void dump_backtrace(struct pt_regs *regs, struct task_struct *task,
 		    const char *loglvl)
 {
-	walk_stackframe(task, regs, print_trace_address, (void *)loglvl);
+	arch_stack_walk(print_trace_address, (void *)loglvl, task, regs);
 }
 
 void show_stack(struct task_struct *task, unsigned long *sp, const char *loglvl)
@@ -143,13 +151,7 @@ unsigned long __get_wchan(struct task_struct *task)
 
 	if (!try_get_task_stack(task))
 		return 0;
-	walk_stackframe(task, NULL, save_wchan, &pc);
+	arch_stack_walk(save_wchan, &pc, task, NULL);
 	put_task_stack(task);
 	return pc;
 }
-
-noinline void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
-		     struct task_struct *task, struct pt_regs *regs)
-{
-	walk_stackframe(task, regs, consume_entry, cookie);
-}
-- 
2.40.1

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

[Puranjay Mohan]

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@gmail.com>
---
 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)
+{
+	struct unwind_consume_entry_data *data = cookie;
+
+	return data->consume_entry(data->cookie, state->pc);
+}
+
 noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
 				      struct task_struct *task, struct pt_regs *regs)
 {
-	walk_stackframe(task, regs, consume_entry, cookie);
+	struct unwind_consume_entry_data data = {
+		.consume_entry = consume_entry,
+		.cookie = cookie,
+	};
+
+	walk_stackframe(task, regs, arch_unwind_consume_entry, &data);
 }
 
 static bool print_trace_address(void *arg, unsigned long pc)
-- 
2.40.1

Re: [PATCH 0/2] riscv: ftrace: make stack walk more robust.

[Björn Töpel]

Puranjay Mohan <puranjay12@gmail.com> writes:

> The current stack walker in riscv implemented in walk_stackframe() provides
> the PC to a callback function when it unwinds the stacks. This doesn't
> allow implementing stack walkers that need access to more information like
> the frame pointer, etc.
>
> This series makes walk_stackframe() provide a unwinde_state structure to
> callback functions. This structure has all the information that
> walk_stackframe() can provide.
>
> Currently, there are four users of walk_stackframe(): return_address(),
> perf_callchain_kernel(), dump_backtrace(), and __get_wchan(). All of these
> have been converted to use arch_stack_walk() rather than calling
> walk_stackframe() directly.
>
> We need this to implement arch_bpf_stack_walk() that provides a callback
> that needs the FP, SP, and PC. This will be needed for implementing BFP
> exceptions for RISCV.

Hmm, I wonder if it's easier to have these two patches as part of the
BPF exception series, instead of having the dependencies be cross-tree?

> There are no functional changes in this series.
>
> I have tested this by crashing the kernel and looking at the stack trace
> with and without CONFIG_FRAME_POINTER

I have two really minor style nits, but regardless if they're fixed or
not:

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>

Re: [PATCH 1/2] riscv: stacktrace: use arch_stack_walk() in place of walk_stackframe

[Björn Töpel]

Puranjay Mohan <puranjay12@gmail.com> writes:

> Currently, dump_backtrace(), __get_wchan(), and perf_callchain_kernel()
> directly call walk_stackframe(). Make then call arch_stack_walk() which
> is a wrapper around walk_stackframe() and make walk_stackframe() static.
>
> This allows making changes to walk_stackframe() without disturbing the
> users of arch_stack_walk() which is the exposed function.
>
> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
> ---
>  arch/riscv/include/asm/stacktrace.h |  2 --
>  arch/riscv/kernel/perf_callchain.c  |  2 +-
>  arch/riscv/kernel/stacktrace.c      | 26 ++++++++++++++------------
>  3 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/arch/riscv/include/asm/stacktrace.h b/arch/riscv/include/asm/stacktrace.h
> index b1495a7e06ce6..32213e37c379f 100644
> --- a/arch/riscv/include/asm/stacktrace.h
> +++ b/arch/riscv/include/asm/stacktrace.h
> @@ -11,8 +11,6 @@ struct stackframe {
>  	unsigned long ra;
>  };
>  
> -extern void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
> -				    bool (*fn)(void *, unsigned long), void *arg);
>  extern void dump_backtrace(struct pt_regs *regs, struct task_struct *task,
>  			   const char *loglvl);
>  
> diff --git a/arch/riscv/kernel/perf_callchain.c b/arch/riscv/kernel/perf_callchain.c
> index 3348a61de7d99..c023e0b1eb814 100644
> --- a/arch/riscv/kernel/perf_callchain.c
> +++ b/arch/riscv/kernel/perf_callchain.c
> @@ -74,5 +74,5 @@ static bool fill_callchain(void *entry, unsigned long pc)
>  void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
>  			   struct pt_regs *regs)
>  {
> -	walk_stackframe(NULL, regs, fill_callchain, entry);
> +	arch_stack_walk(fill_callchain, entry, NULL, regs);
>  }
> diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
> index 64a9c093aef93..e28f7b2e4b6a6 100644
> --- a/arch/riscv/kernel/stacktrace.c
> +++ b/arch/riscv/kernel/stacktrace.c
> @@ -18,8 +18,9 @@
>  
>  extern asmlinkage void ret_from_exception(void);
>  
> -void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
> -			     bool (*fn)(void *, unsigned long), void *arg)
> +static __always_inline void
> +walk_stackframe(struct task_struct *task, struct pt_regs *regs,
> +		bool (*fn)(void *, unsigned long), void *arg)

Really a nit, but please try to keep the return value/linkage/function
name on one line if possible.

>  {
>  	unsigned long fp, sp, pc;
>  	int level = 0;
> @@ -76,8 +77,9 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
>  
>  #else /* !CONFIG_FRAME_POINTER */
>  
> -void notrace walk_stackframe(struct task_struct *task,
> -	struct pt_regs *regs, bool (*fn)(void *, unsigned long), void *arg)
> +static __always_inline void
> +walk_stackframe(struct task_struct *task, struct pt_regs *regs,
> +		bool (*fn)(void *, unsigned long), void *arg)

...and here.


Björn

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

[Björn Töpel]

Puranjay Mohan <puranjay12@gmail.com> 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@gmail.com>
> ---
>  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