On Mon, Jan 03, 2022 at 10:52:07AM -0600, madvenka@xxxxxxxxxxxxxxxxxxx wrote:
> From: "Madhavan T. Venkataraman" <madvenka@xxxxxxxxxxxxxxxxxxx>
>
> Copy the following arguments passed to arch_stack_walk() to unwind_state
> so that they can be passed to unwind functions via unwind_state rather
> than as separate arguments:
>
> - task
I agree the task should be placed in the unwind state, since it's a key part of
the environment for the unwind.
> - regs
This isn't relevant in all cases, and so for now I'd strongly prefer *not* to
have this in the unwind state as it's liable to lead to confusion and get
misused.
> - consume_entry
> - cookie
These are only relevant for the invocation of the consume_entry() function, and
so similarly I do not think they should be part of the state. It's simpler for
these to be local variables.
>
> Signed-off-by: Madhavan T. Venkataraman <madvenka@xxxxxxxxxxxxxxxxxxx>
> ---
> arch/arm64/include/asm/stacktrace.h | 12 ++++++++
> arch/arm64/kernel/stacktrace.c | 45 ++++++++++++++++-------------
> 2 files changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index fc828c3c5dfd..322817d40a75 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -51,6 +51,14 @@ struct stack_info {
> * @kr_cur: When KRETPOLINES is selected, holds the kretprobe instance
> * associated with the most recently encountered replacement lr
> * value.
> + *
> + * @task: Pointer to the task structure.
> + *
> + * @regs: Registers, if any.
> + *
> + * @consume_pc Consume PC function pointer.
> + *
> + * @cookie Argument to consume_pc().
> */
> struct unwind_state {
> unsigned long fp;
> @@ -61,6 +69,10 @@ struct unwind_state {
> #ifdef CONFIG_KRETPROBES
> struct llist_node *kr_cur;
> #endif
> + struct task_struct *task;
> + struct pt_regs *regs;
> + stack_trace_consume_fn consume_pc;
> + void *cookie;
> };
>
> extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index bd797e3f7789..3ecb8242caa5 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -33,8 +33,17 @@
> */
>
>
> -static void unwind_init_common(struct unwind_state *state)
> +static void unwind_init_common(struct unwind_state *state,
> + struct task_struct *task,
> + struct pt_regs *regs,
> + stack_trace_consume_fn consume_pc,
> + void *cookie)
> {
> + state->task = task;
> + state->regs = regs;
> + state->consume_pc = consume_pc;
> + state->cookie = cookie;
> +
> #ifdef CONFIG_KRETPROBES
> state->kr_cur = NULL;
> #endif
> @@ -56,11 +65,10 @@ static void unwind_init_common(struct unwind_state *state)
> /*
> * TODO: document requirements here.
> */
> -static inline void unwind_init_regs(struct unwind_state *state,
> - struct pt_regs *regs)
> +static inline void unwind_init_regs(struct unwind_state *state)
> {
> - state->fp = regs->regs[29];
> - state->pc = regs->pc;
> + state->fp = state->regs->regs[29];
> + state->pc = state->regs->pc;
> }
>
> /*
> @@ -80,11 +88,10 @@ static __always_inline void unwind_init_current(struct unwind_state *state)
> *
> * The caller guarantees that the task is not running.
> */
> -static inline void unwind_init_task(struct unwind_state *state,
> - struct task_struct *task)
> +static inline void unwind_init_task(struct unwind_state *state)
> {
> - state->fp = thread_saved_fp(task);
> - state->pc = thread_saved_pc(task);
> + state->fp = thread_saved_fp(state->task);
> + state->pc = thread_saved_pc(state->task);
> }
>
> /*
> @@ -94,9 +101,9 @@ static inline void unwind_init_task(struct unwind_state *state,
> * records (e.g. a cycle), determined based on the location and fp value of A
> * and the location (but not the fp value) of B.
> */
> -static int notrace unwind_next(struct task_struct *tsk,
> - struct unwind_state *state)
> +static int notrace unwind_next(struct unwind_state *state)
> {
> + struct task_struct *tsk = state->task;
> unsigned long fp = state->fp;
> struct stack_info info;
>
> @@ -170,16 +177,14 @@ static int notrace unwind_next(struct task_struct *tsk,
> }
> NOKPROBE_SYMBOL(unwind_next);
>
> -static void notrace unwind(struct task_struct *tsk,
> - struct unwind_state *state,
> - bool (*fn)(void *, unsigned long), void *data)
> +static void notrace unwind(struct unwind_state *state)
> {
> while (1) {
> int ret;
>
> - if (!fn(data, state->pc))
> + if (!state->consume_pc(state->cookie, state->pc))
> break;
> - ret = unwind_next(tsk, state);
> + ret = unwind_next(state);
> if (ret < 0)
> break;
> }
> @@ -225,14 +230,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
> {
> struct unwind_state state;
>
> - unwind_init_common(&state);
> + unwind_init_common(&state, task, regs, consume_entry, cookie);
>
> if (regs)
> - unwind_init_regs(&state, regs);
> + unwind_init_regs(&state);
> else if (task == current)
> unwind_init_current(&state);
> else
> - unwind_init_task(&state, task);
> + unwind_init_task(&state);
>
> - unwind(task, &state, consume_entry, cookie);
> + unwind(&state);
I don't like the changes here in particular since they hide the information
flow relevant to each case.
Thanks,
Mark.
> }
> --
> 2.25.1
>
