[PATCHv2 0/3] arm64: stacktrace: improve robustness

[PATCHv2 0/3] arm64: stacktrace: improve robustness

[Mark Rutland]

The arm64 stacktrace code is careful to only access valid stack
locations, but in the presence of a corrupted stack where frame records
form a loop, it will never terminate.

This series updates the stacktrace code to terminate in finite time even
when a stack is corrupted. A stacktrace will be terminated if the next
record is at a lower (or equal) address on the current stack, or when
the next record is on a stack we've already completed unwinding.

The first couple of patches come from Dave's prior attempt to fix this
[1], with the final patch relying on infrastructure which has been
introduced in the mean time.

I've given this a quick spin with magic-sysrq L in a KVM guest, and
things look fine, but further testing would be appreciated.

This series (based on v5.2-rc1) can also be found in my
arm64/robust-stracktrace branch on kernel.org [2].

Since v1 [3]:
* Use start_backtrace() consistently
* Don't use tsk in start_backtrace()
* Track the previous FP and type explicitly

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-April/572685.html
[2] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git arm64/robust-stacktrace
[3] https://lore.kernel.org/linux-arm-kernel/20190606125402.10229-1-mark.rutland@arm.com/

Dave Martin (2):
  arm64: stacktrace: Constify stacktrace.h functions
  arm64: stacktrace: Factor out backtrace initialisation

Mark Rutland (1):
  arm64: stacktrace: better handle corrupted stacks

 arch/arm64/include/asm/stacktrace.h | 53 ++++++++++++++++++++++++++++---------
 arch/arm64/kernel/perf_callchain.c  |  7 +----
 arch/arm64/kernel/process.c         |  7 ++---
 arch/arm64/kernel/return_address.c  |  9 +++----
 arch/arm64/kernel/stacktrace.c      | 34 ++++++++++++++----------
 arch/arm64/kernel/time.c            |  7 ++---
 arch/arm64/kernel/traps.c           | 13 +++++----
 7 files changed, 74 insertions(+), 56 deletions(-)

-- 
2.11.0


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

[PATCHv2 1/3] arm64: stacktrace: Constify stacktrace.h functions

[Mark Rutland]

From: Dave Martin <Dave.Martin@arm.com>

on_accessible_stack() and on_task_stack() shouldn't (and don't)
modify their task argument, so it can be const.

This patch adds the appropriate modifiers. Whitespace violations in the
parameter lists are fixed at the same time.

No functional change.

Signed-off-by: Dave Martin <dave.martin@arm.com>
[Mark: fixup const location, whitespace]
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/arm64/include/asm/stacktrace.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index e86737b7c924..4dd569592e65 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -75,8 +75,9 @@ static inline bool on_irq_stack(unsigned long sp,
 	return true;
 }
 
-static inline bool on_task_stack(struct task_struct *tsk, unsigned long sp,
-				struct stack_info *info)
+static inline bool on_task_stack(const struct task_struct *tsk,
+				 unsigned long sp,
+				 struct stack_info *info)
 {
 	unsigned long low = (unsigned long)task_stack_page(tsk);
 	unsigned long high = low + THREAD_SIZE;
@@ -123,9 +124,9 @@ static inline bool on_overflow_stack(unsigned long sp,
  * We can only safely access per-cpu stacks from current in a non-preemptible
  * context.
  */
-static inline bool on_accessible_stack(struct task_struct *tsk,
-					unsigned long sp,
-					struct stack_info *info)
+static inline bool on_accessible_stack(const struct task_struct *tsk,
+				       unsigned long sp,
+				       struct stack_info *info)
 {
 	if (on_task_stack(tsk, sp, info))
 		return true;
-- 
2.11.0


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

[PATCHv2 2/3] arm64: stacktrace: Factor out backtrace initialisation

[Mark Rutland]

From: Dave Martin <Dave.Martin@arm.com>

Some common code is required by each stacktrace user to initialise
struct stackframe before the first call to unwind_frame().

In preparation for adding to the common code, this patch factors it
out into a separate function start_backtrace(), and modifies the
stacktrace callers appropriately.

No functional change.

Signed-off-by: Dave Martin <dave.martin@arm.com>
[Mark: drop tsk argument, update more callsites]
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/stacktrace.h | 10 ++++++++++
 arch/arm64/kernel/perf_callchain.c  |  7 +------
 arch/arm64/kernel/process.c         |  7 ++-----
 arch/arm64/kernel/return_address.c  |  9 +++------
 arch/arm64/kernel/stacktrace.c      | 19 ++++++-------------
 arch/arm64/kernel/time.c            |  7 ++-----
 arch/arm64/kernel/traps.c           | 13 ++++++-------
 7 files changed, 30 insertions(+), 42 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 4dd569592e65..18f90bf1385c 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -142,4 +142,14 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
 	return false;
 }
 
+static inline void start_backtrace(struct stackframe *frame,
+				   unsigned long fp, unsigned long pc)
+{
+	frame->fp = fp;
+	frame->pc = pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	frame->graph = 0;
+#endif
+}
+
 #endif	/* __ASM_STACKTRACE_H */
diff --git a/arch/arm64/kernel/perf_callchain.c b/arch/arm64/kernel/perf_callchain.c
index 61d983f5756f..35d024735869 100644
--- a/arch/arm64/kernel/perf_callchain.c
+++ b/arch/arm64/kernel/perf_callchain.c
@@ -165,12 +165,7 @@ void perf_callchain_kernel(struct perf_callchain_entry_ctx *entry,
 		return;
 	}
 
-	frame.fp = regs->regs[29];
-	frame.pc = regs->pc;
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	frame.graph = 0;
-#endif
-
+	start_backtrace(&frame, regs->regs[29], regs->pc);
 	walk_stackframe(current, &frame, callchain_trace, entry);
 }
 
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 58efc3727778..46d6b21630a2 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -509,11 +509,8 @@ unsigned long get_wchan(struct task_struct *p)
 	if (!stack_page)
 		return 0;
 
-	frame.fp = thread_saved_fp(p);
-	frame.pc = thread_saved_pc(p);
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	frame.graph = 0;
-#endif
+	start_backtrace(&frame, thread_saved_fp(p), thread_saved_pc(p));
+
 	do {
 		if (unwind_frame(p, &frame))
 			goto out;
diff --git a/arch/arm64/kernel/return_address.c b/arch/arm64/kernel/return_address.c
index 53c40196b607..1b25321c7bf3 100644
--- a/arch/arm64/kernel/return_address.c
+++ b/arch/arm64/kernel/return_address.c
@@ -41,12 +41,9 @@ void *return_address(unsigned int level)
 	data.level = level + 2;
 	data.addr = NULL;
 
-	frame.fp = (unsigned long)__builtin_frame_address(0);
-	frame.pc = (unsigned long)return_address; /* dummy */
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	frame.graph = 0;
-#endif
-
+	start_backtrace(&frame,
+			(unsigned long)__builtin_frame_address(0),
+			(unsigned long)return_address);
 	walk_stackframe(current, &frame, save_return_addr, &data);
 
 	if (!data.level)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index b00ec7d483d1..e5338216deaa 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -133,12 +133,7 @@ void save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
 	data.skip = trace->skip;
 	data.no_sched_functions = 0;
 
-	frame.fp = regs->regs[29];
-	frame.pc = regs->pc;
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	frame.graph = 0;
-#endif
-
+	start_backtrace(&frame, regs->regs[29], regs->pc);
 	walk_stackframe(current, &frame, save_trace, &data);
 }
 EXPORT_SYMBOL_GPL(save_stack_trace_regs);
@@ -157,17 +152,15 @@ static noinline void __save_stack_trace(struct task_struct *tsk,
 	data.no_sched_functions = nosched;
 
 	if (tsk != current) {
-		frame.fp = thread_saved_fp(tsk);
-		frame.pc = thread_saved_pc(tsk);
+		start_backtrace(&frame, thread_saved_fp(tsk),
+				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.pc = (unsigned long)__save_stack_trace;
+		start_backtrace(&frame,
+				(unsigned long)__builtin_frame_address(0),
+				(unsigned long)__save_stack_trace);
 	}
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	frame.graph = 0;
-#endif
 
 	walk_stackframe(tsk, &frame, save_trace, &data);
 
diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index a777ae90044d..6af8d976a2d8 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -49,11 +49,8 @@ unsigned long profile_pc(struct pt_regs *regs)
 	if (!in_lock_functions(regs->pc))
 		return regs->pc;
 
-	frame.fp = regs->regs[29];
-	frame.pc = regs->pc;
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	frame.graph = 0;
-#endif
+	start_backtrace(&frame, regs->regs[29], regs->pc);
+
 	do {
 		int ret = unwind_frame(NULL, &frame);
 		if (ret < 0)
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 8a395523adcf..44810d5aab49 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -111,18 +111,17 @@ void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 		return;
 
 	if (tsk == current) {
-		frame.fp = (unsigned long)__builtin_frame_address(0);
-		frame.pc = (unsigned long)dump_backtrace;
+		start_backtrace(&frame,
+				(unsigned long)__builtin_frame_address(0),
+				(unsigned long)dump_backtrace);
 	} else {
 		/*
 		 * task blocked in __switch_to
 		 */
-		frame.fp = thread_saved_fp(tsk);
-		frame.pc = thread_saved_pc(tsk);
+		start_backtrace(&frame,
+				thread_saved_fp(tsk),
+				thread_saved_pc(tsk));
 	}
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	frame.graph = 0;
-#endif
 
 	printk("Call trace:\n");
 	do {
-- 
2.11.0


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

[PATCHv2 3/3] arm64: stacktrace: better handle corrupted stacks

[Mark Rutland]

The arm64 stacktrace code is careful to only dereference frame records
in valid stack ranges, ensuring that a corrupted frame record won't
result in a faulting access.

However, it's still possible for corrupt frame records to result in
infinite loops in the stacktrace code, which is also undesirable.

This patch ensures that we complete a stacktrace in finite time, by
keeping track of which stacks we have already completed unwinding, and
verifying that if the next frame record is on the same stack, it is at a
higher address.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Dave Martin <dave.martin@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Tengfei Fan <tengfeif@codeaurora.org>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/stacktrace.h | 32 ++++++++++++++++++++++++--------
 arch/arm64/kernel/stacktrace.c      | 15 ++++++++++++++-
 2 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
index 18f90bf1385c..938b96ba1f0f 100644
--- a/arch/arm64/include/asm/stacktrace.h
+++ b/arch/arm64/include/asm/stacktrace.h
@@ -19,19 +19,12 @@
 #include <linux/percpu.h>
 #include <linux/sched.h>
 #include <linux/sched/task_stack.h>
+#include <linux/types.h>
 
 #include <asm/memory.h>
 #include <asm/ptrace.h>
 #include <asm/sdei.h>
 
-struct stackframe {
-	unsigned long fp;
-	unsigned long pc;
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	int graph;
-#endif
-};
-
 enum stack_type {
 	STACK_TYPE_UNKNOWN,
 	STACK_TYPE_TASK,
@@ -39,6 +32,7 @@ enum stack_type {
 	STACK_TYPE_OVERFLOW,
 	STACK_TYPE_SDEI_NORMAL,
 	STACK_TYPE_SDEI_CRITICAL,
+	__NR_STACK_TYPES
 };
 
 struct stack_info {
@@ -47,6 +41,17 @@ struct stack_info {
 	enum stack_type type;
 };
 
+struct stackframe {
+	unsigned long fp;
+	unsigned long pc;
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	int graph;
+#endif
+	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
+	unsigned long prev_fp;
+	enum stack_type prev_type;
+};
+
 extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
 extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
 			    int (*fn)(struct stackframe *, void *), void *data);
@@ -128,6 +133,9 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
 				       unsigned long sp,
 				       struct stack_info *info)
 {
+	if (info)
+		info->type = STACK_TYPE_UNKNOWN;
+
 	if (on_task_stack(tsk, sp, info))
 		return true;
 	if (tsk != current || preemptible())
@@ -150,6 +158,14 @@ static inline void start_backtrace(struct stackframe *frame,
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	frame->graph = 0;
 #endif
+
+	/*
+	 * Prime the first unwind, which will be treated as a transition from
+	 * STACK_TYPE_UNKNOWN to some valid stack.
+	 */
+	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
+	frame->prev_fp = 0;
+	frame->prev_type = STACK_TYPE_UNKNOWN;
 }
 
 #endif	/* __ASM_STACKTRACE_H */
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index e5338216deaa..2e4b59e10e71 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -43,6 +43,7 @@
 int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 {
 	unsigned long fp = frame->fp;
+	struct stack_info info, prev_info;
 
 	if (fp & 0xf)
 		return -EINVAL;
@@ -50,11 +51,23 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
 	if (!tsk)
 		tsk = current;
 
-	if (!on_accessible_stack(tsk, fp, NULL))
+	if (!on_accessible_stack(tsk, fp, &info))
 		return -EINVAL;
 
+	if (test_bit(info.type, frame->stacks_done))
+		return -EINVAL;
+
+	if (info.type == frame->prev_type) {
+		if (fp <= frame->prev_fp)
+			return -EINVAL;
+	} else {
+		set_bit(prev_info.type, frame->stacks_done);
+	}
+
 	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;
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	if (tsk->ret_stack &&
-- 
2.11.0


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

Re: [PATCHv2 0/3] arm64: stacktrace: improve robustness

[Will Deacon]

On Fri, Jun 28, 2019 at 04:46:36PM +0100, Mark Rutland wrote:
> The arm64 stacktrace code is careful to only access valid stack
> locations, but in the presence of a corrupted stack where frame records
> form a loop, it will never terminate.
> 
> This series updates the stacktrace code to terminate in finite time even
> when a stack is corrupted. A stacktrace will be terminated if the next
> record is at a lower (or equal) address on the current stack, or when
> the next record is on a stack we've already completed unwinding.
> 
> The first couple of patches come from Dave's prior attempt to fix this
> [1], with the final patch relying on infrastructure which has been
> introduced in the mean time.
> 
> I've given this a quick spin with magic-sysrq L in a KVM guest, and
> things look fine, but further testing would be appreciated.
> 
> This series (based on v5.2-rc1) can also be found in my
> arm64/robust-stracktrace branch on kernel.org [2].
> 
> Since v1 [3]:
> * Use start_backtrace() consistently
> * Don't use tsk in start_backtrace()
> * Track the previous FP and type explicitly

Given that you had to draw a diagram to figure this out, please could you
include something along those lines in the code as well? I worry that this
will just regress otherwise.

Will

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

Re: [PATCHv2 0/3] arm64: stacktrace: improve robustness

[Mark Rutland]

On Mon, Jul 01, 2019 at 09:29:41AM +0100, Will Deacon wrote:
> On Fri, Jun 28, 2019 at 04:46:36PM +0100, Mark Rutland wrote:
> > The arm64 stacktrace code is careful to only access valid stack
> > locations, but in the presence of a corrupted stack where frame records
> > form a loop, it will never terminate.
> > 
> > This series updates the stacktrace code to terminate in finite time even
> > when a stack is corrupted. A stacktrace will be terminated if the next
> > record is at a lower (or equal) address on the current stack, or when
> > the next record is on a stack we've already completed unwinding.
> > 
> > The first couple of patches come from Dave's prior attempt to fix this
> > [1], with the final patch relying on infrastructure which has been
> > introduced in the mean time.
> > 
> > I've given this a quick spin with magic-sysrq L in a KVM guest, and
> > things look fine, but further testing would be appreciated.
> > 
> > This series (based on v5.2-rc1) can also be found in my
> > arm64/robust-stracktrace branch on kernel.org [2].
> > 
> > Since v1 [3]:
> > * Use start_backtrace() consistently
> > * Don't use tsk in start_backtrace()
> > * Track the previous FP and type explicitly
> 
> Given that you had to draw a diagram to figure this out, please could you
> include something along those lines in the code as well? I worry that this
> will just regress otherwise.

I'll see what I can do, though I may just use words rather than ASCII
art -- the key point is that we should only account one transition, from
the stackframe we're passed to the stackframe it points to.

Thanks,
Mark.

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

Re: [PATCHv2 3/3] arm64: stacktrace: better handle corrupted stacks

[Dave Martin]

On Fri, Jun 28, 2019 at 04:46:39PM +0100, Mark Rutland wrote:
> The arm64 stacktrace code is careful to only dereference frame records
> in valid stack ranges, ensuring that a corrupted frame record won't
> result in a faulting access.
> 
> However, it's still possible for corrupt frame records to result in
> infinite loops in the stacktrace code, which is also undesirable.
> 
> This patch ensures that we complete a stacktrace in finite time, by
> keeping track of which stacks we have already completed unwinding, and
> verifying that if the next frame record is on the same stack, it is at a
> higher address.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Dave Martin <dave.martin@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Tengfei Fan <tengfeif@codeaurora.org>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/include/asm/stacktrace.h | 32 ++++++++++++++++++++++++--------
>  arch/arm64/kernel/stacktrace.c      | 15 ++++++++++++++-
>  2 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 18f90bf1385c..938b96ba1f0f 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -19,19 +19,12 @@
>  #include <linux/percpu.h>
>  #include <linux/sched.h>
>  #include <linux/sched/task_stack.h>
> +#include <linux/types.h>
>  
>  #include <asm/memory.h>
>  #include <asm/ptrace.h>
>  #include <asm/sdei.h>
>  
> -struct stackframe {
> -	unsigned long fp;
> -	unsigned long pc;
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -	int graph;
> -#endif
> -};
> -
>  enum stack_type {
>  	STACK_TYPE_UNKNOWN,
>  	STACK_TYPE_TASK,
> @@ -39,6 +32,7 @@ enum stack_type {
>  	STACK_TYPE_OVERFLOW,
>  	STACK_TYPE_SDEI_NORMAL,
>  	STACK_TYPE_SDEI_CRITICAL,
> +	__NR_STACK_TYPES
>  };
>  
>  struct stack_info {
> @@ -47,6 +41,17 @@ struct stack_info {
>  	enum stack_type type;
>  };
>  
> +struct stackframe {
> +	unsigned long fp;
> +	unsigned long pc;
> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	int graph;
> +#endif
> +	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
> +	unsigned long prev_fp;
> +	enum stack_type prev_type;
> +};
> +
>  extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
>  extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
>  			    int (*fn)(struct stackframe *, void *), void *data);
> @@ -128,6 +133,9 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
>  				       unsigned long sp,
>  				       struct stack_info *info)
>  {
> +	if (info)
> +		info->type = STACK_TYPE_UNKNOWN;
> +
>  	if (on_task_stack(tsk, sp, info))
>  		return true;
>  	if (tsk != current || preemptible())
> @@ -150,6 +158,14 @@ static inline void start_backtrace(struct stackframe *frame,
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  	frame->graph = 0;
>  #endif
> +
> +	/*
> +	 * Prime the first unwind, which will be treated as a transition from
> +	 * STACK_TYPE_UNKNOWN to some valid stack.
> +	 */
> +	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
> +	frame->prev_fp = 0;
> +	frame->prev_type = STACK_TYPE_UNKNOWN;
>  }
>  
>  #endif	/* __ASM_STACKTRACE_H */
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index e5338216deaa..2e4b59e10e71 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -43,6 +43,7 @@
>  int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  {
>  	unsigned long fp = frame->fp;
> +	struct stack_info info, prev_info;
>  
>  	if (fp & 0xf)
>  		return -EINVAL;
> @@ -50,11 +51,23 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  	if (!tsk)
>  		tsk = current;
>  
> -	if (!on_accessible_stack(tsk, fp, NULL))
> +	if (!on_accessible_stack(tsk, fp, &info))
>  		return -EINVAL;
>  
> +	if (test_bit(info.type, frame->stacks_done))
> +		return -EINVAL;
> +
> +	if (info.type == frame->prev_type) {
> +		if (fp <= frame->prev_fp)
> +			return -EINVAL;
> +	} else {
> +		set_bit(prev_info.type, frame->stacks_done);
> +	}
> +
>  	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;

As in my response on the last series, do we really need to track 2
frames at the same time in struct stackframe?

The transition (i.e., where we came from and where we're going to should
be visible in unwind_frame().  I don't see why we need additional history
in order to detect stack changes or track which stacks have been visited.


So, say (incomplete paste-o-code):

	unsigned long fp = frame->fp;
	enum stack_type fp_stack = frame->fp_stack;

  	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
  	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
	
	if (!on_accessible_stack(tsk, frame->fp, &info))
		return -EINVAL;

	if (test_bit(info.type, frame->stacks_done))
		return -EINVAL;

	if (info.type == frame->fp_stack) {
		/* no stack change */
	} else {
		/* stack change */
	}


This would require stack_backtrace() to be tweaked, so that fp_stack
describes where frame->fp is and the corresponding bit is already set in
stacks_done, rather than starting out as STACK_UNKNOWN.

I haven't fleshed this out, so the idea may fall down somewhere else.


Finally, can we enforce which stack transitions are valid?  (i.e.,
TASK -> SDEI or TASK -> IRQ should never be seen in backtraces).
There may be some gotchas here, so it might not be worth it...  I
vaguely remember some past discussion.

Cheers
---Dave

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

Re: [PATCHv2 3/3] arm64: stacktrace: better handle corrupted stacks

[Mark Rutland]

On Mon, Jul 01, 2019 at 11:56:56AM +0100, Dave Martin wrote:
> On Fri, Jun 28, 2019 at 04:46:39PM +0100, Mark Rutland wrote:
> > The arm64 stacktrace code is careful to only dereference frame records
> > in valid stack ranges, ensuring that a corrupted frame record won't
> > result in a faulting access.
> > 
> > However, it's still possible for corrupt frame records to result in
> > infinite loops in the stacktrace code, which is also undesirable.
> > 
> > This patch ensures that we complete a stacktrace in finite time, by
> > keeping track of which stacks we have already completed unwinding, and
> > verifying that if the next frame record is on the same stack, it is at a
> > higher address.
> > 
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Dave Martin <dave.martin@arm.com>
> > Cc: James Morse <james.morse@arm.com>
> > Cc: Tengfei Fan <tengfeif@codeaurora.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > ---
> >  arch/arm64/include/asm/stacktrace.h | 32 ++++++++++++++++++++++++--------
> >  arch/arm64/kernel/stacktrace.c      | 15 ++++++++++++++-
> >  2 files changed, 38 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> > index 18f90bf1385c..938b96ba1f0f 100644
> > --- a/arch/arm64/include/asm/stacktrace.h
> > +++ b/arch/arm64/include/asm/stacktrace.h
> > @@ -19,19 +19,12 @@
> >  #include <linux/percpu.h>
> >  #include <linux/sched.h>
> >  #include <linux/sched/task_stack.h>
> > +#include <linux/types.h>
> >  
> >  #include <asm/memory.h>
> >  #include <asm/ptrace.h>
> >  #include <asm/sdei.h>
> >  
> > -struct stackframe {
> > -	unsigned long fp;
> > -	unsigned long pc;
> > -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > -	int graph;
> > -#endif
> > -};
> > -
> >  enum stack_type {
> >  	STACK_TYPE_UNKNOWN,
> >  	STACK_TYPE_TASK,
> > @@ -39,6 +32,7 @@ enum stack_type {
> >  	STACK_TYPE_OVERFLOW,
> >  	STACK_TYPE_SDEI_NORMAL,
> >  	STACK_TYPE_SDEI_CRITICAL,
> > +	__NR_STACK_TYPES
> >  };
> >  
> >  struct stack_info {
> > @@ -47,6 +41,17 @@ struct stack_info {
> >  	enum stack_type type;
> >  };
> >  
> > +struct stackframe {
> > +	unsigned long fp;
> > +	unsigned long pc;
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +	int graph;
> > +#endif
> > +	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
> > +	unsigned long prev_fp;
> > +	enum stack_type prev_type;
> > +};
> > +
> >  extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
> >  extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
> >  			    int (*fn)(struct stackframe *, void *), void *data);
> > @@ -128,6 +133,9 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
> >  				       unsigned long sp,
> >  				       struct stack_info *info)
> >  {
> > +	if (info)
> > +		info->type = STACK_TYPE_UNKNOWN;
> > +
> >  	if (on_task_stack(tsk, sp, info))
> >  		return true;
> >  	if (tsk != current || preemptible())
> > @@ -150,6 +158,14 @@ static inline void start_backtrace(struct stackframe *frame,
> >  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> >  	frame->graph = 0;
> >  #endif
> > +
> > +	/*
> > +	 * Prime the first unwind, which will be treated as a transition from
> > +	 * STACK_TYPE_UNKNOWN to some valid stack.
> > +	 */
> > +	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
> > +	frame->prev_fp = 0;
> > +	frame->prev_type = STACK_TYPE_UNKNOWN;
> >  }
> >  
> >  #endif	/* __ASM_STACKTRACE_H */
> > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > index e5338216deaa..2e4b59e10e71 100644
> > --- a/arch/arm64/kernel/stacktrace.c
> > +++ b/arch/arm64/kernel/stacktrace.c
> > @@ -43,6 +43,7 @@
> >  int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> >  {
> >  	unsigned long fp = frame->fp;
> > +	struct stack_info info, prev_info;
> >  
> >  	if (fp & 0xf)
> >  		return -EINVAL;
> > @@ -50,11 +51,23 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> >  	if (!tsk)
> >  		tsk = current;
> >  
> > -	if (!on_accessible_stack(tsk, fp, NULL))
> > +	if (!on_accessible_stack(tsk, fp, &info))
> >  		return -EINVAL;
> >  
> > +	if (test_bit(info.type, frame->stacks_done))
> > +		return -EINVAL;
> > +
> > +	if (info.type == frame->prev_type) {
> > +		if (fp <= frame->prev_fp)
> > +			return -EINVAL;
> > +	} else {
> > +		set_bit(prev_info.type, frame->stacks_done);
> > +	}
> > +
> >  	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;
> 
> As in my response on the last series, do we really need to track 2
> frames at the same time in struct stackframe?

It's better to think of this as tracking the location and contents of
one stackframe.

If we back up a bit, I want to ensure that if we have a chain A->B->C
and the B->C transition is bogus, we report A and B in the backtrace.

The struct stackframe is a snapshot of the frame record A (which may
have been the FP and LR rather than an in-memory record). The contents
of A tells us where the B can be found, but we need the location of A so
that we can check intra-stack whether A < B.

> The transition (i.e., where we came from and where we're going to should
> be visible in unwind_frame().  I don't see why we need additional history
> in order to detect stack changes or track which stacks have been visited.

Please see my prior reply to James, where I described this.

> So, say (incomplete paste-o-code):
 
Let's consider this for the above A->B->C transition.

> 	unsigned long fp = frame->fp;
> 	enum stack_type fp_stack = frame->fp_stack;

When we enter with frame describing A, frame->fp points at B, and
frame->fp_stack describes where A is (i.e. it describes the same thing
as prev_type in my code).

> 
>   	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
>   	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));

Here we update frame to be the contents of B.

> 	if (!on_accessible_stack(tsk, frame->fp, &info))
> 		return -EINVAL;

... and here we bail out if C is inaccessible, before we report B.

> 
> 	if (test_bit(info.type, frame->stacks_done))
> 		return -EINVAL;

Likewise, here we check C's stack type, and bail out an entry earlier
than necessary.

> 
> 	if (info.type == frame->fp_stack) {

We haven't updated frame->fp_stack  yet, so this is comparing A's
location to C's location, ignoring B.

> 		/* no stack change */
> 	} else {
> 		/* stack change */
> 	}
> 
> 
> This would require stack_backtrace() to be tweaked, so that fp_stack
> describes where frame->fp is and the corresponding bit is already set in
> stacks_done, rather than starting out as STACK_UNKNOWN.
> 
> I haven't fleshed this out, so the idea may fall down somewhere else.

I think I've laid that out above.

> Finally, can we enforce which stack transitions are valid?  (i.e.,
> TASK -> SDEI or TASK -> IRQ should never be seen in backtraces).
> There may be some gotchas here, so it might not be worth it...  I
> vaguely remember some past discussion.

We could to some extent, but the valid transitions are a lattice, so
we end up needing some ad-hoc checks.

This isn't necessary to ensure termination so long as we have
intra-stack monotonicity checks and we only permit each stack transition
to occur once.

Thanks,
Mark.

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

Re: [PATCHv2 3/3] arm64: stacktrace: better handle corrupted stacks

[Dave Martin]

On Mon, Jul 01, 2019 at 01:53:52PM +0100, Mark Rutland wrote:
> On Mon, Jul 01, 2019 at 11:56:56AM +0100, Dave Martin wrote:
> > On Fri, Jun 28, 2019 at 04:46:39PM +0100, Mark Rutland wrote:
> > > The arm64 stacktrace code is careful to only dereference frame records
> > > in valid stack ranges, ensuring that a corrupted frame record won't
> > > result in a faulting access.
> > > 
> > > However, it's still possible for corrupt frame records to result in
> > > infinite loops in the stacktrace code, which is also undesirable.
> > > 
> > > This patch ensures that we complete a stacktrace in finite time, by
> > > keeping track of which stacks we have already completed unwinding, and
> > > verifying that if the next frame record is on the same stack, it is at a
> > > higher address.
> > > 
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Dave Martin <dave.martin@arm.com>
> > > Cc: James Morse <james.morse@arm.com>
> > > Cc: Tengfei Fan <tengfeif@codeaurora.org>
> > > Cc: Will Deacon <will.deacon@arm.com>
> > > ---
> > >  arch/arm64/include/asm/stacktrace.h | 32 ++++++++++++++++++++++++--------
> > >  arch/arm64/kernel/stacktrace.c      | 15 ++++++++++++++-
> > >  2 files changed, 38 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> > > index 18f90bf1385c..938b96ba1f0f 100644
> > > --- a/arch/arm64/include/asm/stacktrace.h
> > > +++ b/arch/arm64/include/asm/stacktrace.h
> > > @@ -19,19 +19,12 @@
> > >  #include <linux/percpu.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/sched/task_stack.h>
> > > +#include <linux/types.h>
> > >  
> > >  #include <asm/memory.h>
> > >  #include <asm/ptrace.h>
> > >  #include <asm/sdei.h>
> > >  
> > > -struct stackframe {
> > > -	unsigned long fp;
> > > -	unsigned long pc;
> > > -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > -	int graph;
> > > -#endif
> > > -};
> > > -
> > >  enum stack_type {
> > >  	STACK_TYPE_UNKNOWN,
> > >  	STACK_TYPE_TASK,
> > > @@ -39,6 +32,7 @@ enum stack_type {
> > >  	STACK_TYPE_OVERFLOW,
> > >  	STACK_TYPE_SDEI_NORMAL,
> > >  	STACK_TYPE_SDEI_CRITICAL,
> > > +	__NR_STACK_TYPES
> > >  };
> > >  
> > >  struct stack_info {
> > > @@ -47,6 +41,17 @@ struct stack_info {
> > >  	enum stack_type type;
> > >  };
> > >  
> > > +struct stackframe {
> > > +	unsigned long fp;
> > > +	unsigned long pc;
> > > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > +	int graph;
> > > +#endif
> > > +	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
> > > +	unsigned long prev_fp;
> > > +	enum stack_type prev_type;
> > > +};
> > > +
> > >  extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
> > >  extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
> > >  			    int (*fn)(struct stackframe *, void *), void *data);
> > > @@ -128,6 +133,9 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
> > >  				       unsigned long sp,
> > >  				       struct stack_info *info)
> > >  {
> > > +	if (info)
> > > +		info->type = STACK_TYPE_UNKNOWN;
> > > +
> > >  	if (on_task_stack(tsk, sp, info))
> > >  		return true;
> > >  	if (tsk != current || preemptible())
> > > @@ -150,6 +158,14 @@ static inline void start_backtrace(struct stackframe *frame,
> > >  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > >  	frame->graph = 0;
> > >  #endif
> > > +
> > > +	/*
> > > +	 * Prime the first unwind, which will be treated as a transition from
> > > +	 * STACK_TYPE_UNKNOWN to some valid stack.
> > > +	 */
> > > +	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
> > > +	frame->prev_fp = 0;
> > > +	frame->prev_type = STACK_TYPE_UNKNOWN;
> > >  }
> > >  
> > >  #endif	/* __ASM_STACKTRACE_H */
> > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > > index e5338216deaa..2e4b59e10e71 100644
> > > --- a/arch/arm64/kernel/stacktrace.c
> > > +++ b/arch/arm64/kernel/stacktrace.c
> > > @@ -43,6 +43,7 @@
> > >  int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> > >  {
> > >  	unsigned long fp = frame->fp;
> > > +	struct stack_info info, prev_info;
> > >  
> > >  	if (fp & 0xf)
> > >  		return -EINVAL;
> > > @@ -50,11 +51,23 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> > >  	if (!tsk)
> > >  		tsk = current;
> > >  
> > > -	if (!on_accessible_stack(tsk, fp, NULL))
> > > +	if (!on_accessible_stack(tsk, fp, &info))
> > >  		return -EINVAL;
> > >  
> > > +	if (test_bit(info.type, frame->stacks_done))
> > > +		return -EINVAL;
> > > +
> > > +	if (info.type == frame->prev_type) {
> > > +		if (fp <= frame->prev_fp)
> > > +			return -EINVAL;
> > > +	} else {
> > > +		set_bit(prev_info.type, frame->stacks_done);
> > > +	}
> > > +
> > >  	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;
> > 
> > As in my response on the last series, do we really need to track 2
> > frames at the same time in struct stackframe?
> 
> It's better to think of this as tracking the location and contents of
> one stackframe.
> 
> If we back up a bit, I want to ensure that if we have a chain A->B->C
> and the B->C transition is bogus, we report A and B in the backtrace.
> 
> The struct stackframe is a snapshot of the frame record A (which may
> have been the FP and LR rather than an in-memory record). The contents
> of A tells us where the B can be found, but we need the location of A so
> that we can check intra-stack whether A < B.
> 
> > The transition (i.e., where we came from and where we're going to should
> > be visible in unwind_frame().  I don't see why we need additional history
> > in order to detect stack changes or track which stacks have been visited.
> 
> Please see my prior reply to James, where I described this.
> 
> > So, say (incomplete paste-o-code):
>  
> Let's consider this for the above A->B->C transition.
> 
> > 	unsigned long fp = frame->fp;
> > 	enum stack_type fp_stack = frame->fp_stack;
> 
> When we enter with frame describing A, frame->fp points at B, and
> frame->fp_stack describes where A is (i.e. it describes the same thing
> as prev_type in my code).
> 
> > 
> >   	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
> >   	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
> 
> Here we update frame to be the contents of B.
> 
> > 	if (!on_accessible_stack(tsk, frame->fp, &info))
> > 		return -EINVAL;
> 
> ... and here we bail out if C is inaccessible, before we report B.
> 
> > 
> > 	if (test_bit(info.type, frame->stacks_done))
> > 		return -EINVAL;
> 
> Likewise, here we check C's stack type, and bail out an entry earlier
> than necessary.
> 
> > 
> > 	if (info.type == frame->fp_stack) {
> 
> We haven't updated frame->fp_stack  yet, so this is comparing A's
> location to C's location, ignoring B.

No it isn't.  on_accessible_stack() updated info.type to describe B (I
think).

I think I've understood, though: the bad transition is detected when
calling unwind_frame() on the prior frame to the transitioning frame.
So, my version causes us to abandon the backtrace one frame early.

I still think that can be delayed in a more straightforward way
by sticking in "invalid" flag in struct stackframe, then checking
that as the first step of unwind_frame().

However, the logic of your updated patch makes sense to me now.  I have
a suspicion that we're somehow making this problem more complex than it
is, but unless I can come up with something better, I'll shut up!

Cheers
---Dave

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

Re: [PATCHv2 3/3] arm64: stacktrace: better handle corrupted stacks

[Mark Rutland]

On Mon, Jul 01, 2019 at 03:16:20PM +0100, Dave Martin wrote:
> On Mon, Jul 01, 2019 at 01:53:52PM +0100, Mark Rutland wrote:
> > On Mon, Jul 01, 2019 at 11:56:56AM +0100, Dave Martin wrote:
> > > On Fri, Jun 28, 2019 at 04:46:39PM +0100, Mark Rutland wrote:
> > > > The arm64 stacktrace code is careful to only dereference frame records
> > > > in valid stack ranges, ensuring that a corrupted frame record won't
> > > > result in a faulting access.
> > > > 
> > > > However, it's still possible for corrupt frame records to result in
> > > > infinite loops in the stacktrace code, which is also undesirable.
> > > > 
> > > > This patch ensures that we complete a stacktrace in finite time, by
> > > > keeping track of which stacks we have already completed unwinding, and
> > > > verifying that if the next frame record is on the same stack, it is at a
> > > > higher address.
> > > > 
> > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > Cc: Dave Martin <dave.martin@arm.com>
> > > > Cc: James Morse <james.morse@arm.com>
> > > > Cc: Tengfei Fan <tengfeif@codeaurora.org>
> > > > Cc: Will Deacon <will.deacon@arm.com>
> > > > ---
> > > >  arch/arm64/include/asm/stacktrace.h | 32 ++++++++++++++++++++++++--------
> > > >  arch/arm64/kernel/stacktrace.c      | 15 ++++++++++++++-
> > > >  2 files changed, 38 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> > > > index 18f90bf1385c..938b96ba1f0f 100644
> > > > --- a/arch/arm64/include/asm/stacktrace.h
> > > > +++ b/arch/arm64/include/asm/stacktrace.h
> > > > @@ -19,19 +19,12 @@
> > > >  #include <linux/percpu.h>
> > > >  #include <linux/sched.h>
> > > >  #include <linux/sched/task_stack.h>
> > > > +#include <linux/types.h>
> > > >  
> > > >  #include <asm/memory.h>
> > > >  #include <asm/ptrace.h>
> > > >  #include <asm/sdei.h>
> > > >  
> > > > -struct stackframe {
> > > > -	unsigned long fp;
> > > > -	unsigned long pc;
> > > > -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > > -	int graph;
> > > > -#endif
> > > > -};
> > > > -
> > > >  enum stack_type {
> > > >  	STACK_TYPE_UNKNOWN,
> > > >  	STACK_TYPE_TASK,
> > > > @@ -39,6 +32,7 @@ enum stack_type {
> > > >  	STACK_TYPE_OVERFLOW,
> > > >  	STACK_TYPE_SDEI_NORMAL,
> > > >  	STACK_TYPE_SDEI_CRITICAL,
> > > > +	__NR_STACK_TYPES
> > > >  };
> > > >  
> > > >  struct stack_info {
> > > > @@ -47,6 +41,17 @@ struct stack_info {
> > > >  	enum stack_type type;
> > > >  };
> > > >  
> > > > +struct stackframe {
> > > > +	unsigned long fp;
> > > > +	unsigned long pc;
> > > > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > > +	int graph;
> > > > +#endif
> > > > +	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
> > > > +	unsigned long prev_fp;
> > > > +	enum stack_type prev_type;
> > > > +};
> > > > +
> > > >  extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
> > > >  extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
> > > >  			    int (*fn)(struct stackframe *, void *), void *data);
> > > > @@ -128,6 +133,9 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
> > > >  				       unsigned long sp,
> > > >  				       struct stack_info *info)
> > > >  {
> > > > +	if (info)
> > > > +		info->type = STACK_TYPE_UNKNOWN;
> > > > +
> > > >  	if (on_task_stack(tsk, sp, info))
> > > >  		return true;
> > > >  	if (tsk != current || preemptible())
> > > > @@ -150,6 +158,14 @@ static inline void start_backtrace(struct stackframe *frame,
> > > >  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > >  	frame->graph = 0;
> > > >  #endif
> > > > +
> > > > +	/*
> > > > +	 * Prime the first unwind, which will be treated as a transition from
> > > > +	 * STACK_TYPE_UNKNOWN to some valid stack.
> > > > +	 */
> > > > +	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
> > > > +	frame->prev_fp = 0;
> > > > +	frame->prev_type = STACK_TYPE_UNKNOWN;
> > > >  }
> > > >  
> > > >  #endif	/* __ASM_STACKTRACE_H */
> > > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > > > index e5338216deaa..2e4b59e10e71 100644
> > > > --- a/arch/arm64/kernel/stacktrace.c
> > > > +++ b/arch/arm64/kernel/stacktrace.c
> > > > @@ -43,6 +43,7 @@
> > > >  int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> > > >  {
> > > >  	unsigned long fp = frame->fp;
> > > > +	struct stack_info info, prev_info;
> > > >  
> > > >  	if (fp & 0xf)
> > > >  		return -EINVAL;
> > > > @@ -50,11 +51,23 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> > > >  	if (!tsk)
> > > >  		tsk = current;
> > > >  
> > > > -	if (!on_accessible_stack(tsk, fp, NULL))
> > > > +	if (!on_accessible_stack(tsk, fp, &info))
> > > >  		return -EINVAL;
> > > >  
> > > > +	if (test_bit(info.type, frame->stacks_done))
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (info.type == frame->prev_type) {
> > > > +		if (fp <= frame->prev_fp)
> > > > +			return -EINVAL;
> > > > +	} else {
> > > > +		set_bit(prev_info.type, frame->stacks_done);
> > > > +	}
> > > > +
> > > >  	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;
> > > 
> > > As in my response on the last series, do we really need to track 2
> > > frames at the same time in struct stackframe?
> > 
> > It's better to think of this as tracking the location and contents of
> > one stackframe.
> > 
> > If we back up a bit, I want to ensure that if we have a chain A->B->C
> > and the B->C transition is bogus, we report A and B in the backtrace.
> > 
> > The struct stackframe is a snapshot of the frame record A (which may
> > have been the FP and LR rather than an in-memory record). The contents
> > of A tells us where the B can be found, but we need the location of A so
> > that we can check intra-stack whether A < B.
> > 
> > > The transition (i.e., where we came from and where we're going to should
> > > be visible in unwind_frame().  I don't see why we need additional history
> > > in order to detect stack changes or track which stacks have been visited.
> > 
> > Please see my prior reply to James, where I described this.
> > 
> > > So, say (incomplete paste-o-code):
> >  
> > Let's consider this for the above A->B->C transition.
> > 
> > > 	unsigned long fp = frame->fp;
> > > 	enum stack_type fp_stack = frame->fp_stack;
> > 
> > When we enter with frame describing A, frame->fp points at B, and
> > frame->fp_stack describes where A is (i.e. it describes the same thing
> > as prev_type in my code).
> > 
> > > 
> > >   	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
> > >   	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
> > 
> > Here we update frame to be the contents of B.
> > 
> > > 	if (!on_accessible_stack(tsk, frame->fp, &info))
> > > 		return -EINVAL;
> > 
> > ... and here we bail out if C is inaccessible, before we report B.
> > 
> > > 
> > > 	if (test_bit(info.type, frame->stacks_done))
> > > 		return -EINVAL;
> > 
> > Likewise, here we check C's stack type, and bail out an entry earlier
> > than necessary.
> > 
> > > 
> > > 	if (info.type == frame->fp_stack) {
> > 
> > We haven't updated frame->fp_stack  yet, so this is comparing A's
> > location to C's location, ignoring B.
> 
> No it isn't.  on_accessible_stack() updated info.type to describe B (I
> think).

it describes C:

* At entry to the function, frame has the values of A, and thus
  frame->fp pointed at B.

* We updated frame with B's values, reading the fp and pc. Thus after
  the READ_ONCE_NOCHECK()s, frame->fp points at C.

* We checked which stack C was on with:
  on_accessible_stack(tsk, frame->fp, &info);

  ... and thus info describes C.

> I think I've understood, though: the bad transition is detected when
> calling unwind_frame() on the prior frame to the transitioning frame.
> So, my version causes us to abandon the backtrace one frame early.

Yup!

> I still think that can be delayed in a more straightforward way
> by sticking in "invalid" flag in struct stackframe, then checking
> that as the first step of unwind_frame().
> 
> However, the logic of your updated patch makes sense to me now.  I have
> a suspicion that we're somehow making this problem more complex than it
> is, but unless I can come up with something better, I'll shut up!

I've tried a number of approaches now, and this is about as simple as it
can be, I'm afraid. Every simplification ended up missing some info
necessary for an edge case. At minimum we need the previous FP value,
AFAICT.

I will try to write a comprehensive comment to explain what we're doing
and why.

Thanks,
Mark.

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

Re: [PATCHv2 3/3] arm64: stacktrace: better handle corrupted stacks

[James Morse]

Hi Mark,

On 28/06/2019 16:46, Mark Rutland wrote:
> The arm64 stacktrace code is careful to only dereference frame records
> in valid stack ranges, ensuring that a corrupted frame record won't
> result in a faulting access.
> 
> However, it's still possible for corrupt frame records to result in
> infinite loops in the stacktrace code, which is also undesirable.
> 
> This patch ensures that we complete a stacktrace in finite time, by
> keeping track of which stacks we have already completed unwinding, and
> verifying that if the next frame record is on the same stack, it is at a
> higher address.


> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 18f90bf1385c..938b96ba1f0f 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -47,6 +41,17 @@ struct stack_info {
>  	enum stack_type type;
>  };
>  
> +struct stackframe {
> +	unsigned long fp;
> +	unsigned long pc;

> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +	int graph;
> +#endif

(Could we get rid of this #ifdef? It just prevents us using IS_ENABLED() elsewhere)


> +	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
> +	unsigned long prev_fp;
> +	enum stack_type prev_type;
> +};
> +
>  extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
>  extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
>  			    int (*fn)(struct stackframe *, void *), void *data);


> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index e5338216deaa..2e4b59e10e71 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -43,6 +43,7 @@
>  int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  {
>  	unsigned long fp = frame->fp;
> +	struct stack_info info, prev_info;
>  
>  	if (fp & 0xf)
>  		return -EINVAL;
> @@ -50,11 +51,23 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
>  	if (!tsk)
>  		tsk = current;
>  
> -	if (!on_accessible_stack(tsk, fp, NULL))
> +	if (!on_accessible_stack(tsk, fp, &info))
>  		return -EINVAL;
>  
> +	if (test_bit(info.type, frame->stacks_done))
> +		return -EINVAL;
> +
> +	if (info.type == frame->prev_type) {
> +		if (fp <= frame->prev_fp)
> +			return -EINVAL;
> +	} else {

> +		set_bit(prev_info.type, frame->stacks_done);

What is prev_info for? This looks like we're setting $uninitialised_stack bit.


> +	}
> +
>  	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;


With prev_info, this doesn't boot when lockdep is enabled. The pre-linear-map stacktrace
calls generate bad addresses and we panic() before earlycon.

With that set_bit() line changed to read:
|		set_bit(frame->prev_type, frame->stacks_done);

this works with perf and ftrace, and stepping off the sdei stack.

Reviewed-by: James Morse <james.morse@arm.com>
Tested-by: James Morse <james.morse@arm.com>


Thanks,

James

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

Re: [PATCHv2 2/3] arm64: stacktrace: Factor out backtrace initialisation

[James Morse]

Hi Mark, Dave,

On 28/06/2019 16:46, Mark Rutland wrote:
> From: Dave Martin <Dave.Martin@arm.com>
> 
> Some common code is required by each stacktrace user to initialise
> struct stackframe before the first call to unwind_frame().
> 
> In preparation for adding to the common code, this patch factors it
> out into a separate function start_backtrace(), and modifies the
> stacktrace callers appropriately.
> 
> No functional change.

Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James

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

Re: [PATCHv2 3/3] arm64: stacktrace: better handle corrupted stacks

[Mark Rutland]

On Tue, Jul 02, 2019 at 10:33:51AM +0100, James Morse wrote:
> On 28/06/2019 16:46, Mark Rutland wrote:
> > +struct stackframe {
> > +	unsigned long fp;
> > +	unsigned long pc;
> 
> > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > +	int graph;
> > +#endif
> 
> (Could we get rid of this #ifdef? It just prevents us using IS_ENABLED() elsewhere)

I think we could, but I think that would be a separate patch.

I can look at that as a followup.

> > @@ -50,11 +51,23 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> >  	if (!tsk)
> >  		tsk = current;
> >  
> > -	if (!on_accessible_stack(tsk, fp, NULL))
> > +	if (!on_accessible_stack(tsk, fp, &info))
> >  		return -EINVAL;
> >  
> > +	if (test_bit(info.type, frame->stacks_done))
> > +		return -EINVAL;
> > +
> > +	if (info.type == frame->prev_type) {
> > +		if (fp <= frame->prev_fp)
> > +			return -EINVAL;
> > +	} else {
> 
> > +		set_bit(prev_info.type, frame->stacks_done);
> 
> What is prev_info for? This looks like we're setting $uninitialised_stack bit.

Ugh -- that's a leftover from before I added stack_info::prev_type, when
I always derived the type from prev_fp. Having prev_type ends up much
simpler.

> > +	}
> > +
> >  	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;
> 
> 
> With prev_info, this doesn't boot when lockdep is enabled. The pre-linear-map stacktrace
> calls generate bad addresses and we panic() before earlycon.
> 
> With that set_bit() line changed to read:
> |		set_bit(frame->prev_type, frame->stacks_done);
> 
> this works with perf and ftrace, and stepping off the sdei stack.

Thanks, I'll fold that in

> Reviewed-by: James Morse <james.morse@arm.com>
> Tested-by: James Morse <james.morse@arm.com>

Cheers!

Mark.

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

Re: [PATCHv2 3/3] arm64: stacktrace: better handle corrupted stacks

[Dave Martin]

On Mon, Jul 01, 2019 at 03:33:00PM +0100, Mark Rutland wrote:
> On Mon, Jul 01, 2019 at 03:16:20PM +0100, Dave Martin wrote:
> > On Mon, Jul 01, 2019 at 01:53:52PM +0100, Mark Rutland wrote:
> > > On Mon, Jul 01, 2019 at 11:56:56AM +0100, Dave Martin wrote:
> > > > On Fri, Jun 28, 2019 at 04:46:39PM +0100, Mark Rutland wrote:
> > > > > The arm64 stacktrace code is careful to only dereference frame records
> > > > > in valid stack ranges, ensuring that a corrupted frame record won't
> > > > > result in a faulting access.
> > > > > 
> > > > > However, it's still possible for corrupt frame records to result in
> > > > > infinite loops in the stacktrace code, which is also undesirable.
> > > > > 
> > > > > This patch ensures that we complete a stacktrace in finite time, by
> > > > > keeping track of which stacks we have already completed unwinding, and
> > > > > verifying that if the next frame record is on the same stack, it is at a
> > > > > higher address.
> > > > > 
> > > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > > > Cc: Dave Martin <dave.martin@arm.com>
> > > > > Cc: James Morse <james.morse@arm.com>
> > > > > Cc: Tengfei Fan <tengfeif@codeaurora.org>
> > > > > Cc: Will Deacon <will.deacon@arm.com>
> > > > > ---
> > > > >  arch/arm64/include/asm/stacktrace.h | 32 ++++++++++++++++++++++++--------
> > > > >  arch/arm64/kernel/stacktrace.c      | 15 ++++++++++++++-
> > > > >  2 files changed, 38 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> > > > > index 18f90bf1385c..938b96ba1f0f 100644
> > > > > --- a/arch/arm64/include/asm/stacktrace.h
> > > > > +++ b/arch/arm64/include/asm/stacktrace.h
> > > > > @@ -19,19 +19,12 @@
> > > > >  #include <linux/percpu.h>
> > > > >  #include <linux/sched.h>
> > > > >  #include <linux/sched/task_stack.h>
> > > > > +#include <linux/types.h>
> > > > >  
> > > > >  #include <asm/memory.h>
> > > > >  #include <asm/ptrace.h>
> > > > >  #include <asm/sdei.h>
> > > > >  
> > > > > -struct stackframe {
> > > > > -	unsigned long fp;
> > > > > -	unsigned long pc;
> > > > > -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > > > -	int graph;
> > > > > -#endif
> > > > > -};
> > > > > -
> > > > >  enum stack_type {
> > > > >  	STACK_TYPE_UNKNOWN,
> > > > >  	STACK_TYPE_TASK,
> > > > > @@ -39,6 +32,7 @@ enum stack_type {
> > > > >  	STACK_TYPE_OVERFLOW,
> > > > >  	STACK_TYPE_SDEI_NORMAL,
> > > > >  	STACK_TYPE_SDEI_CRITICAL,
> > > > > +	__NR_STACK_TYPES
> > > > >  };
> > > > >  
> > > > >  struct stack_info {
> > > > > @@ -47,6 +41,17 @@ struct stack_info {
> > > > >  	enum stack_type type;
> > > > >  };
> > > > >  
> > > > > +struct stackframe {
> > > > > +	unsigned long fp;
> > > > > +	unsigned long pc;
> > > > > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > > > +	int graph;
> > > > > +#endif
> > > > > +	DECLARE_BITMAP(stacks_done, __NR_STACK_TYPES);
> > > > > +	unsigned long prev_fp;
> > > > > +	enum stack_type prev_type;
> > > > > +};
> > > > > +
> > > > >  extern int unwind_frame(struct task_struct *tsk, struct stackframe *frame);
> > > > >  extern void walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
> > > > >  			    int (*fn)(struct stackframe *, void *), void *data);
> > > > > @@ -128,6 +133,9 @@ static inline bool on_accessible_stack(const struct task_struct *tsk,
> > > > >  				       unsigned long sp,
> > > > >  				       struct stack_info *info)
> > > > >  {
> > > > > +	if (info)
> > > > > +		info->type = STACK_TYPE_UNKNOWN;
> > > > > +
> > > > >  	if (on_task_stack(tsk, sp, info))
> > > > >  		return true;
> > > > >  	if (tsk != current || preemptible())
> > > > > @@ -150,6 +158,14 @@ static inline void start_backtrace(struct stackframe *frame,
> > > > >  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
> > > > >  	frame->graph = 0;
> > > > >  #endif
> > > > > +
> > > > > +	/*
> > > > > +	 * Prime the first unwind, which will be treated as a transition from
> > > > > +	 * STACK_TYPE_UNKNOWN to some valid stack.
> > > > > +	 */
> > > > > +	bitmap_zero(frame->stacks_done, __NR_STACK_TYPES);
> > > > > +	frame->prev_fp = 0;
> > > > > +	frame->prev_type = STACK_TYPE_UNKNOWN;
> > > > >  }
> > > > >  
> > > > >  #endif	/* __ASM_STACKTRACE_H */
> > > > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > > > > index e5338216deaa..2e4b59e10e71 100644
> > > > > --- a/arch/arm64/kernel/stacktrace.c
> > > > > +++ b/arch/arm64/kernel/stacktrace.c
> > > > > @@ -43,6 +43,7 @@
> > > > >  int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> > > > >  {
> > > > >  	unsigned long fp = frame->fp;
> > > > > +	struct stack_info info, prev_info;
> > > > >  
> > > > >  	if (fp & 0xf)
> > > > >  		return -EINVAL;
> > > > > @@ -50,11 +51,23 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> > > > >  	if (!tsk)
> > > > >  		tsk = current;
> > > > >  
> > > > > -	if (!on_accessible_stack(tsk, fp, NULL))
> > > > > +	if (!on_accessible_stack(tsk, fp, &info))
> > > > >  		return -EINVAL;
> > > > >  
> > > > > +	if (test_bit(info.type, frame->stacks_done))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	if (info.type == frame->prev_type) {
> > > > > +		if (fp <= frame->prev_fp)
> > > > > +			return -EINVAL;
> > > > > +	} else {
> > > > > +		set_bit(prev_info.type, frame->stacks_done);
> > > > > +	}
> > > > > +
> > > > >  	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;
> > > > 
> > > > As in my response on the last series, do we really need to track 2
> > > > frames at the same time in struct stackframe?
> > > 
> > > It's better to think of this as tracking the location and contents of
> > > one stackframe.
> > > 
> > > If we back up a bit, I want to ensure that if we have a chain A->B->C
> > > and the B->C transition is bogus, we report A and B in the backtrace.
> > > 
> > > The struct stackframe is a snapshot of the frame record A (which may
> > > have been the FP and LR rather than an in-memory record). The contents
> > > of A tells us where the B can be found, but we need the location of A so
> > > that we can check intra-stack whether A < B.
> > > 
> > > > The transition (i.e., where we came from and where we're going to should
> > > > be visible in unwind_frame().  I don't see why we need additional history
> > > > in order to detect stack changes or track which stacks have been visited.
> > > 
> > > Please see my prior reply to James, where I described this.
> > > 
> > > > So, say (incomplete paste-o-code):
> > >  
> > > Let's consider this for the above A->B->C transition.
> > > 
> > > > 	unsigned long fp = frame->fp;
> > > > 	enum stack_type fp_stack = frame->fp_stack;
> > > 
> > > When we enter with frame describing A, frame->fp points at B, and
> > > frame->fp_stack describes where A is (i.e. it describes the same thing
> > > as prev_type in my code).
> > > 
> > > > 
> > > >   	frame->fp = READ_ONCE_NOCHECK(*(unsigned long *)(fp));
> > > >   	frame->pc = READ_ONCE_NOCHECK(*(unsigned long *)(fp + 8));
> > > 
> > > Here we update frame to be the contents of B.
> > > 
> > > > 	if (!on_accessible_stack(tsk, frame->fp, &info))
> > > > 		return -EINVAL;
> > > 
> > > ... and here we bail out if C is inaccessible, before we report B.
> > > 
> > > > 
> > > > 	if (test_bit(info.type, frame->stacks_done))
> > > > 		return -EINVAL;
> > > 
> > > Likewise, here we check C's stack type, and bail out an entry earlier
> > > than necessary.
> > > 
> > > > 
> > > > 	if (info.type == frame->fp_stack) {
> > > 
> > > We haven't updated frame->fp_stack  yet, so this is comparing A's
> > > location to C's location, ignoring B.
> > 
> > No it isn't.  on_accessible_stack() updated info.type to describe B (I
> > think).
> 
> it describes C:
> 
> * At entry to the function, frame has the values of A, and thus
>   frame->fp pointed at B.
> 
> * We updated frame with B's values, reading the fp and pc. Thus after
>   the READ_ONCE_NOCHECK()s, frame->fp points at C.
> 
> * We checked which stack C was on with:
>   on_accessible_stack(tsk, frame->fp, &info);
> 
>   ... and thus info describes C.
> 
> > I think I've understood, though: the bad transition is detected when
> > calling unwind_frame() on the prior frame to the transitioning frame.
> > So, my version causes us to abandon the backtrace one frame early.
> 
> Yup!
> 
> > I still think that can be delayed in a more straightforward way
> > by sticking in "invalid" flag in struct stackframe, then checking
> > that as the first step of unwind_frame().
> > 
> > However, the logic of your updated patch makes sense to me now.  I have
> > a suspicion that we're somehow making this problem more complex than it
> > is, but unless I can come up with something better, I'll shut up!
> 
> I've tried a number of approaches now, and this is about as simple as it
> can be, I'm afraid. Every simplification ended up missing some info
> necessary for an edge case. At minimum we need the previous FP value,
> AFAICT.
> 
> I will try to write a comprehensive comment to explain what we're doing
> and why.

Sounds good.

I'm still not that keen on the new approach, but getting rid of the
pipelined state in struct stackframe seems to require additional
refactoring elsewhere, and I'm not sure it's worth it given that we'd
probably introduce new bugs in the process...

So, FWIW,

Acked-by: Dave Martin <Dave.Martin@arm.com>

Cheers
---Dave

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