* [PATCH 1/2] riscv: stacktrace: use arch_stack_walk() in place of walk_stackframe
2024-03-28 18:40 [PATCH 0/2] riscv: ftrace: make stack walk more robust Puranjay Mohan
@ 2024-03-28 18:40 ` Puranjay Mohan
2024-04-02 13:19 ` Björn Töpel
2024-03-28 18:40 ` [PATCH 2/2] riscv: stacktrace: make walk_stackframe() more robust Puranjay Mohan
2024-04-02 13:18 ` [PATCH 0/2] riscv: ftrace: make stack walk " Björn Töpel
2 siblings, 1 reply; 6+ messages in thread
From: Puranjay Mohan @ 2024-03-28 18:40 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
linux-kernel, Björn Töpel
Cc: puranjay12
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] riscv: stacktrace: use arch_stack_walk() in place of walk_stackframe
2024-03-28 18:40 ` [PATCH 1/2] riscv: stacktrace: use arch_stack_walk() in place of walk_stackframe Puranjay Mohan
@ 2024-04-02 13:19 ` Björn Töpel
0 siblings, 0 replies; 6+ messages in thread
From: Björn Töpel @ 2024-04-02 13:19 UTC (permalink / raw)
To: Puranjay Mohan, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-riscv, linux-kernel, Björn Töpel
Cc: puranjay12
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] riscv: stacktrace: make walk_stackframe() more robust
2024-03-28 18:40 [PATCH 0/2] riscv: ftrace: make stack walk more robust Puranjay Mohan
2024-03-28 18:40 ` [PATCH 1/2] riscv: stacktrace: use arch_stack_walk() in place of walk_stackframe Puranjay Mohan
@ 2024-03-28 18:40 ` Puranjay Mohan
2024-04-02 13:20 ` Björn Töpel
2024-04-02 13:18 ` [PATCH 0/2] riscv: ftrace: make stack walk " Björn Töpel
2 siblings, 1 reply; 6+ messages in thread
From: Puranjay Mohan @ 2024-03-28 18:40 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv,
linux-kernel, Björn Töpel
Cc: puranjay12
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] riscv: stacktrace: make walk_stackframe() more robust
2024-03-28 18:40 ` [PATCH 2/2] riscv: stacktrace: make walk_stackframe() more robust Puranjay Mohan
@ 2024-04-02 13:20 ` Björn Töpel
0 siblings, 0 replies; 6+ messages in thread
From: Björn Töpel @ 2024-04-02 13:20 UTC (permalink / raw)
To: Puranjay Mohan, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-riscv, linux-kernel, Björn Töpel
Cc: puranjay12
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] riscv: ftrace: make stack walk more robust.
2024-03-28 18:40 [PATCH 0/2] riscv: ftrace: make stack walk more robust Puranjay Mohan
2024-03-28 18:40 ` [PATCH 1/2] riscv: stacktrace: use arch_stack_walk() in place of walk_stackframe Puranjay Mohan
2024-03-28 18:40 ` [PATCH 2/2] riscv: stacktrace: make walk_stackframe() more robust Puranjay Mohan
@ 2024-04-02 13:18 ` Björn Töpel
2 siblings, 0 replies; 6+ messages in thread
From: Björn Töpel @ 2024-04-02 13:18 UTC (permalink / raw)
To: Puranjay Mohan, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-riscv, linux-kernel, Björn Töpel
Cc: puranjay12
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>
^ permalink raw reply [flat|nested] 6+ messages in thread