On Mon, Mar 14, 2022 at 8:01 AM Marco Elver <elver@xxxxxxxxxx> wrote: > > > Instead of invoking the unwinder, collect the stack trace by copying > > frames from the Shadow Call Stack whenever it is enabled. This reduces > > boot time by 30% for all KASAN modes when Shadow Call Stack is enabled. > > This is impressive. I was surprised too. > > We could integrate shadow stack trace collection into kernel/stacktrace.c > > as e.g. stack_trace_save_shadow(). However, using stack_trace_consume_fn > > leads to invoking a callback on each saved from, which is undesirable. > > The plain copy loop is faster. > > Why is stack_trace_consume_fn required? This is an internal detail of > arch_stack_walk(), but to implement stack_trace_save_shadow() that's > not used at all. > > I think having stack_trace_save_shadow() as you have implemented in > kernel/stacktrace.c or simply in kernel/scs.c itself would be > appropriate. The other stack trace routines consistently use on stack_trace_consume_fn. But I think you're right, we don't need it. Will do in v2. > > We could add a command line flag to switch between stack trace collection > > modes. I noticed that Shadow Call Stack might be missing certain frames > > in stacks originating from a fault that happens in the middle of a > > function. I am not sure if this case is important to handle though. > > I think SCS should just work - and if it doesn't, can we fix it? It is > unclear to me what would be a deciding factor to choose between stack > trace collection modes, since it is hard to quantify when and if SCS > doesn't work as intended. So I fear it'd just be an option that's > never used because we don't understand when it's required to be used. Let's just rely on SCS for now and reconsider in case any significant limitations are discovered. > > +#ifdef CONFIG_SHADOW_CALL_STACK > > + > > +#ifdef CONFIG_ARM64_PTR_AUTH > > +#define PAC_TAG_RESET(x) (x | GENMASK(63, CONFIG_ARM64_VA_BITS)) > > This should go into arch/arm64/include/asm/kasan.h, and here it should > then just do > > #ifndef PAC_TAG_RESET > #define ... > > > > +#else > > +#define PAC_TAG_RESET(x) (x) > > +#endif > > But perhaps there's a better, more generic location for this macro? Will move in v2. Thanks!
