| Message ID | 20220117145608.6781-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 17, 2022 at 08:56:01AM -0600, madvenka@linux.microsoft.com wrote: > +/* > + * TODO: document requirements here. > + */ > +static inline void unwind_init_from_regs(struct unwind_state *state, > + struct pt_regs *regs) > +/* > + * 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_from_current(struct unwind_state *state) > +/* > + * TODO: document requirements here. > + * > + * The caller guarantees that the task is not running. > + */ > +static inline void unwind_init_from_task(struct unwind_state *state, > + struct task_struct *task) Other than the obvious gap this looks good to me. For _current() I don't think we've got any particular requirements other than what's documented. For the others I think the main thing is that trying to walk the stack of a task that is actively executing is going to be a bad idea so we should say that the task shouldn't be running, but in general given that one of the main use cases is printing diagnostics on error we shouldn't have too many *requirements* for calling these.
On 2/2/22 12:44, Mark Brown wrote: > On Mon, Jan 17, 2022 at 08:56:01AM -0600, madvenka@linux.microsoft.com wrote: > >> +/* >> + * TODO: document requirements here. >> + */ >> +static inline void unwind_init_from_regs(struct unwind_state *state, >> + struct pt_regs *regs) > >> +/* >> + * 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_from_current(struct unwind_state *state) > >> +/* >> + * TODO: document requirements here. >> + * >> + * The caller guarantees that the task is not running. >> + */ >> +static inline void unwind_init_from_task(struct unwind_state *state, >> + struct task_struct *task) > > Other than the obvious gap this looks good to me. For _current() I > don't think we've got any particular requirements other than what's > documented. For the others I think the main thing is that trying to > walk the stack of a task that is actively executing is going to be a bad > idea so we should say that the task shouldn't be running, but in general > given that one of the main use cases is printing diagnostics on error > we shouldn't have too many *requirements* for calling these. OK. For now, I will remove the TODO comment from individual functions. I will add only a common general comment above all 3 helpers that additional requirements may be documented as seen fit. And, I will add that the task must not be running in other-directed cases. Madhavan
On 2/2/22 18:26, Madhavan T. Venkataraman wrote: > > > On 2/2/22 12:44, Mark Brown wrote: >> On Mon, Jan 17, 2022 at 08:56:01AM -0600, madvenka@linux.microsoft.com wrote: >> >>> +/* >>> + * TODO: document requirements here. >>> + */ >>> +static inline void unwind_init_from_regs(struct unwind_state *state, >>> + struct pt_regs *regs) >> >>> +/* >>> + * 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_from_current(struct unwind_state *state) >> >>> +/* >>> + * TODO: document requirements here. >>> + * >>> + * The caller guarantees that the task is not running. >>> + */ >>> +static inline void unwind_init_from_task(struct unwind_state *state, >>> + struct task_struct *task) >> >> Other than the obvious gap this looks good to me. For _current() I >> don't think we've got any particular requirements other than what's >> documented. For the others I think the main thing is that trying to >> walk the stack of a task that is actively executing is going to be a bad >> idea so we should say that the task shouldn't be running, but in general >> given that one of the main use cases is printing diagnostics on error >> we shouldn't have too many *requirements* for calling these. > > OK. For now, I will remove the TODO comment from individual functions. > I will add only a common general comment above all 3 helpers that > additional requirements may be documented as seen fit. And, I will > add that the task must not be running in other-directed cases. > If what I have suggested above for comments is good enough, can I get a Reviewed-by for this? I will fix the comments on the next send. > Madhavan
On Wed, Feb 02, 2022 at 06:39:29PM -0600, Madhavan T. Venkataraman wrote: > If what I have suggested above for comments is good enough, can I get a > Reviewed-by for this? I will fix the comments on the next send. I would think so, I'll take a look at the new version.
Hi Madhavan, The diff itself largely looks good, but we need to actually write the comments. Can you pleaes pick up the wording I've written below for those? That and renaming `unwind_init_from_current` to `unwind_init_from_caller`. With those I think this is good, but I'd like to see the updated version before I provide Acked-by or Reviewed-by tags -- hopefully that's just a formality! :) On Mon, Jan 17, 2022 at 08:56:01AM -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_from_regs() - initialize from regs passed by caller. > > - unwind_init_from_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. > > This is done for two reasons: > > - the different ways of initializing are clear > > - specialized code can be added to each initializer in the future. > > Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> > --- > arch/arm64/kernel/stacktrace.c | 54 +++++++++++++++++++++++++++------- > 1 file changed, 44 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index a1a7ff93b84f..b2b568e5deba 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,46 @@ static void unwind_init(struct unwind_state *state, unsigned long fp, > state->prev_type = STACK_TYPE_UNKNOWN; > } > > +/* > + * TODO: document requirements here. > + */ Please make this: /* * Start an unwind from a pt_regs. * * The unwind will begin at the PC within the regs. * * The regs must be on a stack currently owned by the calling task. */ > +static inline void unwind_init_from_regs(struct unwind_state *state, > + struct pt_regs *regs) > +{ In future we could add: WARN_ON_ONCE(!on_accessible_stack(current, regs, sizeof(*regs), NULL)); ... to validate the requirements, but I'm happy to lave that for a future patch so this patch can be a pure refactoring. > + unwind_init_common(state); > + > + 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. > + */ Please make this: /* * Start an unwind from a caller. * * The unwind will begin at the caller of whichever function this is inlined * into. * * The function which invokes this must be noinline. */ > +static __always_inline void unwind_init_from_current(struct unwind_state *state) Can we please rename s/current/caller/ here? That way it's clear *where* in current we're unwinding from, and the fact that it's current is implicit but obvious. > +{ Similarly to unwind_init_from_regs(), in a future patch we could add: WARN_ON_ONCE(task == current); ... but for now we can omit that so this patch can be a pure refactoring. > + unwind_init_common(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. > + */ Please make this: /* * Start an unwind from a blocked task. * * The unwind will begin at the blocked tasks saved PC (i.e. the caller of * cpu_switch_to()). * * The caller should ensure the task is blocked in cpu_switch_to() for the * duration of the unwind, or the unwind will be bogus. It is never valid to * call this for the current task. */ Thanks, Mark. > +static inline void unwind_init_from_task(struct unwind_state *state, > + struct task_struct *task) > +{ > + unwind_init_common(state); > + > + state->fp = thread_saved_fp(task); > + state->pc = thread_saved_pc(task); > +} > + > /* > * Unwind from one frame record (A) to the next frame record (B). > * > @@ -195,14 +232,11 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, > struct unwind_state state; > > if (regs) > - unwind_init(&state, regs->regs[29], regs->pc); > + unwind_init_from_regs(&state, regs); > else if (task == current) > - unwind_init(&state, > - (unsigned long)__builtin_frame_address(1), > - (unsigned long)__builtin_return_address(0)); > + unwind_init_from_current(&state); > else > - unwind_init(&state, thread_saved_fp(task), > - thread_saved_pc(task)); > + unwind_init_from_task(&state, task); > > unwind(task, &state, consume_entry, cookie); > } > -- > 2.25.1 >
On 2/15/22 07:07, Mark Rutland wrote: > Hi Madhavan, > > The diff itself largely looks good, but we need to actually write the comments. > Can you pleaes pick up the wording I've written below for those? > > That and renaming `unwind_init_from_current` to `unwind_init_from_caller`. > > With those I think this is good, but I'd like to see the updated version before > I provide Acked-by or Reviewed-by tags -- hopefully that's just a formality! :) > Will do. Madhavan > On Mon, Jan 17, 2022 at 08:56:01AM -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_from_regs() - initialize from regs passed by caller. >> >> - unwind_init_from_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. >> >> This is done for two reasons: >> >> - the different ways of initializing are clear >> >> - specialized code can be added to each initializer in the future. >> >> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com> >> --- >> arch/arm64/kernel/stacktrace.c | 54 +++++++++++++++++++++++++++------- >> 1 file changed, 44 insertions(+), 10 deletions(-) >> >> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >> index a1a7ff93b84f..b2b568e5deba 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,46 @@ static void unwind_init(struct unwind_state *state, unsigned long fp, >> state->prev_type = STACK_TYPE_UNKNOWN; >> } >> >> +/* >> + * TODO: document requirements here. >> + */ > > Please make this: > > /* > * Start an unwind from a pt_regs. > * > * The unwind will begin at the PC within the regs. > * > * The regs must be on a stack currently owned by the calling task. > */ > >> +static inline void unwind_init_from_regs(struct unwind_state *state, >> + struct pt_regs *regs) >> +{ > > In future we could add: > > WARN_ON_ONCE(!on_accessible_stack(current, regs, sizeof(*regs), NULL)); > > ... to validate the requirements, but I'm happy to lave that for a future patch > so this patch can be a pure refactoring. > >> + unwind_init_common(state); >> + >> + 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. >> + */ > > Please make this: > > /* > * Start an unwind from a caller. > * > * The unwind will begin at the caller of whichever function this is inlined > * into. > * > * The function which invokes this must be noinline. > */ > >> +static __always_inline void unwind_init_from_current(struct unwind_state *state) > > Can we please rename s/current/caller/ here? That way it's clear *where* in > current we're unwinding from, and the fact that it's current is implicit but > obvious. > >> +{ > > Similarly to unwind_init_from_regs(), in a future patch we could add: > > WARN_ON_ONCE(task == current); > > ... but for now we can omit that so this patch can be a pure refactoring. > >> + unwind_init_common(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. >> + */ > > Please make this: > > /* > * Start an unwind from a blocked task. > * > * The unwind will begin at the blocked tasks saved PC (i.e. the caller of > * cpu_switch_to()). > * > * The caller should ensure the task is blocked in cpu_switch_to() for the > * duration of the unwind, or the unwind will be bogus. It is never valid to > * call this for the current task. > */ > > Thanks, > Mark. > >> +static inline void unwind_init_from_task(struct unwind_state *state, >> + struct task_struct *task) >> +{ >> + unwind_init_common(state); >> + >> + state->fp = thread_saved_fp(task); >> + state->pc = thread_saved_pc(task); >> +} >> + >> /* >> * Unwind from one frame record (A) to the next frame record (B). >> * >> @@ -195,14 +232,11 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, >> struct unwind_state state; >> >> if (regs) >> - unwind_init(&state, regs->regs[29], regs->pc); >> + unwind_init_from_regs(&state, regs); >> else if (task == current) >> - unwind_init(&state, >> - (unsigned long)__builtin_frame_address(1), >> - (unsigned long)__builtin_return_address(0)); >> + unwind_init_from_current(&state); >> else >> - unwind_init(&state, thread_saved_fp(task), >> - thread_saved_pc(task)); >> + unwind_init_from_task(&state, task); >> >> unwind(task, &state, consume_entry, cookie); >> } >> -- >> 2.25.1 >>
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c index a1a7ff93b84f..b2b568e5deba 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,46 @@ 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_from_regs(struct unwind_state *state, + struct pt_regs *regs) +{ + unwind_init_common(state); + + 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_from_current(struct unwind_state *state) +{ + unwind_init_common(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_from_task(struct unwind_state *state, + struct task_struct *task) +{ + unwind_init_common(state); + + state->fp = thread_saved_fp(task); + state->pc = thread_saved_pc(task); +} + /* * Unwind from one frame record (A) to the next frame record (B). * @@ -195,14 +232,11 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, struct unwind_state state; if (regs) - unwind_init(&state, regs->regs[29], regs->pc); + unwind_init_from_regs(&state, regs); else if (task == current) - unwind_init(&state, - (unsigned long)__builtin_frame_address(1), - (unsigned long)__builtin_return_address(0)); + unwind_init_from_current(&state); else - unwind_init(&state, thread_saved_fp(task), - thread_saved_pc(task)); + unwind_init_from_task(&state, task); unwind(task, &state, consume_entry, cookie); }