linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RESEND v3 0/4] ARM: Convert to ARCH_STACKWALK
@ 2022-08-26  7:40 Li Huafei
  2022-08-26  7:40 ` [PATCH RESEND v3 1/4] ARM: stacktrace: Skip frame pointer boundary check for call_with_stack() Li Huafei
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Li Huafei @ 2022-08-26  7:40 UTC (permalink / raw)
  To: linux, rmk+kernel, ardb, will, broonie, linus.walleij
  Cc: mark.rutland, peterz, mingo, acme, alexander.shishkin, jolsa,
	namhyung, arnd, rostedt, nick.hawkins, john, mhiramat, ast,
	linyujun809, ndesaulniers, lihuafei1, linux-arm-kernel,
	linux-kernel, linux-perf-users

This series mainly updates the ARM stack trace code to use the newer and
simpler arch_stack_walk() interface (see patches 3 and 4). Two issues
were fixed before that (see patches 1 and 2).

v3 RESEND notes:

Rebase to v6.0-rc1.

It's been a month since I sent the v3. Linus told me I needed to add the
patch to Russell's patch tracker myself, rebase to v6.0-rc1 before that.
Thanks Linus for the tip!

Since commit af6f23b88e95 ("ARM/dma-mapping: use the generic versions of
dma_to_phys/phys_to_dma by default") and commit ae626eb97376
("ARM/dma-mapping: use dma-direct unconditionally") modified
arch/arm/Kconfig, there is a minor conflict with patch 4 when rebase.

v3: https://lore.kernel.org/lkml/20220727040022.139387-1-lihuafei1@huawei.com/
 - According to the discussion with Linus and Russell in v1:
   - Add a comment about "regs[1]" in patch 2, and remove the
     unnecessary ternary operator in the initialization of
     "frame->ex_frame".
   - Remove the patch "ARM: stacktrace: Allow stack trace saving for
     non-current tasks", and keep the check for not being able to
     unwind non-current tasks (including tasks running on other CPUs)
     when CONFIG_SMP=y in patch 4.
 - Rebase to linux-5.19-rc8.

v2: https://lore.kernel.org/lkml/20220713110020.85511-1-lihuafei1@huawei.com/
 - As suggested by Mark, the commit logs for patch 4 and 5 were
   refined for easy review.

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

Li Huafei (4):
  ARM: stacktrace: Skip frame pointer boundary check for
    call_with_stack()
  ARM: stacktrace: Avoid duplicate saving of exception PC value
  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      | 191 ++++++++++++++----------------
 arch/arm/lib/call_with_stack.S    |   2 +
 6 files changed, 110 insertions(+), 110 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH RESEND v3 1/4] ARM: stacktrace: Skip frame pointer boundary check for call_with_stack()
  2022-08-26  7:40 [PATCH RESEND v3 0/4] ARM: Convert to ARCH_STACKWALK Li Huafei
@ 2022-08-26  7:40 ` Li Huafei
  2022-08-26  7:40 ` [PATCH RESEND v3 2/4] ARM: stacktrace: Avoid duplicate saving of exception PC value Li Huafei
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Li Huafei @ 2022-08-26  7:40 UTC (permalink / raw)
  To: linux, rmk+kernel, ardb, will, broonie, linus.walleij
  Cc: mark.rutland, peterz, mingo, acme, alexander.shishkin, jolsa,
	namhyung, arnd, rostedt, nick.hawkins, john, mhiramat, ast,
	linyujun809, ndesaulniers, lihuafei1, linux-arm-kernel,
	linux-kernel, linux-perf-users

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>
Reviewed-by: Linus Waleij <linus.walleij@linaro.org>
---
 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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH RESEND v3 2/4] ARM: stacktrace: Avoid duplicate saving of exception PC value
  2022-08-26  7:40 [PATCH RESEND v3 0/4] ARM: Convert to ARCH_STACKWALK Li Huafei
  2022-08-26  7:40 ` [PATCH RESEND v3 1/4] ARM: stacktrace: Skip frame pointer boundary check for call_with_stack() Li Huafei
@ 2022-08-26  7:40 ` Li Huafei
  2022-08-26  7:40 ` [PATCH RESEND v3 3/4] ARM: stacktrace: Make stack walk callback consistent with generic code Li Huafei
  2022-08-26  7:40 ` [PATCH RESEND v3 4/4] ARM: stacktrace: Convert stacktrace to generic ARCH_STACKWALK Li Huafei
  3 siblings, 0 replies; 5+ messages in thread
From: Li Huafei @ 2022-08-26  7:40 UTC (permalink / raw)
  To: linux, rmk+kernel, ardb, will, broonie, linus.walleij
  Cc: mark.rutland, peterz, mingo, acme, alexander.shishkin, jolsa,
	namhyung, arnd, rostedt, nick.hawkins, john, mhiramat, ast,
	linyujun809, ndesaulniers, lihuafei1, linux-arm-kernel,
	linux-kernel, linux-perf-users

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>
Reviewed-by: Linus Waleij <linus.walleij@linaro.org>
---
 arch/arm/include/asm/stacktrace.h |  6 +++++
 arch/arm/kernel/return_address.c  |  1 +
 arch/arm/kernel/stacktrace.c      | 44 +++++++++++++++++++++----------
 3 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 3e78f921b8b2..39be2d1aa27b 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);
+#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..85443b5d1922 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -82,6 +82,27 @@ 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;
+
+		/*
+		 * We check that 'regs + sizeof(struct pt_regs)' (that is,
+		 * &regs[1]) does not exceed the bottom of the stack to avoid
+		 * accessing data outside the task's stack. This may happen
+		 * when frame->ex_frame is a false positive.
+		 */
+		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 +119,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 +152,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 +162,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 +203,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 +227,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);
+#endif
 
 	walk_stackframe(&frame, save_trace, &data);
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH RESEND v3 3/4] ARM: stacktrace: Make stack walk callback consistent with generic code
  2022-08-26  7:40 [PATCH RESEND v3 0/4] ARM: Convert to ARCH_STACKWALK Li Huafei
  2022-08-26  7:40 ` [PATCH RESEND v3 1/4] ARM: stacktrace: Skip frame pointer boundary check for call_with_stack() Li Huafei
  2022-08-26  7:40 ` [PATCH RESEND v3 2/4] ARM: stacktrace: Avoid duplicate saving of exception PC value Li Huafei
@ 2022-08-26  7:40 ` Li Huafei
  2022-08-26  7:40 ` [PATCH RESEND v3 4/4] ARM: stacktrace: Convert stacktrace to generic ARCH_STACKWALK Li Huafei
  3 siblings, 0 replies; 5+ messages in thread
From: Li Huafei @ 2022-08-26  7:40 UTC (permalink / raw)
  To: linux, rmk+kernel, ardb, will, broonie, linus.walleij
  Cc: mark.rutland, peterz, mingo, acme, alexander.shishkin, jolsa,
	namhyung, arnd, rostedt, nick.hawkins, john, mhiramat, ast,
	linyujun809, ndesaulniers, lihuafei1, linux-arm-kernel,
	linux-kernel, linux-perf-users

As with the generic arch_stack_walk() code the ARM stack walk code takes
a callback that is called per stack frame. Currently the ARM code always
passes a struct stackframe to the callback and the generic code just
passes the pc, however none of the users ever reference anything in the
struct other than the pc value. The ARM code also uses a return type of
int while the generic code uses a return type of bool though in both
cases the return value is a boolean value and the sense is inverted
between the two.

In order to reduce code duplication when ARM is converted to use
arch_stack_walk() change the signature and return sense of the ARM
specific callback to match that of the generic code.

Signed-off-by: Li Huafei <lihuafei1@huawei.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Linus Waleij <linus.walleij@linaro.org>
---
 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 39be2d1aa27b..7269d1e13449 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 85443b5d1922..d05968bc7812 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -127,12 +127,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)
@@ -148,21 +148,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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH RESEND v3 4/4] ARM: stacktrace: Convert stacktrace to generic ARCH_STACKWALK
  2022-08-26  7:40 [PATCH RESEND v3 0/4] ARM: Convert to ARCH_STACKWALK Li Huafei
                   ` (2 preceding siblings ...)
  2022-08-26  7:40 ` [PATCH RESEND v3 3/4] ARM: stacktrace: Make stack walk callback consistent with generic code Li Huafei
@ 2022-08-26  7:40 ` Li Huafei
  3 siblings, 0 replies; 5+ messages in thread
From: Li Huafei @ 2022-08-26  7:40 UTC (permalink / raw)
  To: linux, rmk+kernel, ardb, will, broonie, linus.walleij
  Cc: mark.rutland, peterz, mingo, acme, alexander.shishkin, jolsa,
	namhyung, arnd, rostedt, nick.hawkins, john, mhiramat, ast,
	linyujun809, ndesaulniers, lihuafei1, linux-arm-kernel,
	linux-kernel, linux-perf-users

Historically architectures have had duplicated code in their stack trace
implementations for filtering what gets traced. In order to avoid this
duplication some generic code has been provided using a new interface
arch_stack_walk(), enabled by selecting ARCH_STACKWALK in Kconfig, which
factors all this out into the generic stack trace code. Convert ARM to
use this common infrastructure.

When initializing the stack frame of the current task, arm64 uses
__builtin_frame_address(1) to initialize the frame pointer, skipping
arch_stack_walk(), see the commit c607ab4f916d ("arm64: stacktrace:
don't trace arch_ stack_walk()"). Since __builtin_frame_address(1) does
not work on ARM, unwind_frame() is used to unwind the stack one layer
forward before calling walk_stackframe().

Signed-off-by: Li Huafei <lihuafei1@huawei.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 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 87badeae3181..3a8abc84260a 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -17,6 +17,7 @@ config ARM
 	select ARCH_HAS_PTE_SPECIAL if ARM_LPAE
 	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
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index d05968bc7812..620aa82e3bdd 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -142,40 +142,32 @@ 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)
+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 = 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;
+	frame->fp = fp;
+	frame->sp = sp;
+	frame->lr = lr;
+	frame->pc = pc;
+#ifdef CONFIG_KRETPROBES
+	frame->kr_cur = NULL;
+	frame->tsk = task;
+#endif
+#ifdef CONFIG_UNWINDER_FRAME_POINTER
+	frame->ex_frame = in_entry_text(frame->pc);
+#endif
 }
 
-/* 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)
+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 = nosched;
-
-	if (tsk != current) {
+	if (regs) {
+		start_stack_trace(&frame, NULL, regs->ARM_fp, regs->ARM_sp,
+				  regs->ARM_lr, regs->ARM_pc);
+	} else if (task != current) {
 #ifdef CONFIG_SMP
 		/*
 		 * What guarantees do we have here that 'tsk' is not
@@ -184,64 +176,22 @@ static noinline void __save_stack_trace(struct task_struct *tsk,
 		 */
 		return;
 #else
-		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);
+		start_stack_trace(&frame, task, thread_saved_fp(task),
+				  thread_saved_sp(task), 0,
+				  thread_saved_pc(task));
 #endif
 	} 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;
+		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;
 	}
-#ifdef CONFIG_KRETPROBES
-	frame.kr_cur = NULL;
-	frame.tsk = tsk;
-#endif
-#ifdef CONFIG_UNWINDER_FRAME_POINTER
-	frame.ex_frame = false;
-#endif
 
-	walk_stackframe(&frame, save_trace, &data);
-}
-
-void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
-{
-	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);
-#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);
-
-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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-08-26  7:44 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-26  7:40 [PATCH RESEND v3 0/4] ARM: Convert to ARCH_STACKWALK Li Huafei
2022-08-26  7:40 ` [PATCH RESEND v3 1/4] ARM: stacktrace: Skip frame pointer boundary check for call_with_stack() Li Huafei
2022-08-26  7:40 ` [PATCH RESEND v3 2/4] ARM: stacktrace: Avoid duplicate saving of exception PC value Li Huafei
2022-08-26  7:40 ` [PATCH RESEND v3 3/4] ARM: stacktrace: Make stack walk callback consistent with generic code Li Huafei
2022-08-26  7:40 ` [PATCH RESEND v3 4/4] ARM: stacktrace: Convert stacktrace to generic ARCH_STACKWALK Li Huafei

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).