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