Message ID | 20230921232441.1181843-1-mpe@ellerman.id.au (mailing list archive) |
---|---|
State | Accepted |
Commit | c5cc3ca707bc916a3f326364751a41f25040aef3 |
Headers | show |
Series | powerpc/stacktrace: Fix arch_stack_walk_reliable() | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 6 jobs. |
snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
On Fri 2023-09-22 09:24:41, Michael Ellerman wrote: > The changes to copy_thread() made in commit eed7c420aac7 ("powerpc: > copy_thread differentiate kthreads and user mode threads") inadvertently > broke arch_stack_walk_reliable() because it has knowledge of the stack > layout. > > Fix it by changing the condition to match the new logic in > copy_thread(). The changes make the comments about the stack layout > incorrect, rather than rephrasing them just refer the reader to > copy_thread(). > > Also the comment about the stack backchain is no longer true, since > commit edbd0387f324 ("powerpc: copy_thread add a back chain to the > switch stack frame"), so remove that as well. > > Reported-by: Joe Lawrence <joe.lawrence@redhat.com> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > Fixes: eed7c420aac7 ("powerpc: copy_thread differentiate kthreads and user mode threads") The change makes sense to me. Well, I could not test it easily. Anyway, feel free to use: Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr
On Fri, Sep 22, 2023 at 09:24:41AM +1000, Michael Ellerman wrote: > The changes to copy_thread() made in commit eed7c420aac7 ("powerpc: > copy_thread differentiate kthreads and user mode threads") inadvertently > broke arch_stack_walk_reliable() because it has knowledge of the stack > layout. > > Fix it by changing the condition to match the new logic in > copy_thread(). The changes make the comments about the stack layout > incorrect, rather than rephrasing them just refer the reader to > copy_thread(). > > Also the comment about the stack backchain is no longer true, since > commit edbd0387f324 ("powerpc: copy_thread add a back chain to the > switch stack frame"), so remove that as well. > > Reported-by: Joe Lawrence <joe.lawrence@redhat.com> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > Fixes: eed7c420aac7 ("powerpc: copy_thread differentiate kthreads and user mode threads") > --- > arch/powerpc/kernel/stacktrace.c | 27 +++++---------------------- > 1 file changed, 5 insertions(+), 22 deletions(-) > > diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c > index b15f15dcacb5..e6a958a5da27 100644 > --- a/arch/powerpc/kernel/stacktrace.c > +++ b/arch/powerpc/kernel/stacktrace.c > @@ -73,29 +73,12 @@ int __no_sanitize_address arch_stack_walk_reliable(stack_trace_consume_fn consum > bool firstframe; > > stack_end = stack_page + THREAD_SIZE; > - if (!is_idle_task(task)) { > - /* > - * For user tasks, this is the SP value loaded on > - * kernel entry, see "PACAKSAVE(r13)" in _switch() and > - * system_call_common(). > - * > - * Likewise for non-swapper kernel threads, > - * this also happens to be the top of the stack > - * as setup by copy_thread(). > - * > - * Note that stack backlinks are not properly setup by > - * copy_thread() and thus, a forked task() will have > - * an unreliable stack trace until it's been > - * _switch()'ed to for the first time. > - */ > - stack_end -= STACK_USER_INT_FRAME_SIZE; > - } else { > - /* > - * idle tasks have a custom stack layout, > - * c.f. cpu_idle_thread_init(). > - */ > + > + // See copy_thread() for details. > + if (task->flags & PF_KTHREAD) > stack_end -= STACK_FRAME_MIN_SIZE; > - } > + else > + stack_end -= STACK_USER_INT_FRAME_SIZE; > > if (task == current) > sp = current_stack_frame(); > -- > 2.41.0 > > Reviewed-by: Joe Lawrence <joe.lawrence@redhat.com> Thanks for posting, Michael. Livepatching kselftests are happy now. Minimal kpatch testing good, too (we have not rebased our full integration tests to latest upstreams just yet). -- Joe
On Fri, 22 Sep 2023 09:24:41 +1000, Michael Ellerman wrote: > The changes to copy_thread() made in commit eed7c420aac7 ("powerpc: > copy_thread differentiate kthreads and user mode threads") inadvertently > broke arch_stack_walk_reliable() because it has knowledge of the stack > layout. > > Fix it by changing the condition to match the new logic in > copy_thread(). The changes make the comments about the stack layout > incorrect, rather than rephrasing them just refer the reader to > copy_thread(). > > [...] Applied to powerpc/fixes. [1/1] powerpc/stacktrace: Fix arch_stack_walk_reliable() https://git.kernel.org/powerpc/c/c5cc3ca707bc916a3f326364751a41f25040aef3 cheers
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c index b15f15dcacb5..e6a958a5da27 100644 --- a/arch/powerpc/kernel/stacktrace.c +++ b/arch/powerpc/kernel/stacktrace.c @@ -73,29 +73,12 @@ int __no_sanitize_address arch_stack_walk_reliable(stack_trace_consume_fn consum bool firstframe; stack_end = stack_page + THREAD_SIZE; - if (!is_idle_task(task)) { - /* - * For user tasks, this is the SP value loaded on - * kernel entry, see "PACAKSAVE(r13)" in _switch() and - * system_call_common(). - * - * Likewise for non-swapper kernel threads, - * this also happens to be the top of the stack - * as setup by copy_thread(). - * - * Note that stack backlinks are not properly setup by - * copy_thread() and thus, a forked task() will have - * an unreliable stack trace until it's been - * _switch()'ed to for the first time. - */ - stack_end -= STACK_USER_INT_FRAME_SIZE; - } else { - /* - * idle tasks have a custom stack layout, - * c.f. cpu_idle_thread_init(). - */ + + // See copy_thread() for details. + if (task->flags & PF_KTHREAD) stack_end -= STACK_FRAME_MIN_SIZE; - } + else + stack_end -= STACK_USER_INT_FRAME_SIZE; if (task == current) sp = current_stack_frame();
The changes to copy_thread() made in commit eed7c420aac7 ("powerpc: copy_thread differentiate kthreads and user mode threads") inadvertently broke arch_stack_walk_reliable() because it has knowledge of the stack layout. Fix it by changing the condition to match the new logic in copy_thread(). The changes make the comments about the stack layout incorrect, rather than rephrasing them just refer the reader to copy_thread(). Also the comment about the stack backchain is no longer true, since commit edbd0387f324 ("powerpc: copy_thread add a back chain to the switch stack frame"), so remove that as well. Reported-by: Joe Lawrence <joe.lawrence@redhat.com> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> Fixes: eed7c420aac7 ("powerpc: copy_thread differentiate kthreads and user mode threads") --- arch/powerpc/kernel/stacktrace.c | 27 +++++---------------------- 1 file changed, 5 insertions(+), 22 deletions(-)