Re: [PATCH 2/2] arm64: stacktrace: Add skip when task == current

[Catalin Marinas]

On Wed, Mar 17, 2021 at 02:20:50PM +0000, Chen Jun wrote:
> On ARM64, cat /sys/kernel/debug/page_owner, all pages return the same
> stack:
>  stack_trace_save+0x4c/0x78
>  register_early_stack+0x34/0x70
>  init_page_owner+0x34/0x230
>  page_ext_init+0x1bc/0x1dc
> 
> The reason is that:
> check_recursive_alloc always return 1 because that
> entries[0] is always equal to ip (__set_page_owner+0x3c/0x60).
> 
> The root cause is that:
> commit 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> make the save_trace save 2 more entries.
> 
> Add skip in arch_stack_walk when task == current.
> 
> Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> Signed-off-by: Chen Jun <chenjun...@huawei.com>
> ---
>  arch/arm64/kernel/stacktrace.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index ad20981..c26b0ac 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -201,11 +201,12 @@ void arch_stack_walk(stack_trace_consume_fn 
> consume_entry, void *cookie,
>  
>       if (regs)
>               start_backtrace(&frame, regs->regs[29], regs->pc);
> -     else if (task == current)
> +     else if (task == current) {
> +             ((struct stacktrace_cookie *)cookie)->skip += 2;
>               start_backtrace(&frame,
>                               (unsigned long)__builtin_frame_address(0),
>                               (unsigned long)arch_stack_walk);
> -     else
> +     } else
>               start_backtrace(&frame, thread_saved_fp(task),
>                               thread_saved_pc(task));

I don't like abusing the cookie here. It's void * as it's meant to be an
opaque type. I'd rather skip the first two frames in walk_stackframe()
instead before invoking fn().

Prior to the conversion to ARCH_STACKWALK, we were indeed skipping two
more entries in __save_stack_trace() if tsk == current. Something like
below, completely untested:

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index ad20981dfda4..2a9f759aa41a 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -115,10 +115,15 @@ NOKPROBE_SYMBOL(unwind_frame);
 void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
                             bool (*fn)(void *, unsigned long), void *data)
 {
+       /* for the current task, we don't want this function nor its caller */
+       int skip = tsk == current ? 2 : 0;
+
        while (1) {
                int ret;
 
-               if (!fn(data, frame->pc))
+               if (skip)
+                       skip--;
+               else if (!fn(data, frame->pc))
                        break;
                ret = unwind_frame(tsk, frame);
                if (ret < 0)


-- 
Catalin