[PATCH 0/5] ARM: Convert to ARCH_STACKWALK

[PATCH 0/5] ARM: Convert to ARCH_STACKWALK

[Li Huafei]

This series mainly updates the ARM stack trace code to use the newer and
simpler arch_stack_walk() interface. Two issues were fixed before that
(see patch 1 and 2), as well as allowing stack_trace_save_tsk() to
trace non-current tasks (see patch 3).

Li Huafei (5):
  ARM: stacktrace: Skip frame pointer boundary check for
    call_with_stack()
  ARM: stacktrace: Avoid duplicate saving of exception PC value
  ARM: stacktrace: Allow stack trace saving for non-current tasks
  ARM: stacktrace: Make stack walk callback consistent with generic code
  ARM: stacktrace: Convert stacktrace to generic ARCH_STACKWALK

 arch/arm/Kconfig                  |   1 +
 arch/arm/include/asm/stacktrace.h |   8 +-
 arch/arm/kernel/perf_callchain.c  |   9 +-
 arch/arm/kernel/return_address.c  |   9 +-
 arch/arm/kernel/stacktrace.c      | 195 +++++++++++++-----------------
 arch/arm/lib/call_with_stack.S    |   2 +
 6 files changed, 105 insertions(+), 119 deletions(-)

-- 
2.17.1

[PATCH 1/5] ARM: stacktrace: Skip frame pointer boundary check for call_with_stack()

[Li Huafei]

When using the frame pointer unwinder, it was found that the stack trace
output of stack_trace_save() is incomplete if the stack contains
call_with_stack():

 [0x7f00002c] dump_stack_task+0x2c/0x90 [hrtimer]
 [0x7f0000a0] hrtimer_hander+0x10/0x18 [hrtimer]
 [0x801a67f0] __hrtimer_run_queues+0x1b0/0x3b4
 [0x801a7350] hrtimer_run_queues+0xc4/0xd8
 [0x801a597c] update_process_times+0x3c/0x88
 [0x801b5a98] tick_periodic+0x50/0xd8
 [0x801b5bf4] tick_handle_periodic+0x24/0x84
 [0x8010ffc4] twd_handler+0x38/0x48
 [0x8017d220] handle_percpu_devid_irq+0xa8/0x244
 [0x80176e9c] generic_handle_domain_irq+0x2c/0x3c
 [0x8052e3a8] gic_handle_irq+0x7c/0x90
 [0x808ab15c] generic_handle_arch_irq+0x60/0x80
 [0x8051191c] call_with_stack+0x1c/0x20

For the frame pointer unwinder, unwind_frame() checks stackframe::fp by
stackframe::sp. Since call_with_stack() switches the SP from one stack
to another, stackframe::fp and stackframe: :sp will point to different
stacks, so we can no longer check stackframe::fp by stackframe::sp. Skip
checking stackframe::fp at this point to avoid this problem.

Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
 arch/arm/kernel/stacktrace.c   | 40 ++++++++++++++++++++++++++++------
 arch/arm/lib/call_with_stack.S |  2 ++
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index d0fa2037460a..af87040b0353 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -9,6 +9,8 @@
 #include <asm/stacktrace.h>
 #include <asm/traps.h>
 
+#include "reboot.h"
+
 #if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
 /*
  * Unwind the current stack frame and store the new register values in the
@@ -39,29 +41,53 @@
  * Note that with framepointer enabled, even the leaf functions have the same
  * prologue and epilogue, therefore we can ignore the LR value in this case.
  */
-int notrace unwind_frame(struct stackframe *frame)
+
+extern unsigned long call_with_stack_end;
+
+static int frame_pointer_check(struct stackframe *frame)
 {
 	unsigned long high, low;
 	unsigned long fp = frame->fp;
+	unsigned long pc = frame->pc;
+
+	/*
+	 * call_with_stack() is the only place we allow SP to jump from one
+	 * stack to another, with FP and SP pointing to different stacks,
+	 * skipping the FP boundary check at this point.
+	 */
+	if (pc >= (unsigned long)&call_with_stack &&
+			pc < (unsigned long)&call_with_stack_end)
+		return 0;
 
 	/* only go to a higher address on the stack */
 	low = frame->sp;
 	high = ALIGN(low, THREAD_SIZE);
 
-#ifdef CONFIG_CC_IS_CLANG
 	/* check current frame pointer is within bounds */
+#ifdef CONFIG_CC_IS_CLANG
 	if (fp < low + 4 || fp > high - 4)
 		return -EINVAL;
-
-	frame->sp = frame->fp;
-	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
-	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 4));
 #else
-	/* check current frame pointer is within bounds */
 	if (fp < low + 12 || fp > high - 4)
 		return -EINVAL;
+#endif
+
+	return 0;
+}
+
+int notrace unwind_frame(struct stackframe *frame)
+{
+	unsigned long fp = frame->fp;
+
+	if (frame_pointer_check(frame))
+		return -EINVAL;
 
 	/* restore the registers from the stack frame */
+#ifdef CONFIG_CC_IS_CLANG
+	frame->sp = frame->fp;
+	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
+	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 4));
+#else
 	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp - 12));
 	frame->sp = READ_ONCE_NOCHECK(*(unsigned long *)(fp - 8));
 	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp - 4));
diff --git a/arch/arm/lib/call_with_stack.S b/arch/arm/lib/call_with_stack.S
index 0a268a6c513c..5030d4e8d126 100644
--- a/arch/arm/lib/call_with_stack.S
+++ b/arch/arm/lib/call_with_stack.S
@@ -46,4 +46,6 @@ UNWIND( .setfp	fpreg, sp	)
 	pop	{fpreg, pc}
 UNWIND( .fnend			)
 #endif
+	.globl call_with_stack_end
+call_with_stack_end:
 ENDPROC(call_with_stack)
-- 
2.17.1

Re: [PATCH 1/5] ARM: stacktrace: Skip frame pointer boundary check for call_with_stack()

[Linus Walleij]

On Tue, Jul 12, 2022 at 4:18 AM Li Huafei <lihuafei1@huawei.com> wrote:

> When using the frame pointer unwinder, it was found that the stack trace
> output of stack_trace_save() is incomplete if the stack contains
> call_with_stack():
>
>  [0x7f00002c] dump_stack_task+0x2c/0x90 [hrtimer]
>  [0x7f0000a0] hrtimer_hander+0x10/0x18 [hrtimer]
>  [0x801a67f0] __hrtimer_run_queues+0x1b0/0x3b4
>  [0x801a7350] hrtimer_run_queues+0xc4/0xd8
>  [0x801a597c] update_process_times+0x3c/0x88
>  [0x801b5a98] tick_periodic+0x50/0xd8
>  [0x801b5bf4] tick_handle_periodic+0x24/0x84
>  [0x8010ffc4] twd_handler+0x38/0x48
>  [0x8017d220] handle_percpu_devid_irq+0xa8/0x244
>  [0x80176e9c] generic_handle_domain_irq+0x2c/0x3c
>  [0x8052e3a8] gic_handle_irq+0x7c/0x90
>  [0x808ab15c] generic_handle_arch_irq+0x60/0x80
>  [0x8051191c] call_with_stack+0x1c/0x20
>
> For the frame pointer unwinder, unwind_frame() checks stackframe::fp by
> stackframe::sp. Since call_with_stack() switches the SP from one stack
> to another, stackframe::fp and stackframe: :sp will point to different
> stacks, so we can no longer check stackframe::fp by stackframe::sp. Skip
> checking stackframe::fp at this point to avoid this problem.
>
> Signed-off-by: Li Huafei <lihuafei1@huawei.com>

Very nice catch! Took me some time to realize what was
going on here.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Nitpick below:

> +       /*
> +        * call_with_stack() is the only place we allow SP to jump from one
> +        * stack to another, with FP and SP pointing to different stacks,
> +        * skipping the FP boundary check at this point.
> +        */
> +       if (pc >= (unsigned long)&call_with_stack &&
> +                       pc < (unsigned long)&call_with_stack_end)
> +               return 0;

Can we create a local helper macro to do this, if it needs to happen
some other time?

#define ARM_PC_IN_FUNCTION(pc, func) (pc >=. ...)

Yours,
Linus Walleij

Re: [PATCH 1/5] ARM: stacktrace: Skip frame pointer boundary check for call_with_stack()

[Li Huafei]

Hi Linus, sorry for the late reply.

On 2022/7/18 16:57, Linus Walleij wrote:
> On Tue, Jul 12, 2022 at 4:18 AM Li Huafei <lihuafei1@huawei.com> wrote:
>
>> When using the frame pointer unwinder, it was found that the stack trace
>> output of stack_trace_save() is incomplete if the stack contains
>> call_with_stack():
>>
>>   [0x7f00002c] dump_stack_task+0x2c/0x90 [hrtimer]
>>   [0x7f0000a0] hrtimer_hander+0x10/0x18 [hrtimer]
>>   [0x801a67f0] __hrtimer_run_queues+0x1b0/0x3b4
>>   [0x801a7350] hrtimer_run_queues+0xc4/0xd8
>>   [0x801a597c] update_process_times+0x3c/0x88
>>   [0x801b5a98] tick_periodic+0x50/0xd8
>>   [0x801b5bf4] tick_handle_periodic+0x24/0x84
>>   [0x8010ffc4] twd_handler+0x38/0x48
>>   [0x8017d220] handle_percpu_devid_irq+0xa8/0x244
>>   [0x80176e9c] generic_handle_domain_irq+0x2c/0x3c
>>   [0x8052e3a8] gic_handle_irq+0x7c/0x90
>>   [0x808ab15c] generic_handle_arch_irq+0x60/0x80
>>   [0x8051191c] call_with_stack+0x1c/0x20
>>
>> For the frame pointer unwinder, unwind_frame() checks stackframe::fp by
>> stackframe::sp. Since call_with_stack() switches the SP from one stack
>> to another, stackframe::fp and stackframe: :sp will point to different
>> stacks, so we can no longer check stackframe::fp by stackframe::sp. Skip
>> checking stackframe::fp at this point to avoid this problem.
>>
>> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
> Very nice catch! Took me some time to realize what was
> going on here.

Yeah, it took me some time to discover the cause of the problem too.

>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!

>
> Nitpick below:
>
>> +       /*
>> +        * call_with_stack() is the only place we allow SP to jump from one
>> +        * stack to another, with FP and SP pointing to different stacks,
>> +        * skipping the FP boundary check at this point.
>> +        */
>> +       if (pc >= (unsigned long)&call_with_stack &&
>> +                       pc < (unsigned long)&call_with_stack_end)
>> +               return 0;
> Can we create a local helper macro to do this, if it needs to happen
> some other time?

Hopefully this won't come up again.:(

Maybe it would be better to define a macro when this happens?


Thanks,

Huafei

>
> #define ARM_PC_IN_FUNCTION(pc, func) (pc >=. ...)
>
> Yours,
> Linus Walleij
> .

[PATCH 2/5] ARM: stacktrace: Avoid duplicate saving of exception PC value

[Li Huafei]

Because an exception stack frame is not created in the exception entry,
save_trace() does special handling for the exception PC, but this is
only needed when CONFIG_FRAME_POINTER_UNWIND=y. When
CONFIG_ARM_UNWIND=y, unwind annotations have been added to the exception
entry and save_trace() will repeatedly save the exception PC:

    [0x7f000090] hrtimer_hander+0x8/0x10 [hrtimer]
    [0x8019ec50] __hrtimer_run_queues+0x18c/0x394
    [0x8019f760] hrtimer_run_queues+0xbc/0xd0
    [0x8019def0] update_process_times+0x34/0x80
    [0x801ad2a4] tick_periodic+0x48/0xd0
    [0x801ad3dc] tick_handle_periodic+0x1c/0x7c
    [0x8010f2e0] twd_handler+0x30/0x40
    [0x80177620] handle_percpu_devid_irq+0xa0/0x23c
    [0x801718d0] generic_handle_domain_irq+0x24/0x34
    [0x80502d28] gic_handle_irq+0x74/0x88
    [0x8085817c] generic_handle_arch_irq+0x58/0x78
    [0x80100ba8] __irq_svc+0x88/0xc8
    [0x80108114] arch_cpu_idle+0x38/0x3c
    [0x80108114] arch_cpu_idle+0x38/0x3c    <==== duplicate saved exception PC
    [0x80861bf8] default_idle_call+0x38/0x130
    [0x8015d5cc] do_idle+0x150/0x214
    [0x8015d978] cpu_startup_entry+0x18/0x1c
    [0x808589c0] rest_init+0xd8/0xdc
    [0x80c00a44] arch_post_acpi_subsys_init+0x0/0x8

We can move the special handling of the exception PC in save_trace() to
the unwind_frame() of the frame pointer unwinder.

Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
 arch/arm/include/asm/stacktrace.h |  6 +++++
 arch/arm/kernel/return_address.c  |  1 +
 arch/arm/kernel/stacktrace.c      | 38 +++++++++++++++++++------------
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 3e78f921b8b2..25282ff645fb 100644
--- a/arch/arm/include/asm/stacktrace.h
+++ b/arch/arm/include/asm/stacktrace.h
@@ -21,6 +21,9 @@ struct stackframe {
 	struct llist_node *kr_cur;
 	struct task_struct *tsk;
 #endif
+#ifdef CONFIG_UNWINDER_FRAME_POINTER
+	bool ex_frame;
+#endif
 };
 
 static __always_inline
@@ -34,6 +37,9 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
 		frame->kr_cur = NULL;
 		frame->tsk = current;
 #endif
+#ifdef CONFIG_UNWINDER_FRAME_POINTER
+		frame->ex_frame = in_entry_text(frame->pc) ? true : false;
+#endif
 }
 
 extern int unwind_frame(struct stackframe *frame);
diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
index 8aac1e10b117..38f1ea9c724d 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -47,6 +47,7 @@ void *return_address(unsigned int level)
 	frame.kr_cur = NULL;
 	frame.tsk = current;
 #endif
+	frame.ex_frame = false;
 
 	walk_stackframe(&frame, save_return_addr, &data);
 
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index af87040b0353..3acf51ee46bb 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -82,6 +82,21 @@ int notrace unwind_frame(struct stackframe *frame)
 	if (frame_pointer_check(frame))
 		return -EINVAL;
 
+	/*
+	 * When we unwind through an exception stack, include the saved PC
+	 * value into the stack trace.
+	 */
+	if (frame->ex_frame) {
+		struct pt_regs *regs = (struct pt_regs *)frame->sp;
+
+		if ((unsigned long)&regs[1] > ALIGN(frame->sp, THREAD_SIZE))
+			return -EINVAL;
+
+		frame->pc = regs->ARM_pc;
+		frame->ex_frame = false;
+		return 0;
+	}
+
 	/* restore the registers from the stack frame */
 #ifdef CONFIG_CC_IS_CLANG
 	frame->sp = frame->fp;
@@ -98,6 +113,9 @@ int notrace unwind_frame(struct stackframe *frame)
 					(void *)frame->fp, &frame->kr_cur);
 #endif
 
+	if (in_entry_text(frame->pc))
+		frame->ex_frame = true;
+
 	return 0;
 }
 #endif
@@ -128,7 +146,6 @@ static int save_trace(struct stackframe *frame, void *d)
 {
 	struct stack_trace_data *data = d;
 	struct stack_trace *trace = data->trace;
-	struct pt_regs *regs;
 	unsigned long addr = frame->pc;
 
 	if (data->no_sched_functions && in_sched_functions(addr))
@@ -139,19 +156,6 @@ static int save_trace(struct stackframe *frame, void *d)
 	}
 
 	trace->entries[trace->nr_entries++] = addr;
-
-	if (trace->nr_entries >= trace->max_entries)
-		return 1;
-
-	if (!in_entry_text(frame->pc))
-		return 0;
-
-	regs = (struct pt_regs *)frame->sp;
-	if ((unsigned long)&regs[1] > ALIGN(frame->sp, THREAD_SIZE))
-		return 0;
-
-	trace->entries[trace->nr_entries++] = regs->ARM_pc;
-
 	return trace->nr_entries >= trace->max_entries;
 }
 
@@ -193,6 +197,9 @@ static noinline void __save_stack_trace(struct task_struct *tsk,
 	frame.kr_cur = NULL;
 	frame.tsk = tsk;
 #endif
+#ifdef CONFIG_UNWINDER_FRAME_POINTER
+	frame.ex_frame = false;
+#endif
 
 	walk_stackframe(&frame, save_trace, &data);
 }
@@ -214,6 +221,9 @@ void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
 	frame.kr_cur = NULL;
 	frame.tsk = current;
 #endif
+#ifdef CONFIG_UNWINDER_FRAME_POINTER
+	frame.ex_frame = in_entry_text(frame.pc) ? true : false;
+#endif
 
 	walk_stackframe(&frame, save_trace, &data);
 }
-- 
2.17.1

Re: [PATCH 2/5] ARM: stacktrace: Avoid duplicate saving of exception PC value

[Linus Walleij]

On Tue, Jul 12, 2022 at 4:18 AM Li Huafei <lihuafei1@huawei.com> wrote:

> Because an exception stack frame is not created in the exception entry,
> save_trace() does special handling for the exception PC, but this is
> only needed when CONFIG_FRAME_POINTER_UNWIND=y. When
> CONFIG_ARM_UNWIND=y, unwind annotations have been added to the exception
> entry and save_trace() will repeatedly save the exception PC:
>
>     [0x7f000090] hrtimer_hander+0x8/0x10 [hrtimer]
>     [0x8019ec50] __hrtimer_run_queues+0x18c/0x394
>     [0x8019f760] hrtimer_run_queues+0xbc/0xd0
>     [0x8019def0] update_process_times+0x34/0x80
>     [0x801ad2a4] tick_periodic+0x48/0xd0
>     [0x801ad3dc] tick_handle_periodic+0x1c/0x7c
>     [0x8010f2e0] twd_handler+0x30/0x40
>     [0x80177620] handle_percpu_devid_irq+0xa0/0x23c
>     [0x801718d0] generic_handle_domain_irq+0x24/0x34
>     [0x80502d28] gic_handle_irq+0x74/0x88
>     [0x8085817c] generic_handle_arch_irq+0x58/0x78
>     [0x80100ba8] __irq_svc+0x88/0xc8
>     [0x80108114] arch_cpu_idle+0x38/0x3c
>     [0x80108114] arch_cpu_idle+0x38/0x3c    <==== duplicate saved exception PC
>     [0x80861bf8] default_idle_call+0x38/0x130
>     [0x8015d5cc] do_idle+0x150/0x214
>     [0x8015d978] cpu_startup_entry+0x18/0x1c
>     [0x808589c0] rest_init+0xd8/0xdc
>     [0x80c00a44] arch_post_acpi_subsys_init+0x0/0x8
>
> We can move the special handling of the exception PC in save_trace() to
> the unwind_frame() of the frame pointer unwinder.
>
> Signed-off-by: Li Huafei <lihuafei1@huawei.com>

This is another very nice patch!
Reviewed-by: Linus Waleij <linus.walleij@linaro.org>

Nitpick:

> +               if ((unsigned long)&regs[1] > ALIGN(frame->sp, THREAD_SIZE))
> +                       return -EINVAL;

It'd be nice to add a comment saying what is in regs[1] at this point
so it is easier to understand the code. Not your fault as it is just
moved code, but if you have time please add a small comment.

Yours,
Linus Walleij

Re: [PATCH 2/5] ARM: stacktrace: Avoid duplicate saving of exception PC value

[Li Huafei]

On 2022/7/18 17:01, Linus Walleij wrote:
> On Tue, Jul 12, 2022 at 4:18 AM Li Huafei <lihuafei1@huawei.com> wrote:
> 
>> Because an exception stack frame is not created in the exception entry,
>> save_trace() does special handling for the exception PC, but this is
>> only needed when CONFIG_FRAME_POINTER_UNWIND=y. When
>> CONFIG_ARM_UNWIND=y, unwind annotations have been added to the exception
>> entry and save_trace() will repeatedly save the exception PC:
>>
>>     [0x7f000090] hrtimer_hander+0x8/0x10 [hrtimer]
>>     [0x8019ec50] __hrtimer_run_queues+0x18c/0x394
>>     [0x8019f760] hrtimer_run_queues+0xbc/0xd0
>>     [0x8019def0] update_process_times+0x34/0x80
>>     [0x801ad2a4] tick_periodic+0x48/0xd0
>>     [0x801ad3dc] tick_handle_periodic+0x1c/0x7c
>>     [0x8010f2e0] twd_handler+0x30/0x40
>>     [0x80177620] handle_percpu_devid_irq+0xa0/0x23c
>>     [0x801718d0] generic_handle_domain_irq+0x24/0x34
>>     [0x80502d28] gic_handle_irq+0x74/0x88
>>     [0x8085817c] generic_handle_arch_irq+0x58/0x78
>>     [0x80100ba8] __irq_svc+0x88/0xc8
>>     [0x80108114] arch_cpu_idle+0x38/0x3c
>>     [0x80108114] arch_cpu_idle+0x38/0x3c    <==== duplicate saved exception PC
>>     [0x80861bf8] default_idle_call+0x38/0x130
>>     [0x8015d5cc] do_idle+0x150/0x214
>>     [0x8015d978] cpu_startup_entry+0x18/0x1c
>>     [0x808589c0] rest_init+0xd8/0xdc
>>     [0x80c00a44] arch_post_acpi_subsys_init+0x0/0x8
>>
>> We can move the special handling of the exception PC in save_trace() to
>> the unwind_frame() of the frame pointer unwinder.
>>
>> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
> 
> This is another very nice patch!
> Reviewed-by: Linus Waleij <linus.walleij@linaro.org>
> 
> Nitpick:
> 
>> +               if ((unsigned long)&regs[1] > ALIGN(frame->sp, THREAD_SIZE))
>> +                       return -EINVAL;
> 
> It'd be nice to add a comment saying what is in regs[1] at this point
> so it is easier to understand the code. Not your fault as it is just
> moved code, but if you have time please add a small comment.
> 

It is necessary to add the comment. This check is to ensure that 'regs +
sizeof(struct pt_regs)' (that is, &regs[1]) does not go beyond the
bottom of the stack, to avoid accessing data outside the task's stack,
see commit 40ff1ddb5570 ("ARM: 8948/1: Prevent OOB access in
stacktrace") for details. I will add a comment in the next version.

Thanks,
Huafei

> Yours,
> Linus Walleij
> .
> 

Re: [PATCH 2/5] ARM: stacktrace: Avoid duplicate saving of exception PC value

[Russell King (Oracle)]

On Tue, Jul 12, 2022 at 10:15:24AM +0800, Li Huafei wrote:
> @@ -34,6 +37,9 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
>  		frame->kr_cur = NULL;
>  		frame->tsk = current;
>  #endif
> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
> +		frame->ex_frame = in_entry_text(frame->pc) ? true : false;

in_entry_text() returns a bool, so there's no need for the ternary
operator. The same comment applies throughout this patch.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/5] ARM: stacktrace: Avoid duplicate saving of exception PC value

[Li Huafei]

On 2022/7/26 18:22, Russell King (Oracle) wrote:
> On Tue, Jul 12, 2022 at 10:15:24AM +0800, Li Huafei wrote:
>> @@ -34,6 +37,9 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
>>  		frame->kr_cur = NULL;
>>  		frame->tsk = current;
>>  #endif
>> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
>> +		frame->ex_frame = in_entry_text(frame->pc) ? true : false;
> 
> in_entry_text() returns a bool, so there's no need for the ternary
> operator. The same comment applies throughout this patch.
> 
OK, I will fix it in v3.

Thanks,
Huafei

[PATCH 3/5] ARM: stacktrace: Allow stack trace saving for non-current tasks

[Li Huafei]

The current ARM implementation of save_stack_trace_tsk() does not allow
saving stack trace for non-current tasks, which may limit the scenarios
in which stack_trace_save_tsk() can be used. Like other architectures,
or like ARM's unwind_backtrace(), we can leave it up to the caller to
ensure that the task that needs to be unwound is not running.

Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
 arch/arm/kernel/stacktrace.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 3acf51ee46bb..836420c00938 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -171,19 +171,11 @@ static noinline void __save_stack_trace(struct task_struct *tsk,
 	data.no_sched_functions = nosched;
 
 	if (tsk != current) {
-#ifdef CONFIG_SMP
-		/*
-		 * What guarantees do we have here that 'tsk' is not
-		 * running on another CPU?  For now, ignore it as we
-		 * can't guarantee we won't explode.
-		 */
-		return;
-#else
+		/* task blocked in __switch_to */
 		frame.fp = thread_saved_fp(tsk);
 		frame.sp = thread_saved_sp(tsk);
 		frame.lr = 0;		/* recovered from the stack */
 		frame.pc = thread_saved_pc(tsk);
-#endif
 	} else {
 		/* We don't want this function nor the caller */
 		data.skip += 2;
-- 
2.17.1

Re: [PATCH 3/5] ARM: stacktrace: Allow stack trace saving for non-current tasks

[Linus Walleij]

On Tue, Jul 12, 2022 at 4:18 AM Li Huafei <lihuafei1@huawei.com> wrote:

> The current ARM implementation of save_stack_trace_tsk() does not allow
> saving stack trace for non-current tasks, which may limit the scenarios
> in which stack_trace_save_tsk() can be used. Like other architectures,
> or like ARM's unwind_backtrace(), we can leave it up to the caller to
> ensure that the task that needs to be unwound is not running.
>
> Signed-off-by: Li Huafei <lihuafei1@huawei.com>

That sounds good, but:

>         if (tsk != current) {
> -#ifdef CONFIG_SMP
> -               /*
> -                * What guarantees do we have here that 'tsk' is not
> -                * running on another CPU?  For now, ignore it as we
> -                * can't guarantee we won't explode.
> -                */
> -               return;
> -#else
> +               /* task blocked in __switch_to */

The commit text is not consistent with the comment you are removing.

The commit is talking about "non-current" tasks which is one thing,
but the code is avoiding any tasks under SMP because they may be
running on another CPU. So you need to update the commit
message to say something like "non-current or running on another CPU".

If this condition will be checked at call sites in following patches,
then mention
that in the commit as well, so we know the end result is that we do
not break it,

I think Russell want to check this commit as well,

Yours,
Linus Walleij

Re: [PATCH 3/5] ARM: stacktrace: Allow stack trace saving for non-current tasks

[Li Huafei]

On 2022/7/18 17:07, Linus Walleij wrote:
> On Tue, Jul 12, 2022 at 4:18 AM Li Huafei <lihuafei1@huawei.com> wrote:
> 
>> The current ARM implementation of save_stack_trace_tsk() does not allow
>> saving stack trace for non-current tasks, which may limit the scenarios
>> in which stack_trace_save_tsk() can be used. Like other architectures,
>> or like ARM's unwind_backtrace(), we can leave it up to the caller to
>> ensure that the task that needs to be unwound is not running.
>>
>> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
> 
> That sounds good, but:
> 
>>         if (tsk != current) {
>> -#ifdef CONFIG_SMP
>> -               /*
>> -                * What guarantees do we have here that 'tsk' is not
>> -                * running on another CPU?  For now, ignore it as we
>> -                * can't guarantee we won't explode.
>> -                */
>> -               return;
>> -#else
>> +               /* task blocked in __switch_to */
> 
> The commit text is not consistent with the comment you are removing.
> 
> The commit is talking about "non-current" tasks which is one thing,
> but the code is avoiding any tasks under SMP because they may be
> running on another CPU. So you need to update the commit
> message to say something like "non-current or running on another CPU".
> 
> If this condition will be checked at call sites in following patches,
> then mention
> that in the commit as well, so we know the end result is that we do
> not break it,

The generic code stack_trace_save_tsk() does not have this check, and by
'caller' I mean the caller of stack_trace_save_tsk(), expecting the
'caller' to ensure that the task is not running. So in effect this check
has been dropped and there is no more guarantee. Sorry for not
clarifying the change here.

But can we assume that the user should know that the stacktrace is
unreliable for a task that is running on another CPU? If not, I should
remove this patch and keep the check.

Thanks,
Huafei
> 
> I think Russell want to check this commit as well,
> 
> Yours,
> Linus Walleij
> .
> 

Re: [PATCH 3/5] ARM: stacktrace: Allow stack trace saving for non-current tasks

[Russell King (Oracle)]

On Tue, Jul 26, 2022 at 05:12:39PM +0800, Li Huafei wrote:
> 
> 
> On 2022/7/18 17:07, Linus Walleij wrote:
> > On Tue, Jul 12, 2022 at 4:18 AM Li Huafei <lihuafei1@huawei.com> wrote:
> > 
> >> The current ARM implementation of save_stack_trace_tsk() does not allow
> >> saving stack trace for non-current tasks, which may limit the scenarios
> >> in which stack_trace_save_tsk() can be used. Like other architectures,
> >> or like ARM's unwind_backtrace(), we can leave it up to the caller to
> >> ensure that the task that needs to be unwound is not running.
> >>
> >> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
> > 
> > That sounds good, but:
> > 
> >>         if (tsk != current) {
> >> -#ifdef CONFIG_SMP
> >> -               /*
> >> -                * What guarantees do we have here that 'tsk' is not
> >> -                * running on another CPU?  For now, ignore it as we
> >> -                * can't guarantee we won't explode.
> >> -                */
> >> -               return;
> >> -#else
> >> +               /* task blocked in __switch_to */
> > 
> > The commit text is not consistent with the comment you are removing.
> > 
> > The commit is talking about "non-current" tasks which is one thing,
> > but the code is avoiding any tasks under SMP because they may be
> > running on another CPU. So you need to update the commit
> > message to say something like "non-current or running on another CPU".
> > 
> > If this condition will be checked at call sites in following patches,
> > then mention
> > that in the commit as well, so we know the end result is that we do
> > not break it,
> 
> The generic code stack_trace_save_tsk() does not have this check, and by
> 'caller' I mean the caller of stack_trace_save_tsk(), expecting the
> 'caller' to ensure that the task is not running. So in effect this check
> has been dropped and there is no more guarantee. Sorry for not
> clarifying the change here.

Can you prove in every case that the thread we're being asked to unwind
is not running? I don't think you can.

There are things like proc_pid_stack() in procfs and the stack traces
in sysrq-t which have attempted to unwind everything whether it's
running or not.

So no, there is no guarantee that the thread is blocked in
__switch_to().

> But can we assume that the user should know that the stacktrace is
> unreliable for a task that is running on another CPU? If not, I should
> remove this patch and keep the check.

It's not about "unreliable" stack traces, it's about the unwinder
killing the kernel.

The hint is this:

                frame.fp = thread_saved_fp(tsk);
                frame.sp = thread_saved_sp(tsk);
                frame.lr = 0;           /* recovered from the stack */
                frame.pc = thread_saved_pc(tsk);

These access the context saved by the scheduler when the task is
sleeping. When the thread is running, these saved values will be
the state when the thread last slept. However, with the thread
running, the stack could now contain any data what so ever, and
could change at any moment.

Whether the unwind-table unwinder is truely safe in such a
situation is unknown - we try to ensure that it won't do anything
stupid, but proving that is a hard task, and we've recently had
issues with the unwinder even without that.

So, allowing this feels like we're opening the door to DoS attacks
from userspace, where userspace sits there reading /proc/*/stack of
some thread running on a different CPU waiting for the kernel to
oops itself, possibly holding a lock, resulting in the system
dying.

These decisions need to be made by architecture code not generic
code, particularly where the method of unwinding is architecture
specific and thus may have criteria defining when its safe to do so.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 3/5] ARM: stacktrace: Allow stack trace saving for non-current tasks

[Li Huafei]

On 2022/7/26 17:49, Russell King (Oracle) wrote:
> On Tue, Jul 26, 2022 at 05:12:39PM +0800, Li Huafei wrote:
>>
>>
>> On 2022/7/18 17:07, Linus Walleij wrote:
>>> On Tue, Jul 12, 2022 at 4:18 AM Li Huafei <lihuafei1@huawei.com> wrote:
>>>
>>>> The current ARM implementation of save_stack_trace_tsk() does not allow
>>>> saving stack trace for non-current tasks, which may limit the scenarios
>>>> in which stack_trace_save_tsk() can be used. Like other architectures,
>>>> or like ARM's unwind_backtrace(), we can leave it up to the caller to
>>>> ensure that the task that needs to be unwound is not running.
>>>>
>>>> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
>>>
>>> That sounds good, but:
>>>
>>>>         if (tsk != current) {
>>>> -#ifdef CONFIG_SMP
>>>> -               /*
>>>> -                * What guarantees do we have here that 'tsk' is not
>>>> -                * running on another CPU?  For now, ignore it as we
>>>> -                * can't guarantee we won't explode.
>>>> -                */
>>>> -               return;
>>>> -#else
>>>> +               /* task blocked in __switch_to */
>>>
>>> The commit text is not consistent with the comment you are removing.
>>>
>>> The commit is talking about "non-current" tasks which is one thing,
>>> but the code is avoiding any tasks under SMP because they may be
>>> running on another CPU. So you need to update the commit
>>> message to say something like "non-current or running on another CPU".
>>>
>>> If this condition will be checked at call sites in following patches,
>>> then mention
>>> that in the commit as well, so we know the end result is that we do
>>> not break it,
>>
>> The generic code stack_trace_save_tsk() does not have this check, and by
>> 'caller' I mean the caller of stack_trace_save_tsk(), expecting the
>> 'caller' to ensure that the task is not running. So in effect this check
>> has been dropped and there is no more guarantee. Sorry for not
>> clarifying the change here.
> 
> Can you prove in every case that the thread we're being asked to unwind
> is not running? I don't think you can.
> 
> There are things like proc_pid_stack() in procfs and the stack traces
> in sysrq-t which have attempted to unwind everything whether it's
> running or not.
> 
> So no, there is no guarantee that the thread is blocked in
> __switch_to().
> 

Yes, I agree.

>> But can we assume that the user should know that the stacktrace is
>> unreliable for a task that is running on another CPU? If not, I should
>> remove this patch and keep the check.
> 
> It's not about "unreliable" stack traces, it's about the unwinder
> killing the kernel.
> 
> The hint is this:
> 
>                 frame.fp = thread_saved_fp(tsk);
>                 frame.sp = thread_saved_sp(tsk);
>                 frame.lr = 0;           /* recovered from the stack */
>                 frame.pc = thread_saved_pc(tsk);
> 
> These access the context saved by the scheduler when the task is
> sleeping. When the thread is running, these saved values will be
> the state when the thread last slept. However, with the thread
> running, the stack could now contain any data what so ever, and
> could change at any moment.
> 

I get it. For example, since the data on the stack is changing,
'*(unsigned long *)fp' could access any illegal address and crash the
kernel.

> Whether the unwind-table unwinder is truely safe in such a
> situation is unknown - we try to ensure that it won't do anything
> stupid, but proving that is a hard task, and we've recently had
> issues with the unwinder even without that.
> 
> So, allowing this feels like we're opening the door to DoS attacks
> from userspace, where userspace sits there reading /proc/*/stack of
> some thread running on a different CPU waiting for the kernel to
> oops itself, possibly holding a lock, resulting in the system
> dying.
> 
> These decisions need to be made by architecture code not generic
> code, particularly where the method of unwinding is architecture
> specific and thus may have criteria defining when its safe to do so.
> 

Thank you for your comments, I'll remove the patch and keep the check.

Thanks,
Huafei

[PATCH 4/5] ARM: stacktrace: Make stack walk callback consistent with generic code

[Li Huafei]

In order to use generic arch_stack_walk() code, make stack walk callback
consistent with it.

Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
 arch/arm/include/asm/stacktrace.h |  2 +-
 arch/arm/kernel/perf_callchain.c  |  9 ++++-----
 arch/arm/kernel/return_address.c  |  8 ++++----
 arch/arm/kernel/stacktrace.c      | 13 ++++++-------
 4 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 25282ff645fb..ce85d3ad5d05 100644
--- a/arch/arm/include/asm/stacktrace.h
+++ b/arch/arm/include/asm/stacktrace.h
@@ -44,7 +44,7 @@ void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
 
 extern int unwind_frame(struct stackframe *frame);
 extern void walk_stackframe(struct stackframe *frame,
-			    int (*fn)(struct stackframe *, void *), void *data);
+			    bool (*fn)(void *, unsigned long), void *data);
 extern void dump_mem(const char *lvl, const char *str, unsigned long bottom,
 		     unsigned long top);
 
diff --git a/arch/arm/kernel/perf_callchain.c b/arch/arm/kernel/perf_callchain.c
index bc6b246ab55e..7147edbe56c6 100644
--- a/arch/arm/kernel/perf_callchain.c
+++ b/arch/arm/kernel/perf_callchain.c
@@ -81,13 +81,12 @@ perf_callchain_user(struct perf_callchain_entry_ctx *entry, struct pt_regs *regs
  * whist unwinding the stackframe and is like a subroutine return so we use
  * the PC.
  */
-static int
-callchain_trace(struct stackframe *fr,
-		void *data)
+static bool
+callchain_trace(void *data, unsigned long pc)
 {
 	struct perf_callchain_entry_ctx *entry = data;
-	perf_callchain_store(entry, fr->pc);
-	return 0;
+	perf_callchain_store(entry, pc);
+	return true;
 }
 
 void
diff --git a/arch/arm/kernel/return_address.c b/arch/arm/kernel/return_address.c
index 38f1ea9c724d..ac15db66df4c 100644
--- a/arch/arm/kernel/return_address.c
+++ b/arch/arm/kernel/return_address.c
@@ -16,17 +16,17 @@ struct return_address_data {
 	void *addr;
 };
 
-static int save_return_addr(struct stackframe *frame, void *d)
+static bool save_return_addr(void *d, unsigned long pc)
 {
 	struct return_address_data *data = d;
 
 	if (!data->level) {
-		data->addr = (void *)frame->pc;
+		data->addr = (void *)pc;
 
-		return 1;
+		return false;
 	} else {
 		--data->level;
-		return 0;
+		return true;
 	}
 }
 
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 836420c00938..ec0ca3192775 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -121,12 +121,12 @@ int notrace unwind_frame(struct stackframe *frame)
 #endif
 
 void notrace walk_stackframe(struct stackframe *frame,
-		     int (*fn)(struct stackframe *, void *), void *data)
+		     bool (*fn)(void *, unsigned long), void *data)
 {
 	while (1) {
 		int ret;
 
-		if (fn(frame, data))
+		if (!fn(data, frame->pc))
 			break;
 		ret = unwind_frame(frame);
 		if (ret < 0)
@@ -142,21 +142,20 @@ struct stack_trace_data {
 	unsigned int skip;
 };
 
-static int save_trace(struct stackframe *frame, void *d)
+static bool save_trace(void *d, unsigned long addr)
 {
 	struct stack_trace_data *data = d;
 	struct stack_trace *trace = data->trace;
-	unsigned long addr = frame->pc;
 
 	if (data->no_sched_functions && in_sched_functions(addr))
-		return 0;
+		return true;
 	if (data->skip) {
 		data->skip--;
-		return 0;
+		return true;
 	}
 
 	trace->entries[trace->nr_entries++] = addr;
-	return trace->nr_entries >= trace->max_entries;
+	return trace->nr_entries < trace->max_entries;
 }
 
 /* This must be noinline to so that our skip calculation works correctly */
-- 
2.17.1

Re: [PATCH 4/5] ARM: stacktrace: Make stack walk callback consistent with generic code

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

On Tue, Jul 12, 2022 at 10:15:26AM +0800, Li Huafei wrote:

> In order to use generic arch_stack_walk() code, make stack walk callback
> consistent with it.

It might be useful to say what the changes are here, if nothing else
that makes it easier to review and confirm that the changes are doing
what you intend them to.  See my conversion for arm64 for an example.
The actual changes here seem OK though

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/5] ARM: stacktrace: Make stack walk callback consistent with generic code

[Li Huafei]

On 2022/7/12 21:34, Mark Brown wrote:
> On Tue, Jul 12, 2022 at 10:15:26AM +0800, Li Huafei wrote:
>
>> In order to use generic arch_stack_walk() code, make stack walk callback
>> consistent with it.
> It might be useful to say what the changes are here, if nothing else
> that makes it easier to review and confirm that the changes are doing
> what you intend them to.  See my conversion for arm64 for an example.


Thanks for the advice. I've looked at the commit:

   baa2cd417053 ("arm64: stacktrace: Make stack walk callback consistent 
with generic code")

The description is very clear. It describes the change in detail and
gives a reason for making that change a separate commit. The same
description applies to the current changes, so I took the commit message
directly, just made the s/arm64/ARM/ changes, and updated to v2:

https://lore.kernel.org/lkml/20220713110020.85511-1-lihuafei1@huawei.com/


> The actual changes here seem OK though
>
> Reviewed-by: Mark Brown <broonie@kernel.org>

Thanks for the review!


Thanks,

Huafei

Re: [PATCH 4/5] ARM: stacktrace: Make stack walk callback consistent with generic code

[Linus Walleij]

On Tue, Jul 12, 2022 at 4:19 AM Li Huafei <lihuafei1@huawei.com> wrote:

> In order to use generic arch_stack_walk() code, make stack walk callback
> consistent with it.
>
> Signed-off-by: Li Huafei <lihuafei1@huawei.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 4/5] ARM: stacktrace: Make stack walk callback consistent with generic code

[Li Huafei]

On 2022/7/18 17:09, Linus Walleij wrote:
> On Tue, Jul 12, 2022 at 4:19 AM Li Huafei <lihuafei1@huawei.com> wrote:
> 
>> In order to use generic arch_stack_walk() code, make stack walk callback
>> consistent with it.
>>
>> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!

> 
> Yours,
> Linus Walleij
> .
> 

[PATCH 5/5] ARM: stacktrace: Convert stacktrace to generic ARCH_STACKWALK

[Li Huafei]

This patch converts ARM stacktrace to the generic ARCH_STACKWALK
implemented by commit 214d8ca6ee85 ("stacktrace: Provide common
infrastructure").

Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
 arch/arm/Kconfig             |   1 +
 arch/arm/kernel/stacktrace.c | 114 ++++++++++-------------------------
 2 files changed, 33 insertions(+), 82 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 7630ba9cb6cc..8da192853562 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -18,6 +18,7 @@ config ARM
 	select ARCH_HAS_PHYS_TO_DMA
 	select ARCH_HAS_SETUP_DMA_OPS
 	select ARCH_HAS_SET_MEMORY
+	select ARCH_STACKWALK
 	select ARCH_HAS_STRICT_KERNEL_RWX if MMU && !XIP_KERNEL
 	select ARCH_HAS_STRICT_MODULE_RWX if MMU
 	select ARCH_HAS_SYNC_DMA_FOR_DEVICE if SWIOTLB || !MMU
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index ec0ca3192775..1b9c91e14d2e 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -136,98 +136,48 @@ void notrace walk_stackframe(struct stackframe *frame,
 EXPORT_SYMBOL(walk_stackframe);
 
 #ifdef CONFIG_STACKTRACE
-struct stack_trace_data {
-	struct stack_trace *trace;
-	unsigned int no_sched_functions;
-	unsigned int skip;
-};
-
-static bool save_trace(void *d, unsigned long addr)
-{
-	struct stack_trace_data *data = d;
-	struct stack_trace *trace = data->trace;
-
-	if (data->no_sched_functions && in_sched_functions(addr))
-		return true;
-	if (data->skip) {
-		data->skip--;
-		return true;
-	}
-
-	trace->entries[trace->nr_entries++] = addr;
-	return trace->nr_entries < trace->max_entries;
-}
-
-/* This must be noinline to so that our skip calculation works correctly */
-static noinline void __save_stack_trace(struct task_struct *tsk,
-	struct stack_trace *trace, unsigned int nosched)
+static void start_stack_trace(struct stackframe *frame, struct task_struct *task,
+			      unsigned long fp, unsigned long sp,
+			      unsigned long lr, unsigned long pc)
 {
-	struct stack_trace_data data;
-	struct stackframe frame;
-
-	data.trace = trace;
-	data.skip = trace->skip;
-	data.no_sched_functions = nosched;
-
-	if (tsk != current) {
-		/* task blocked in __switch_to */
-		frame.fp = thread_saved_fp(tsk);
-		frame.sp = thread_saved_sp(tsk);
-		frame.lr = 0;		/* recovered from the stack */
-		frame.pc = thread_saved_pc(tsk);
-	} else {
-		/* We don't want this function nor the caller */
-		data.skip += 2;
-		frame.fp = (unsigned long)__builtin_frame_address(0);
-		frame.sp = current_stack_pointer;
-		frame.lr = (unsigned long)__builtin_return_address(0);
-here:
-		frame.pc = (unsigned long)&&here;
-	}
+	frame->fp = fp;
+	frame->sp = sp;
+	frame->lr = lr;
+	frame->pc = pc;
 #ifdef CONFIG_KRETPROBES
-	frame.kr_cur = NULL;
-	frame.tsk = tsk;
+	frame->kr_cur = NULL;
+	frame->tsk = task;
 #endif
 #ifdef CONFIG_UNWINDER_FRAME_POINTER
-	frame.ex_frame = false;
+	frame->ex_frame = in_entry_text(frame->pc) ? true : false;
 #endif
-
-	walk_stackframe(&frame, save_trace, &data);
 }
 
-void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
+void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
+		     struct task_struct *task, struct pt_regs *regs)
 {
-	struct stack_trace_data data;
 	struct stackframe frame;
 
-	data.trace = trace;
-	data.skip = trace->skip;
-	data.no_sched_functions = 0;
-
-	frame.fp = regs->ARM_fp;
-	frame.sp = regs->ARM_sp;
-	frame.lr = regs->ARM_lr;
-	frame.pc = regs->ARM_pc;
-#ifdef CONFIG_KRETPROBES
-	frame.kr_cur = NULL;
-	frame.tsk = current;
-#endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
-	frame.ex_frame = in_entry_text(frame.pc) ? true : false;
-#endif
-
-	walk_stackframe(&frame, save_trace, &data);
-}
-
-void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
-{
-	__save_stack_trace(tsk, trace, 1);
-}
-EXPORT_SYMBOL(save_stack_trace_tsk);
+	if (regs) {
+		start_stack_trace(&frame, NULL, regs->ARM_fp, regs->ARM_sp,
+				  regs->ARM_lr, regs->ARM_pc);
+	} else if (task != current) {
+		/* task blocked in __switch_to */
+		start_stack_trace(&frame, task, thread_saved_fp(task),
+				  thread_saved_sp(task), 0,
+				  thread_saved_pc(task));
+	} else {
+here:
+		start_stack_trace(&frame, task,
+				  (unsigned long)__builtin_frame_address(0),
+				  current_stack_pointer,
+				  (unsigned long)__builtin_return_address(0),
+				  (unsigned long)&&here);
+		/* skip this function */
+		if (unwind_frame(&frame))
+			return;
+	}
 
-void save_stack_trace(struct stack_trace *trace)
-{
-	__save_stack_trace(current, trace, 0);
+	walk_stackframe(&frame, consume_entry, cookie);
 }
-EXPORT_SYMBOL_GPL(save_stack_trace);
 #endif
-- 
2.17.1

Re: [PATCH 5/5] ARM: stacktrace: Convert stacktrace to generic ARCH_STACKWALK

[Linus Walleij]

On Tue, Jul 12, 2022 at 4:19 AM Li Huafei <lihuafei1@huawei.com> wrote:

> This patch converts ARM stacktrace to the generic ARCH_STACKWALK
> implemented by commit 214d8ca6ee85 ("stacktrace: Provide common
> infrastructure").
>
> Signed-off-by: Li Huafei <lihuafei1@huawei.com>

Looks good to me:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

What I want to know is if this commit will avoid the problem mentioned
in review of commit 3? I.e. the generic stackwalk code will make sure we are
not running the task on another CPU, so that is why we could remove
that check?

Yours,
Linus Walleij

Re: [PATCH 5/5] ARM: stacktrace: Convert stacktrace to generic ARCH_STACKWALK

[Li Huafei]

On 2022/7/18 17:12, Linus Walleij wrote:
> On Tue, Jul 12, 2022 at 4:19 AM Li Huafei <lihuafei1@huawei.com> wrote:
> 
>> This patch converts ARM stacktrace to the generic ARCH_STACKWALK
>> implemented by commit 214d8ca6ee85 ("stacktrace: Provide common
>> infrastructure").
>>
>> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
> 
> Looks good to me:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> What I want to know is if this commit will avoid the problem mentioned
> in review of commit 3? I.e. the generic stackwalk code will make sure we are
> not running the task on another CPU, so that is why we could remove
> that check?
> 

It was explained in commit 3, thank you very much.

Thanks,
Huafei

> Yours,
> Linus Walleij
> .
> 

Re: [PATCH 5/5] ARM: stacktrace: Convert stacktrace to generic ARCH_STACKWALK

[Li Huafei]

Hi Linus,

On 2022/7/18 17:12, Linus Walleij wrote:
> On Tue, Jul 12, 2022 at 4:19 AM Li Huafei <lihuafei1@huawei.com> wrote:
> 
>> This patch converts ARM stacktrace to the generic ARCH_STACKWALK
>> implemented by commit 214d8ca6ee85 ("stacktrace: Provide common
>> infrastructure").
>>
>> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
> 
> Looks good to me:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> What I want to know is if this commit will avoid the problem mentioned
> in review of commit 3? I.e. the generic stackwalk code will make sure we are
> not running the task on another CPU, so that is why we could remove
> that check?
> 

In v3, I removed patch 3 of v1 and kept that check, see

  https://lore.kernel.org/lkml/20220727040022.139387-1-lihuafei1@huawei.com/

Given this change, I did not add your reviewed-by to patch 4 of v3. If 
you think patch 4 of v3 is still ok, please do let me know. Thank you 
very much!

Thanks,
Huafei

> Yours,
> Linus Walleij
> .
>