Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <57133fafc4d74377a4a08d98e276d58fe4a127dc.1647115974.git.andreyknvl@google.com>
 <CANpmjNNBzVovK=N9b2Lv0VUqpE_4nU+6gqO91_ojVoEbR0C5hA@mail.gmail.com>
In-Reply-To: <CANpmjNNBzVovK=N9b2Lv0VUqpE_4nU+6gqO91_ojVoEbR0C5hA@mail.gmail.com>
From:   Andrey Konovalov <andreyknvl@gmail.com>
Date:   Sun, 20 Mar 2022 22:09:33 +0100
Message-ID: <CA+fCnZfYLZhjijPjp3Wd3ZeBQnKNiQCLNn7uuF=cpQi9wU50xA@mail.gmail.com>
Subject: Re: [PATCH] kasan, scs: collect stack traces from shadow stack
To:     Marco Elver <elver@google.com>
Cc:     andrey.konovalov@linux.dev,
        Alexander Potapenko <glider@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Dmitry Vyukov <dvyukov@google.com>,
        Andrey Ryabinin <ryabinin.a.a@gmail.com>,
        kasan-dev <kasan-dev@googlegroups.com>,
        Vincenzo Frascino <vincenzo.frascino@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Sami Tolvanen <samitolvanen@google.com>,
        Peter Collingbourne <pcc@google.com>,
        Evgenii Stepanov <eugenis@google.com>,
        Linux Memory Management List <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Andrey Konovalov <andreyknvl@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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!