| Message ID | 20220103165212.9303-6-madvenka@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Reorganize the unwinder and implement stack trace reliability checks | expand |
On Mon, Jan 03, 2022 at 10:52:07AM -0600, madvenka@linux.microsoft.com wrote: > From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > > 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: Reviwed-by: Mark Brown <broonie@kernel.org>
On Mon, Jan 03, 2022 at 10:52:07AM -0600, madvenka@linux.microsoft.com wrote: > From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > > 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@linux.microsoft.com> > --- > 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 >
On 1/6/22 10:37 AM, Mark Rutland wrote: > On Mon, Jan 03, 2022 at 10:52:07AM -0600, madvenka@linux.microsoft.com wrote: >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >> >> 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. > OK. >> >> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> >> --- >> 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. > Per previous comment I agreed to, I will pass all the arguments other than task directly. Thanks. Madhavan
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); }