diff mbox series

[v2,3/4] arm64: implement stack_trace_save_shadow

Message ID 0bb72ea8fa88ef9ae3508c23d993952a0ae6f0f9.1648049113.git.andreyknvl@google.com (mailing list archive)
State New
Headers show
Series kasan, arm64, scs, stacktrace: collect stack traces from Shadow Call Stack | expand

Commit Message

andrey.konovalov@linux.dev March 23, 2022, 3:32 p.m. UTC 
From: Andrey Konovalov <andreyknvl@google.com>

Implement the stack_trace_save_shadow() interface that collects stack
traces based on the Shadow Call Stack (SCS) for arm64.

The implementation walks through available SCS pointers (the per-task one
and the per-interrupt-type ones) and copies the frames.

Note that the frame of the interrupted function is not included into
the stack trace, as it is not yet saved on the SCS when an interrupt
happens.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 arch/arm64/Kconfig             |  1 +
 arch/arm64/kernel/stacktrace.c | 83 ++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

Comments

kernel test robot March 24, 2022, 8:35 a.m. UTC | #1 
Hi,

I love your patch! Perhaps something to improve:

[auto build test WARNING on next-20220323]
[cannot apply to arm64/for-next/core hnaz-mm/master linus/master v5.17 v5.17-rc8 v5.17-rc7 v5.17]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/andrey-konovalov-linux-dev/kasan-arm64-scs-stacktrace-collect-stack-traces-from-Shadow-Call-Stack/20220323-233436
base:    b61581ae229d8eb9f21f8753be3f4011f7692384
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20220324/202203241622.fKuBI2l5-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/322e934f3c0bb04b4afb32207ba142153f1dd84e
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review andrey-konovalov-linux-dev/kasan-arm64-scs-stacktrace-collect-stack-traces-from-Shadow-Call-Stack/20220323-233436
        git checkout 322e934f3c0bb04b4afb32207ba142153f1dd84e
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm64 SHELL=/bin/bash arch/arm64/kernel/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   arch/arm64/kernel/stacktrace.c: In function 'arch_stack_walk_shadow':
   arch/arm64/kernel/stacktrace.c:289:20: error: implicit declaration of function 'task_scs'; did you mean 'task_lock'? [-Werror=implicit-function-declaration]
     289 |         scs_base = task_scs(current);
         |                    ^~~~~~~~
         |                    task_lock
>> arch/arm64/kernel/stacktrace.c:289:18: warning: assignment to 'long unsigned int *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     289 |         scs_base = task_scs(current);
         |                  ^
   cc1: some warnings being treated as errors


vim +289 arch/arm64/kernel/stacktrace.c

   260	
   261	noinline notrace int arch_stack_walk_shadow(unsigned long *store,
   262						    unsigned int size,
   263						    unsigned int skipnr)
   264	{
   265		unsigned long *scs_top, *scs_base, *scs_next;
   266		unsigned int len = 0, part;
   267	
   268		preempt_disable();
   269	
   270		/* Get the SCS pointer. */
   271		asm volatile("mov %0, x18" : "=&r" (scs_top));
   272	
   273		/* The top SCS slot is empty. */
   274		scs_top -= 1;
   275	
   276		/* Handle SDEI and hardirq frames. */
   277		for (part = 0; part < ARRAY_SIZE(scs_parts); part++) {
   278			scs_next = *this_cpu_ptr(scs_parts[part].saved);
   279			if (scs_next) {
   280				scs_base = *this_cpu_ptr(scs_parts[part].base);
   281				if (walk_shadow_stack_part(scs_top, scs_base, store,
   282							   size, &skipnr, &len))
   283					goto out;
   284				scs_top = scs_next;
   285			}
   286		}
   287	
   288		/* Handle task and softirq frames. */
 > 289		scs_base = task_scs(current);
Mark Rutland March 31, 2022, 9:32 a.m. UTC | #2 
On Wed, Mar 23, 2022 at 04:32:54PM +0100, andrey.konovalov@linux.dev wrote:
> From: Andrey Konovalov <andreyknvl@google.com>
> 
> Implement the stack_trace_save_shadow() interface that collects stack
> traces based on the Shadow Call Stack (SCS) for arm64.
> 
> The implementation walks through available SCS pointers (the per-task one
> and the per-interrupt-type ones) and copies the frames.
> 
> Note that the frame of the interrupted function is not included into
> the stack trace, as it is not yet saved on the SCS when an interrupt
> happens.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  arch/arm64/Kconfig             |  1 +
>  arch/arm64/kernel/stacktrace.c | 83 ++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index a659e238f196..d89cecf6c923 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -201,6 +201,7 @@ config ARM64
>  	select MMU_GATHER_RCU_TABLE_FREE
>  	select HAVE_RSEQ
>  	select HAVE_RUST
> +	select HAVE_SHADOW_STACKTRACE
>  	select HAVE_STACKPROTECTOR
>  	select HAVE_SYSCALL_TRACEPOINTS
>  	select HAVE_KPROBES
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index e4103e085681..89daa710d91b 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -12,9 +12,11 @@
>  #include <linux/sched/debug.h>
>  #include <linux/sched/task_stack.h>
>  #include <linux/stacktrace.h>
> +#include <linux/scs.h>
>  
>  #include <asm/irq.h>
>  #include <asm/pointer_auth.h>
> +#include <asm/scs.h>
>  #include <asm/stack_pointer.h>
>  #include <asm/stacktrace.h>
>  
> @@ -210,3 +212,84 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>  
>  	walk_stackframe(task, &frame, consume_entry, cookie);
>  }
> +
> +static const struct {
> +	unsigned long ** __percpu saved;
> +	unsigned long ** __percpu base;
> +} scs_parts[] = {
> +#ifdef CONFIG_ARM_SDE_INTERFACE
> +	{
> +		.saved = &sdei_shadow_call_stack_critical_saved_ptr,
> +		.base = &sdei_shadow_call_stack_critical_ptr,
> +	},
> +	{
> +		.saved = &sdei_shadow_call_stack_normal_saved_ptr,
> +		.base = &sdei_shadow_call_stack_normal_ptr,
> +	},
> +#endif /* CONFIG_ARM_SDE_INTERFACE */
> +	{
> +		.saved = &irq_shadow_call_stack_saved_ptr,
> +		.base = &irq_shadow_call_stack_ptr,
> +	},
> +};
> +
> +static inline bool walk_shadow_stack_part(
> +				unsigned long *scs_top, unsigned long *scs_base,
> +				unsigned long *store, unsigned int size,
> +				unsigned int *skipnr, unsigned int *len)
> +{
> +	unsigned long *frame;
> +
> +	for (frame = scs_top; frame >= scs_base; frame--) {
> +		if (*skipnr > 0) {
> +			(*skipnr)--;
> +			continue;
> +		}
> +		/*
> +		 * Do not leak PTR_AUTH tags in stack traces.
> +		 * Use READ_ONCE_NOCHECK as SCS is poisoned with Generic KASAN.
> +		 */
> +		store[(*len)++] =
> +			ptrauth_strip_insn_pac(READ_ONCE_NOCHECK(*frame));
> +		if (*len >= size)
> +			return true;
> +	}
> +
> +	return false;
> +}

This doesn't do any of the trampoline repatinting (e.g. for kretprobes or
ftrace graph caller) that the regular unwinder does, so if either of those are
in use this is going to produce bogus results.

I really don't want to have to duplicate this logic.

> +
> +noinline notrace int arch_stack_walk_shadow(unsigned long *store,
> +					    unsigned int size,
> +					    unsigned int skipnr)
> +{
> +	unsigned long *scs_top, *scs_base, *scs_next;
> +	unsigned int len = 0, part;
> +
> +	preempt_disable();

This doesn't look necessary; it's certinaly not needed for the regular unwinder.

Critically, in the common case of unwinding just the task stack, we don't need
to look at any of the per-cpu stacks, and so there's no need to disable
preemption. See the stack nesting logic in the regular unwinder.

If we *do* need to unwind per-cpu stacks, we figure that out and verify our
countext *at* the transition point.

> +
> +	/* Get the SCS pointer. */
> +	asm volatile("mov %0, x18" : "=&r" (scs_top));

Does the compiler guarantee where this happens relative to any prologue
manipulation of x18?

This seems like something we should be using a compilar intrinsic for, or have
a wrapper that passes this in if necessary.

> +
> +	/* The top SCS slot is empty. */
> +	scs_top -= 1;
> +
> +	/* Handle SDEI and hardirq frames. */
> +	for (part = 0; part < ARRAY_SIZE(scs_parts); part++) {
> +		scs_next = *this_cpu_ptr(scs_parts[part].saved);
> +		if (scs_next) {
> +			scs_base = *this_cpu_ptr(scs_parts[part].base);
> +			if (walk_shadow_stack_part(scs_top, scs_base, store,
> +						   size, &skipnr, &len))
> +				goto out;
> +			scs_top = scs_next;
> +		}
> +	}

We have a number of portential stack nesting orders (and may need to introduce
more stacks in future), so I think we need to be more careful with this. The
regular unwinder handles that dynamically.

Thanks,
Mark.

> +
> +	/* Handle task and softirq frames. */
> +	scs_base = task_scs(current);
> +	walk_shadow_stack_part(scs_top, scs_base, store, size, &skipnr, &len);
> +
> +out:
> +	preempt_enable();
> +	return len;
> +}
> -- 
> 2.25.1
>
Andrey Konovalov April 5, 2022, 3:38 p.m. UTC | #3 
On Thu, Mar 31, 2022 at 11:32 AM Mark Rutland <mark.rutland@arm.com> wrote:
>
> This doesn't do any of the trampoline repatinting (e.g. for kretprobes or
> ftrace graph caller) that the regular unwinder does, so if either of those are
> in use this is going to produce bogus results.

Responded on the cover letter wrt this.

> > +noinline notrace int arch_stack_walk_shadow(unsigned long *store,
> > +                                         unsigned int size,
> > +                                         unsigned int skipnr)
> > +{
> > +     unsigned long *scs_top, *scs_base, *scs_next;
> > +     unsigned int len = 0, part;
> > +
> > +     preempt_disable();
>
> This doesn't look necessary; it's certinaly not needed for the regular unwinder.
>
> Critically, in the common case of unwinding just the task stack, we don't need
> to look at any of the per-cpu stacks, and so there's no need to disable
> preemption. See the stack nesting logic in the regular unwinder.

The common unwinder doesn't access per-cpu variables, so
preempt_disable() is not required.

Although, in this case, the per-cpu variable is read-only, so
preempt_disable() is probably also not required. Unless LOCKDEP or
some other tools complain about this.

> If we *do* need to unwind per-cpu stacks, we figure that out and verify our
> countext *at* the transition point.

I'm not sure I understand this statement. You mean we need to keep the
currently relevant SCS stack base and update it in interrupt handlers?
This will require modifying the entry code.

> > +
> > +     /* Get the SCS pointer. */
> > +     asm volatile("mov %0, x18" : "=&r" (scs_top));
>
> Does the compiler guarantee where this happens relative to any prologue
> manipulation of x18?
>
> This seems like something we should be using a compilar intrinsic for, or have
> a wrapper that passes this in if necessary.

This is a good point, I'll investigate this.

> > +
> > +     /* The top SCS slot is empty. */
> > +     scs_top -= 1;
> > +
> > +     /* Handle SDEI and hardirq frames. */
> > +     for (part = 0; part < ARRAY_SIZE(scs_parts); part++) {
> > +             scs_next = *this_cpu_ptr(scs_parts[part].saved);
> > +             if (scs_next) {
> > +                     scs_base = *this_cpu_ptr(scs_parts[part].base);
> > +                     if (walk_shadow_stack_part(scs_top, scs_base, store,
> > +                                                size, &skipnr, &len))
> > +                             goto out;
> > +                     scs_top = scs_next;
> > +             }
> > +     }
>
> We have a number of portential stack nesting orders (and may need to introduce
> more stacks in future), so I think we need to be more careful with this. The
> regular unwinder handles that dynamically.

I'll rewrite this part based on the other comments, so let's discuss it then.

Thanks!
diff mbox series

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index a659e238f196..d89cecf6c923 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -201,6 +201,7 @@  config ARM64
 	select MMU_GATHER_RCU_TABLE_FREE
 	select HAVE_RSEQ
 	select HAVE_RUST
+	select HAVE_SHADOW_STACKTRACE
 	select HAVE_STACKPROTECTOR
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_KPROBES
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index e4103e085681..89daa710d91b 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -12,9 +12,11 @@ 
 #include <linux/sched/debug.h>
 #include <linux/sched/task_stack.h>
 #include <linux/stacktrace.h>
+#include <linux/scs.h>
 
 #include <asm/irq.h>
 #include <asm/pointer_auth.h>
+#include <asm/scs.h>
 #include <asm/stack_pointer.h>
 #include <asm/stacktrace.h>
 
@@ -210,3 +212,84 @@  noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
 
 	walk_stackframe(task, &frame, consume_entry, cookie);
 }
+
+static const struct {
+	unsigned long ** __percpu saved;
+	unsigned long ** __percpu base;
+} scs_parts[] = {
+#ifdef CONFIG_ARM_SDE_INTERFACE
+	{
+		.saved = &sdei_shadow_call_stack_critical_saved_ptr,
+		.base = &sdei_shadow_call_stack_critical_ptr,
+	},
+	{
+		.saved = &sdei_shadow_call_stack_normal_saved_ptr,
+		.base = &sdei_shadow_call_stack_normal_ptr,
+	},
+#endif /* CONFIG_ARM_SDE_INTERFACE */
+	{
+		.saved = &irq_shadow_call_stack_saved_ptr,
+		.base = &irq_shadow_call_stack_ptr,
+	},
+};
+
+static inline bool walk_shadow_stack_part(
+				unsigned long *scs_top, unsigned long *scs_base,
+				unsigned long *store, unsigned int size,
+				unsigned int *skipnr, unsigned int *len)
+{
+	unsigned long *frame;
+
+	for (frame = scs_top; frame >= scs_base; frame--) {
+		if (*skipnr > 0) {
+			(*skipnr)--;
+			continue;
+		}
+		/*
+		 * Do not leak PTR_AUTH tags in stack traces.
+		 * Use READ_ONCE_NOCHECK as SCS is poisoned with Generic KASAN.
+		 */
+		store[(*len)++] =
+			ptrauth_strip_insn_pac(READ_ONCE_NOCHECK(*frame));
+		if (*len >= size)
+			return true;
+	}
+
+	return false;
+}
+
+noinline notrace int arch_stack_walk_shadow(unsigned long *store,
+					    unsigned int size,
+					    unsigned int skipnr)
+{
+	unsigned long *scs_top, *scs_base, *scs_next;
+	unsigned int len = 0, part;
+
+	preempt_disable();
+
+	/* Get the SCS pointer. */
+	asm volatile("mov %0, x18" : "=&r" (scs_top));
+
+	/* The top SCS slot is empty. */
+	scs_top -= 1;
+
+	/* Handle SDEI and hardirq frames. */
+	for (part = 0; part < ARRAY_SIZE(scs_parts); part++) {
+		scs_next = *this_cpu_ptr(scs_parts[part].saved);
+		if (scs_next) {
+			scs_base = *this_cpu_ptr(scs_parts[part].base);
+			if (walk_shadow_stack_part(scs_top, scs_base, store,
+						   size, &skipnr, &len))
+				goto out;
+			scs_top = scs_next;
+		}
+	}
+
+	/* Handle task and softirq frames. */
+	scs_base = task_scs(current);
+	walk_shadow_stack_part(scs_top, scs_base, store, size, &skipnr, &len);
+
+out:
+	preempt_enable();
+	return len;
+}