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@xxxxxxxxxx>
> Signed-off-by: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
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
