[PATCH] powerpc/stacktrace: Fix arch_stack_walk_reliable()

[PATCH] powerpc/stacktrace: Fix arch_stack_walk_reliable()

[Michael Ellerman]

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

Re: [PATCH] powerpc/stacktrace: Fix arch_stack_walk_reliable()

[Petr Mladek]

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

Re: [PATCH] powerpc/stacktrace: Fix arch_stack_walk_reliable()

[Joe Lawrence]

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

Re: [PATCH] powerpc/stacktrace: Fix arch_stack_walk_reliable()

[Michael Ellerman]

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