Re: [PATCH] kasan, scs: collect stack traces from shadow stack

[Andrey Konovalov <andreyknvl@gmail.com>]

On Mon, Mar 14, 2022 at 8:01 AM Marco Elver <elver@google.com> 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!