| Message ID | 20220103165212.9303-5-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:06AM -0600, madvenka@linux.microsoft.com wrote: > From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> > > unwind_init() is currently a single function that initializes all of the > unwind state. Split it into the following functions and call them > appropriately: > > - unwind_init_regs() - initialize from regs passed by caller. > > - unwind_init_current() - initialize for the current task from the > caller of arch_stack_walk(). > > - unwind_init_from_task() - initialize from the saved state of a > task other than the current task. In this case, the other > task must not be running. > > - unwind_init_common() - initialize fields that are common across > the above 3 cases. > > This is done so that specialized initialization can be added to each case > in the future. > > Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> > --- > arch/arm64/kernel/stacktrace.c | 50 +++++++++++++++++++++++++++------- > 1 file changed, 40 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index a1a7ff93b84f..bd797e3f7789 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -33,11 +33,8 @@ > */ > > > -static void unwind_init(struct unwind_state *state, unsigned long fp, > - unsigned long pc) > +static void unwind_init_common(struct unwind_state *state) > { > - state->fp = fp; > - state->pc = pc; > #ifdef CONFIG_KRETPROBES > state->kr_cur = NULL; > #endif > @@ -56,6 +53,40 @@ static void unwind_init(struct unwind_state *state, unsigned long fp, > state->prev_type = STACK_TYPE_UNKNOWN; > } > > +/* > + * TODO: document requirements here. > + */ > +static inline void unwind_init_regs(struct unwind_state *state, > + struct pt_regs *regs) > +{ > + state->fp = regs->regs[29]; > + state->pc = regs->pc; > +} When I suggested this back in: https://lore.kernel.org/linux-arm-kernel/20211123193723.12112-1-madvenka@linux.microsoft.com/T/#md91fbfe08ceab2a02d9f5c326e17997786e53208 ... my intent was that each unwind_init_from_*() helpers was the sole initializer of the structure, and the caller only had to call one function. That way it's not possible to construct an object with an erroneous combination of arguments because the prototype enforces the set of arguments, and the helper function can operate on a consistent snapshot of those arguments. So I'd much prefer that each of these helpers called unwind_init_common(), rather than leaving that to the caller to do. I don't mind if those pass arguments to unwind_init_common(), or explciitly initialize their respective fields, but I don' think the caller should have to care about unwind_init_common(). I'd also prefer the unwind_init_from*() naming I'd previously suggested, so that it's clear which direction information is flowing. > > + > +/* > + * TODO: document requirements here. > + * > + * Note: this is always inlined, and we expect our caller to be a noinline > + * function, such that this starts from our caller's caller. > + */ > +static __always_inline void unwind_init_current(struct unwind_state *state) > +{ > + state->fp = (unsigned long)__builtin_frame_address(1); > + state->pc = (unsigned long)__builtin_return_address(0); > +} > + > +/* > + * TODO: document requirements here. > + * > + * The caller guarantees that the task is not running. > + */ > +static inline void unwind_init_task(struct unwind_state *state, > + struct task_struct *task) > +{ > + state->fp = thread_saved_fp(task); > + state->pc = thread_saved_pc(task); > +} > + > /* > * Unwind from one frame record (A) to the next frame record (B). > * > @@ -194,15 +225,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, > { > struct unwind_state state; > > + unwind_init_common(&state); As above, I really don't like that the caller has to call both the common initializer and a specialized initializer here. Thanks, Mark. > + > if (regs) > - unwind_init(&state, regs->regs[29], regs->pc); > + unwind_init_regs(&state, regs); > else if (task == current) > - unwind_init(&state, > - (unsigned long)__builtin_frame_address(1), > - (unsigned long)__builtin_return_address(0)); > + unwind_init_current(&state); > else > - unwind_init(&state, thread_saved_fp(task), > - thread_saved_pc(task)); > + unwind_init_task(&state, task); > > unwind(task, &state, consume_entry, cookie); > } > -- > 2.25.1 >
On 1/6/22 10:31 AM, Mark Rutland wrote: > On Mon, Jan 03, 2022 at 10:52:06AM -0600, madvenka@linux.microsoft.com wrote: >> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com> >> >> unwind_init() is currently a single function that initializes all of the >> unwind state. Split it into the following functions and call them >> appropriately: >> >> - unwind_init_regs() - initialize from regs passed by caller. >> >> - unwind_init_current() - initialize for the current task from the >> caller of arch_stack_walk(). >> >> - unwind_init_from_task() - initialize from the saved state of a >> task other than the current task. In this case, the other >> task must not be running. >> >> - unwind_init_common() - initialize fields that are common across >> the above 3 cases. >> >> This is done so that specialized initialization can be added to each case >> in the future. >> >> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> >> --- >> arch/arm64/kernel/stacktrace.c | 50 +++++++++++++++++++++++++++------- >> 1 file changed, 40 insertions(+), 10 deletions(-) >> >> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >> index a1a7ff93b84f..bd797e3f7789 100644 >> --- a/arch/arm64/kernel/stacktrace.c >> +++ b/arch/arm64/kernel/stacktrace.c >> @@ -33,11 +33,8 @@ >> */ >> >> >> -static void unwind_init(struct unwind_state *state, unsigned long fp, >> - unsigned long pc) >> +static void unwind_init_common(struct unwind_state *state) >> { >> - state->fp = fp; >> - state->pc = pc; >> #ifdef CONFIG_KRETPROBES >> state->kr_cur = NULL; >> #endif >> @@ -56,6 +53,40 @@ static void unwind_init(struct unwind_state *state, unsigned long fp, >> state->prev_type = STACK_TYPE_UNKNOWN; >> } >> >> +/* >> + * TODO: document requirements here. >> + */ >> +static inline void unwind_init_regs(struct unwind_state *state, >> + struct pt_regs *regs) >> +{ >> + state->fp = regs->regs[29]; >> + state->pc = regs->pc; >> +} > > When I suggested this back in: > > https://lore.kernel.org/linux-arm-kernel/20211123193723.12112-1-madvenka@linux.microsoft.com/T/#md91fbfe08ceab2a02d9f5c326e17997786e53208 > > ... my intent was that each unwind_init_from_*() helpers was the sole > initializer of the structure, and the caller only had to call one function. > That way it's not possible to construct an object with an erroneous combination > of arguments because the prototype enforces the set of arguments, and the > helper function can operate on a consistent snapshot of those arguments. > > So I'd much prefer that each of these helpers called unwind_init_common(), > rather than leaving that to the caller to do. I don't mind if those pass > arguments to unwind_init_common(), or explciitly initialize their respective > fields, but I don' think the caller should have to care about unwind_init_common(). > > I'd also prefer the unwind_init_from*() naming I'd previously suggested, so > that it's clear which direction information is flowing. > OK. No problem. >> >> + >> +/* >> + * TODO: document requirements here. >> + * >> + * Note: this is always inlined, and we expect our caller to be a noinline >> + * function, such that this starts from our caller's caller. >> + */ >> +static __always_inline void unwind_init_current(struct unwind_state *state) >> +{ >> + state->fp = (unsigned long)__builtin_frame_address(1); >> + state->pc = (unsigned long)__builtin_return_address(0); >> +} >> + >> +/* >> + * TODO: document requirements here. >> + * >> + * The caller guarantees that the task is not running. >> + */ >> +static inline void unwind_init_task(struct unwind_state *state, >> + struct task_struct *task) >> +{ >> + state->fp = thread_saved_fp(task); >> + state->pc = thread_saved_pc(task); >> +} >> + >> /* >> * Unwind from one frame record (A) to the next frame record (B). >> * >> @@ -194,15 +225,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, >> { >> struct unwind_state state; >> >> + unwind_init_common(&state); > > As above, I really don't like that the caller has to call both the common > initializer and a specialized initializer here. > OK. Will change this. Thanks. Madhavan
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index a1a7ff93b84f..bd797e3f7789 100644 --- a/arch/arm64/kernel/stacktrace.c +++ b/arch/arm64/kernel/stacktrace.c @@ -33,11 +33,8 @@ */ -static void unwind_init(struct unwind_state *state, unsigned long fp, - unsigned long pc) +static void unwind_init_common(struct unwind_state *state) { - state->fp = fp; - state->pc = pc; #ifdef CONFIG_KRETPROBES state->kr_cur = NULL; #endif @@ -56,6 +53,40 @@ static void unwind_init(struct unwind_state *state, unsigned long fp, state->prev_type = STACK_TYPE_UNKNOWN; } +/* + * TODO: document requirements here. + */ +static inline void unwind_init_regs(struct unwind_state *state, + struct pt_regs *regs) +{ + state->fp = regs->regs[29]; + state->pc = regs->pc; +} + +/* + * TODO: document requirements here. + * + * Note: this is always inlined, and we expect our caller to be a noinline + * function, such that this starts from our caller's caller. + */ +static __always_inline void unwind_init_current(struct unwind_state *state) +{ + state->fp = (unsigned long)__builtin_frame_address(1); + state->pc = (unsigned long)__builtin_return_address(0); +} + +/* + * TODO: document requirements here. + * + * The caller guarantees that the task is not running. + */ +static inline void unwind_init_task(struct unwind_state *state, + struct task_struct *task) +{ + state->fp = thread_saved_fp(task); + state->pc = thread_saved_pc(task); +} + /* * Unwind from one frame record (A) to the next frame record (B). * @@ -194,15 +225,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, { struct unwind_state state; + unwind_init_common(&state); + if (regs) - unwind_init(&state, regs->regs[29], regs->pc); + unwind_init_regs(&state, regs); else if (task == current) - unwind_init(&state, - (unsigned long)__builtin_frame_address(1), - (unsigned long)__builtin_return_address(0)); + unwind_init_current(&state); else - unwind_init(&state, thread_saved_fp(task), - thread_saved_pc(task)); + unwind_init_task(&state, task); unwind(task, &state, consume_entry, cookie); }