[PATCH 0/6] arm64: stacktrace cleanups

[PATCH 0/6] arm64: stacktrace cleanups

[Mark Rutland]

This series is a collection of cleanup / refactoring changes for the
arm64 stacktrace code.

There should be no functional change as a result of this series; the
bulk of the series is a rename for consistency with the core+x86 unwind
code, and the only structural changes are the removal of a redundant
NULL check and making some code private to
arch/arm64/kernel/stacktrace.c.

The bulk of the series has been extracted from v13 of Madhavan's series
adding reliability checks:

  https://lore.kernel.org/linux-arm-kernel/20220117145608.6781-1-madvenka@linux.microsoft.com/

... and I've rebased the extracted patches atop v5.18-rc1. The rest of
that series needs further review, and I'd like to get the
uncontroversial bits out of the way now while that continues so that we
can make other changes in this area.

The couple of patches from me are new as of this posting, but I'm hoping
those are simple enough that this can be picked up as-is.

The series can be found in my arm64/stacktrace/cleanups branch:

  https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/stacktrace/cleanups
  git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/stacktrace/cleanups

Thanks,
Mark.

Madhavan T. Venkataraman (4):
  arm64: stacktrace: remove NULL task check from unwind_frame()
  arm64: stacktrace: rename unwinder functions
  arm64: stacktrace: rename stackframe to unwind_state
  arm64: stacktrace: align with common naming

Mark Rutland (2):
  arm64: stacktrace: delete PCS comment
  arm64: stacktrace: make struct stackframe private to stacktrace.c

 arch/arm64/include/asm/stacktrace.h |  32 -------
 arch/arm64/kernel/stacktrace.c      | 124 ++++++++++++++++------------
 2 files changed, 69 insertions(+), 87 deletions(-)

-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] arm64: stacktrace: remove NULL task check from unwind_frame()

[Mark Rutland]

From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>

Currently, there is a check for a NULL task in unwind_frame(). It is not
needed since all current callers pass a non-NULL task.

There should be no functional change as a result of this patch.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/stacktrace.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index e4103e085681..94932ade5c79 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -70,9 +70,6 @@ static int notrace unwind_frame(struct task_struct *tsk,
 	unsigned long fp = frame->fp;
 	struct stack_info info;
 
-	if (!tsk)
-		tsk = current;
-
 	/* Final frame; nothing to unwind */
 	if (fp == (unsigned long)task_pt_regs(tsk)->stackframe)
 		return -ENOENT;
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/6] arm64: stacktrace: delete PCS comment

[Mark Rutland]

The comment at the top of stacktrace.c isn't all that helpful, as it's
not associated with the code which inspects the frame record, and the
code example isn't representative of common code generation today.

Delete it.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/stacktrace.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 94932ade5c79..08af9ca9a845 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -18,21 +18,6 @@
 #include <asm/stack_pointer.h>
 #include <asm/stacktrace.h>
 
-/*
- * AArch64 PCS assigns the frame pointer to x29.
- *
- * A simple function prologue looks like this:
- * 	sub	sp, sp, #0x10
- *   	stp	x29, x30, [sp]
- *	mov	x29, sp
- *
- * A simple function epilogue looks like this:
- *	mov	sp, x29
- *	ldp	x29, x30, [sp]
- *	add	sp, sp, #0x10
- */
-
-
 static notrace void start_backtrace(struct stackframe *frame, unsigned long fp,
 				    unsigned long pc)
 {
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] arm64: stacktrace: make struct stackframe private to stacktrace.c

[Mark Rutland]

Now that arm64 uses arch_stack_walk() consistently, struct stackframe is
only used within stacktrace.c. To make it easier to read and maintain
this code, it would be nicer if the definition were there too.

Move the definition into stacktrace.c.

There should be no functional change as a result of this patch.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/include/asm/stacktrace.h | 32 -----------------------------
 arch/arm64/kernel/stacktrace.c      | 32 +++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index e77cdef9ca29..aec9315bf156 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -31,38 +31,6 @@ struct stack_info {
 	enum stack_type type;
 };
 
-/*
- * A snapshot of a frame record or fp/lr register values, along with some
- * accounting information necessary for robust unwinding.
- *
- * @fp:          The fp value in the frame record (or the real fp)
- * @pc:          The lr value in the frame record (or the real lr)
- *
- * @stacks_done: Stacks which have been entirely unwound, for which it is no
- *               longer valid to unwind to.
- *
- * @prev_fp:     The fp that pointed to this frame record, or a synthetic value
- *               of 0. This is used to ensure that within a stack, each
- *               subsequent frame record is at an increasing address.
- * @prev_type:   The type of stack this frame record was on, or a synthetic
- *               value of STACK_TYPE_UNKNOWN. This is used to detect a
- *               transition from one stack to another.
- *
- * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
- *               associated with the most recently encountered replacement lr
- *               value.
- */
-struct stackframe {
-	unsigned long fp;
-	unsigned long pc;
-	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
-	unsigned long prev_fp;
-	enum stack_type prev_type;
-#ifdef CONFIG_KRETPROBES
-	struct llist_node *kr_cur;
-#endif
-};
-
 extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
 			   const char *loglvl);
 
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 08af9ca9a845..073d0941a5b6 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -18,6 +18,38 @@
 #include <asm/stack_pointer.h>
 #include <asm/stacktrace.h>
 
+/*
+ * A snapshot of a frame record or fp/lr register values, along with some
+ * accounting information necessary for robust unwinding.
+ *
+ * @fp:          The fp value in the frame record (or the real fp)
+ * @pc:          The lr value in the frame record (or the real lr)
+ *
+ * @stacks_done: Stacks which have been entirely unwound, for which it is no
+ *               longer valid to unwind to.
+ *
+ * @prev_fp:     The fp that pointed to this frame record, or a synthetic value
+ *               of 0. This is used to ensure that within a stack, each
+ *               subsequent frame record is at an increasing address.
+ * @prev_type:   The type of stack this frame record was on, or a synthetic
+ *               value of STACK_TYPE_UNKNOWN. This is used to detect a
+ *               transition from one stack to another.
+ *
+ * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
+ *               associated with the most recently encountered replacement lr
+ *               value.
+ */
+struct stackframe {
+	unsigned long fp;
+	unsigned long pc;
+	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
+	unsigned long prev_fp;
+	enum stack_type prev_type;
+#ifdef CONFIG_KRETPROBES
+	struct llist_node *kr_cur;
+#endif
+};
+
 static notrace void start_backtrace(struct stackframe *frame, unsigned long fp,
 				    unsigned long pc)
 {
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/6] arm64: stacktrace: rename unwinder functions

[Mark Rutland]

From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>

Rename unwinder functions for consistency and better naming.

	- Rename start_backtrace() to unwind_init().
	- Rename unwind_frame() to unwind_next().
	- Rename walk_stackframe() to unwind().

There should be no functional change as a result of this patch.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/stacktrace.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 073d0941a5b6..d65fde99b74a 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -50,8 +50,8 @@ struct stackframe {
 #endif
 };
 
-static notrace void start_backtrace(struct stackframe *frame, unsigned long fp,
-				    unsigned long pc)
+static notrace void unwind_init(struct stackframe *frame, unsigned long fp,
+				unsigned long pc)
 {
 	frame->fp = fp;
 	frame->pc = pc;
@@ -62,7 +62,7 @@ static notrace void start_backtrace(struct stackframe *frame, unsigned long fp,
 	/*
 	 * Prime the first unwind.
 	 *
-	 * In unwind_frame() we'll check that the FP points to a valid stack,
+	 * In unwind_next() we'll check that the FP points to a valid stack,
 	 * which can't be STACK_TYPE_UNKNOWN, and the first unwind will be
 	 * treated as a transition to whichever stack that happens to be. The
 	 * prev_fp value won't be used, but we set it to 0 such that it is
@@ -72,7 +72,7 @@ static notrace void start_backtrace(struct stackframe *frame, unsigned long fp,
 	frame->prev_fp = 0;
 	frame->prev_type = STACK_TYPE_UNKNOWN;
 }
-NOKPROBE_SYMBOL(start_backtrace);
+NOKPROBE_SYMBOL(unwind_init);
 
 /*
  * Unwind from one frame record (A) to the next frame record (B).
@@ -81,8 +81,8 @@ NOKPROBE_SYMBOL(start_backtrace);
  * records (e.g. a cycle), determined based on the location and fp value of A
  * and the location (but not the fp value) of B.
  */
-static int notrace unwind_frame(struct task_struct *tsk,
-				struct stackframe *frame)
+static int notrace unwind_next(struct task_struct *tsk,
+			       struct stackframe *frame)
 {
 	unsigned long fp = frame->fp;
 	struct stack_info info;
@@ -122,7 +122,7 @@ static int notrace unwind_frame(struct task_struct *tsk,
 
 	/*
 	 * Record this frame record's values and location. The prev_fp and
-	 * prev_type are only meaningful to the next unwind_frame() invocation.
+	 * prev_type are only meaningful to the next unwind_next() invocation.
 	 */
 	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
 	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
@@ -155,23 +155,23 @@ static int notrace unwind_frame(struct task_struct *tsk,
 
 	return 0;
 }
-NOKPROBE_SYMBOL(unwind_frame);
+NOKPROBE_SYMBOL(unwind_next);
 
-static void notrace walk_stackframe(struct task_struct *tsk,
-				    struct stackframe *frame,
-				    bool (*fn)(void *, unsigned long), void *data)
+static void notrace unwind(struct task_struct *tsk,
+			   struct stackframe *frame,
+			   bool (*fn)(void *, unsigned long), void *data)
 {
 	while (1) {
 		int ret;
 
 		if (!fn(data, frame->pc))
 			break;
-		ret = unwind_frame(tsk, frame);
+		ret = unwind_next(tsk, frame);
 		if (ret < 0)
 			break;
 	}
 }
-NOKPROBE_SYMBOL(walk_stackframe);
+NOKPROBE_SYMBOL(unwind);
 
 static bool dump_backtrace_entry(void *arg, unsigned long where)
 {
@@ -213,14 +213,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 	struct stackframe frame;
 
 	if (regs)
-		start_backtrace(&frame, regs->regs[29], regs->pc);
+		unwind_init(&frame, regs->regs[29], regs->pc);
 	else if (task == current)
-		start_backtrace(&frame,
+		unwind_init(&frame,
 				(unsigned long)__builtin_frame_address(1),
 				(unsigned long)__builtin_return_address(0));
 	else
-		start_backtrace(&frame, thread_saved_fp(task),
+		unwind_init(&frame, thread_saved_fp(task),
 				thread_saved_pc(task));
 
-	walk_stackframe(task, &frame, consume_entry, cookie);
+	unwind(task, &frame, consume_entry, cookie);
 }
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/6] arm64: stacktrace: rename stackframe to unwind_state

[Mark Rutland]

From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>

Rename "struct stackframe" to "struct unwind_state" for consistency and
better naming. Accordingly, rename variable/argument "frame" to "state".

There should be no functional change as a result of this patch.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/stacktrace.c | 68 +++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index d65fde99b74a..d5a195748aff 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -39,7 +39,7 @@
  *               associated with the most recently encountered replacement lr
  *               value.
  */
-struct stackframe {
+struct unwind_state {
 	unsigned long fp;
 	unsigned long pc;
 	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
@@ -50,13 +50,13 @@ struct stackframe {
 #endif
 };
 
-static notrace void unwind_init(struct stackframe *frame, unsigned long fp,
+static notrace void unwind_init(struct unwind_state *state, unsigned long fp,
 				unsigned long pc)
 {
-	frame->fp = fp;
-	frame->pc = pc;
+	state->fp = fp;
+	state->pc = pc;
 #ifdef CONFIG_KRETPROBES
-	frame->kr_cur = NULL;
+	state->kr_cur = NULL;
 #endif
 
 	/*
@@ -68,9 +68,9 @@ static notrace void unwind_init(struct stackframe *frame, unsigned long fp,
 	 * prev_fp value won't be used, but we set it to 0 such that it is
 	 * definitely not an accessible stack address.
 	 */
-	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
-	frame->prev_fp = 0;
-	frame->prev_type = STACK_TYPE_UNKNOWN;
+	bitmap_zero(state->stacks_done, __NR_STACK_TYPES);
+	state->prev_fp = 0;
+	state->prev_type = STACK_TYPE_UNKNOWN;
 }
 NOKPROBE_SYMBOL(unwind_init);
 
@@ -82,9 +82,9 @@ NOKPROBE_SYMBOL(unwind_init);
  * and the location (but not the fp value) of B.
  */
 static int notrace unwind_next(struct task_struct *tsk,
-			       struct stackframe *frame)
+			       struct unwind_state *state)
 {
-	unsigned long fp = frame->fp;
+	unsigned long fp = state->fp;
 	struct stack_info info;
 
 	/* Final frame; nothing to unwind */
@@ -97,7 +97,7 @@ static int notrace unwind_next(struct task_struct *tsk,
 	if (!on_accessible_stack(tsk, fp, 16, &info))
 		return -EINVAL;
 
-	if (test_bit(info.type, frame->stacks_done))
+	if (test_bit(info.type, state->stacks_done))
 		return -EINVAL;
 
 	/*
@@ -113,27 +113,27 @@ static int notrace unwind_next(struct task_struct *tsk,
 	 * stack to another, it's never valid to unwind back to that first
 	 * stack.
 	 */
-	if (info.type == frame->prev_type) {
-		if (fp <= frame->prev_fp)
+	if (info.type == state->prev_type) {
+		if (fp <= state->prev_fp)
 			return -EINVAL;
 	} else {
-		set_bit(frame->prev_type, frame->stacks_done);
+		set_bit(state->prev_type, state->stacks_done);
 	}
 
 	/*
 	 * Record this frame record's values and location. The prev_fp and
 	 * prev_type are only meaningful to the next unwind_next() invocation.
 	 */
-	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
-	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
-	frame->prev_fp = fp;
-	frame->prev_type = info.type;
+	state->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
+	state->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
+	state->prev_fp = fp;
+	state->prev_type = info.type;
 
-	frame->pc = ptrauth_strip_insn_pac(frame->pc);
+	state->pc = ptrauth_strip_insn_pac(state->pc);
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	if (tsk->ret_stack &&
-		(frame->pc == (unsigned long)return_to_handler)) {
+		(state->pc == (unsigned long)return_to_handler)) {
 		unsigned long orig_pc;
 		/*
 		 * This is a case where function graph tracer has
@@ -141,16 +141,16 @@ static int notrace unwind_next(struct task_struct *tsk,
 		 * to hook a function return.
 		 * So replace it to an original value.
 		 */
-		orig_pc = ftrace_graph_ret_addr(tsk, NULL, frame->pc,
-						(void *)frame->fp);
-		if (WARN_ON_ONCE(frame->pc == orig_pc))
+		orig_pc = ftrace_graph_ret_addr(tsk, NULL, state->pc,
+						(void *)state->fp);
+		if (WARN_ON_ONCE(state->pc == orig_pc))
 			return -EINVAL;
-		frame->pc = orig_pc;
+		state->pc = orig_pc;
 	}
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 #ifdef CONFIG_KRETPROBES
-	if (is_kretprobe_trampoline(frame->pc))
-		frame->pc = kretprobe_find_ret_addr(tsk, (void *)frame->fp, &frame->kr_cur);
+	if (is_kretprobe_trampoline(state->pc))
+		state->pc = kretprobe_find_ret_addr(tsk, (void *)state->fp, &state->kr_cur);
 #endif
 
 	return 0;
@@ -158,15 +158,15 @@ static int notrace unwind_next(struct task_struct *tsk,
 NOKPROBE_SYMBOL(unwind_next);
 
 static void notrace unwind(struct task_struct *tsk,
-			   struct stackframe *frame,
+			   struct unwind_state *state,
 			   bool (*fn)(void *, unsigned long), void *data)
 {
 	while (1) {
 		int ret;
 
-		if (!fn(data, frame->pc))
+		if (!fn(data, state->pc))
 			break;
-		ret = unwind_next(tsk, frame);
+		ret = unwind_next(tsk, state);
 		if (ret < 0)
 			break;
 	}
@@ -210,17 +210,17 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 			      void *cookie, struct task_struct *task,
 			      struct pt_regs *regs)
 {
-	struct stackframe frame;
+	struct unwind_state state;
 
 	if (regs)
-		unwind_init(&frame, regs->regs[29], regs->pc);
+		unwind_init(&state, regs->regs[29], regs->pc);
 	else if (task == current)
-		unwind_init(&frame,
+		unwind_init(&state,
 				(unsigned long)__builtin_frame_address(1),
 				(unsigned long)__builtin_return_address(0));
 	else
-		unwind_init(&frame, thread_saved_fp(task),
+		unwind_init(&state, thread_saved_fp(task),
 				thread_saved_pc(task));
 
-	unwind(task, &frame, consume_entry, cookie);
+	unwind(task, &state, consume_entry, cookie);
 }
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/6] arm64: stacktrace: align with common naming

[Mark Rutland]

From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>

For historical reasons, the naming of parameters and their types in the
arm64 stacktrace code differs from that used in generic code and other
architectures, even though the types are equivalent.

For consistency and clarity, use the generic names.

There should be no functional change as a result of this patch.

Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/kernel/stacktrace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index d5a195748aff..0467cb79f080 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -159,12 +159,12 @@ NOKPROBE_SYMBOL(unwind_next);
 
 static void notrace unwind(struct task_struct *tsk,
 			   struct unwind_state *state,
-			   bool (*fn)(void *, unsigned long), void *data)
+			   stack_trace_consume_fn consume_entry, void *cookie)
 {
 	while (1) {
 		int ret;
 
-		if (!fn(data, state->pc))
+		if (!consume_entry(cookie, state->pc))
 			break;
 		ret = unwind_next(tsk, state);
 		if (ret < 0)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/6] arm64: stacktrace: delete PCS comment

[Madhavan T. Venkataraman]

On 4/13/22 09:59, Mark Rutland wrote:
> The comment at the top of stacktrace.c isn't all that helpful, as it's
> not associated with the code which inspects the frame record, and the
> code example isn't representative of common code generation today.
> 
> Delete it.
> 
> There should be no functional change as a result of this patch.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/kernel/stacktrace.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 94932ade5c79..08af9ca9a845 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -18,21 +18,6 @@
>  #include <asm/stack_pointer.h>
>  #include <asm/stacktrace.h>
>  
> -/*
> - * AArch64 PCS assigns the frame pointer to x29.
> - *
> - * A simple function prologue looks like this:
> - * 	sub	sp, sp, #0x10
> - *   	stp	x29, x30, [sp]
> - *	mov	x29, sp
> - *
> - * A simple function epilogue looks like this:
> - *	mov	sp, x29
> - *	ldp	x29, x30, [sp]
> - *	add	sp, sp, #0x10
> - */
> -
> -
>  static notrace void start_backtrace(struct stackframe *frame, unsigned long fp,
>  				    unsigned long pc)
>  {

Reviewed-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] arm64: stacktrace: make struct stackframe private to stacktrace.c

[Madhavan T. Venkataraman]

On 4/13/22 09:59, Mark Rutland wrote:
> Now that arm64 uses arch_stack_walk() consistently, struct stackframe is
> only used within stacktrace.c. To make it easier to read and maintain
> this code, it would be nicer if the definition were there too.
> 
> Move the definition into stacktrace.c.
> 
> There should be no functional change as a result of this patch.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/stacktrace.h | 32 -----------------------------
>  arch/arm64/kernel/stacktrace.c      | 32 +++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index e77cdef9ca29..aec9315bf156 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -31,38 +31,6 @@ struct stack_info {
>  	enum stack_type type;
>  };
>  
> -/*
> - * A snapshot of a frame record or fp/lr register values, along with some
> - * accounting information necessary for robust unwinding.
> - *
> - * @fp:          The fp value in the frame record (or the real fp)
> - * @pc:          The lr value in the frame record (or the real lr)
> - *
> - * @stacks_done: Stacks which have been entirely unwound, for which it is no
> - *               longer valid to unwind to.
> - *
> - * @prev_fp:     The fp that pointed to this frame record, or a synthetic value
> - *               of 0. This is used to ensure that within a stack, each
> - *               subsequent frame record is at an increasing address.
> - * @prev_type:   The type of stack this frame record was on, or a synthetic
> - *               value of STACK_TYPE_UNKNOWN. This is used to detect a
> - *               transition from one stack to another.
> - *
> - * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
> - *               associated with the most recently encountered replacement lr
> - *               value.
> - */
> -struct stackframe {
> -	unsigned long fp;
> -	unsigned long pc;
> -	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
> -	unsigned long prev_fp;
> -	enum stack_type prev_type;
> -#ifdef CONFIG_KRETPROBES
> -	struct llist_node *kr_cur;
> -#endif
> -};
> -
>  extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>  			   const char *loglvl);
>  
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 08af9ca9a845..073d0941a5b6 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -18,6 +18,38 @@
>  #include <asm/stack_pointer.h>
>  #include <asm/stacktrace.h>
>  
> +/*
> + * A snapshot of a frame record or fp/lr register values, along with some
> + * accounting information necessary for robust unwinding.
> + *
> + * @fp:          The fp value in the frame record (or the real fp)
> + * @pc:          The lr value in the frame record (or the real lr)
> + *
> + * @stacks_done: Stacks which have been entirely unwound, for which it is no
> + *               longer valid to unwind to.
> + *
> + * @prev_fp:     The fp that pointed to this frame record, or a synthetic value
> + *               of 0. This is used to ensure that within a stack, each
> + *               subsequent frame record is at an increasing address.
> + * @prev_type:   The type of stack this frame record was on, or a synthetic
> + *               value of STACK_TYPE_UNKNOWN. This is used to detect a
> + *               transition from one stack to another.
> + *
> + * @kr_cur:      When KRETPROBES is selected, holds the kretprobe instance
> + *               associated with the most recently encountered replacement lr
> + *               value.
> + */
> +struct stackframe {
> +	unsigned long fp;
> +	unsigned long pc;
> +	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
> +	unsigned long prev_fp;
> +	enum stack_type prev_type;
> +#ifdef CONFIG_KRETPROBES
> +	struct llist_node *kr_cur;
> +#endif
> +};
> +
>  static notrace void start_backtrace(struct stackframe *frame, unsigned long fp,
>  				    unsigned long pc)
>  {

Reviewed-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/6] arm64: stacktrace: delete PCS comment

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 514 bytes --]

On Wed, Apr 13, 2022 at 03:59:06PM +0100, Mark Rutland wrote:
> The comment at the top of stacktrace.c isn't all that helpful, as it's
> not associated with the code which inspects the frame record, and the
> code example isn't representative of common code generation today.

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

though I do wonder if it might be better to rather than remove it expand
it to be a bit of an essay on what actually happens these days, that
could be better placed in Documentation/ though.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] arm64: stacktrace: make struct stackframe private to stacktrace.c

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 371 bytes --]

On Wed, Apr 13, 2022 at 03:59:07PM +0100, Mark Rutland wrote:
> Now that arm64 uses arch_stack_walk() consistently, struct stackframe is
> only used within stacktrace.c. To make it easier to read and maintain
> this code, it would be nicer if the definition were there too.
> 
> Move the definition into stacktrace.c.

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] arm64: stacktrace: align with common naming

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 446 bytes --]

On Wed, Apr 13, 2022 at 03:59:10PM +0100, Mark Rutland wrote:
> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
> 
> For historical reasons, the naming of parameters and their types in the
> arm64 stacktrace code differs from that used in generic code and other
> architectures, even though the types are equivalent.
> 
> For consistency and clarity, use the generic names.

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/6] arm64: stacktrace: delete PCS comment

[Mark Rutland]

On Wed, Apr 13, 2022 at 04:59:14PM +0100, Mark Brown wrote:
> On Wed, Apr 13, 2022 at 03:59:06PM +0100, Mark Rutland wrote:
> > The comment at the top of stacktrace.c isn't all that helpful, as it's
> > not associated with the code which inspects the frame record, and the
> > code example isn't representative of common code generation today.
> 
> Reviewed-by: Mark Brown <broonie@kernel.org>
> 
> though I do wonder if it might be better to rather than remove it expand
> it to be a bit of an essay on what actually happens these days, that
> could be better placed in Documentation/ though.

FWIW, I want to add a Documentation/arm64/unwinding.{whatever} to give links to
the relevant documentation (e.g. the AAPCS64 docs) and to explain the
arm64-specific unwinding concerns, but I haven't had the chance to write that
yet.

Either way I'd like to remove this comment since it's both misplaced and
misleading, so thanks for the Reviewed-by tag!

Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/6] arm64: stacktrace cleanups

[Kalesh Singh]

On Wed, Apr 13, 2022 at 7:59 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> This series is a collection of cleanup / refactoring changes for the
> arm64 stacktrace code.
>
> There should be no functional change as a result of this series; the
> bulk of the series is a rename for consistency with the core+x86 unwind
> code, and the only structural changes are the removal of a redundant
> NULL check and making some code private to
> arch/arm64/kernel/stacktrace.c.
>
> The bulk of the series has been extracted from v13 of Madhavan's series
> adding reliability checks:
>
>   https://lore.kernel.org/linux-arm-kernel/20220117145608.6781-1-madvenka@linux.microsoft.com/
>
> ... and I've rebased the extracted patches atop v5.18-rc1. The rest of
> that series needs further review, and I'd like to get the
> uncontroversial bits out of the way now while that continues so that we
> can make other changes in this area.
>
> The couple of patches from me are new as of this posting, but I'm hoping
> those are simple enough that this can be picked up as-is.
>
> The series can be found in my arm64/stacktrace/cleanups branch:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/stacktrace/cleanups
>   git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/stacktrace/cleanups

Hi Mark,

Apologies for the late response.

Reviewed-by: Kalesh Singh <kaleshsingh@google.com> for the series.

Thanks,
Kalesh

>
> Thanks,
> Mark.
>
> Madhavan T. Venkataraman (4):
>   arm64: stacktrace: remove NULL task check from unwind_frame()
>   arm64: stacktrace: rename unwinder functions
>   arm64: stacktrace: rename stackframe to unwind_state
>   arm64: stacktrace: align with common naming
>
> Mark Rutland (2):
>   arm64: stacktrace: delete PCS comment
>   arm64: stacktrace: make struct stackframe private to stacktrace.c
>
>  arch/arm64/include/asm/stacktrace.h |  32 -------
>  arch/arm64/kernel/stacktrace.c      | 124 ++++++++++++++++------------
>  2 files changed, 69 insertions(+), 87 deletions(-)
>
> --
> 2.30.2
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/6] arm64: stacktrace cleanups

[Catalin Marinas]

On Wed, 13 Apr 2022 15:59:04 +0100, Mark Rutland wrote:
> This series is a collection of cleanup / refactoring changes for the
> arm64 stacktrace code.
> 
> There should be no functional change as a result of this series; the
> bulk of the series is a rename for consistency with the core+x86 unwind
> code, and the only structural changes are the removal of a redundant
> NULL check and making some code private to
> arch/arm64/kernel/stacktrace.c.
> 
> [...]

Applied to arm64 (for-next/stacktrace), thanks!

[1/6] arm64: stacktrace: remove NULL task check from unwind_frame()
      https://git.kernel.org/arm64/c/4f6277e8ac39
[2/6] arm64: stacktrace: delete PCS comment
      https://git.kernel.org/arm64/c/cb86a41b35c8
[3/6] arm64: stacktrace: make struct stackframe private to stacktrace.c
      https://git.kernel.org/arm64/c/96bb1530c4f9
[4/6] arm64: stacktrace: rename unwinder functions
      https://git.kernel.org/arm64/c/c797bd45480b
[5/6] arm64: stacktrace: rename stackframe to unwind_state
      https://git.kernel.org/arm64/c/e9d75a0ba878
[6/6] arm64: stacktrace: align with common naming
      https://git.kernel.org/arm64/c/bd5552bc4807

-- 
Catalin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel