Re: [PATCH v2 2/2] arm64: scs: use vmapped IRQ and SDEI shadow stacks

From: Will Deacon <will@kernel.org>
To: Sami Tolvanen <samitolvanen@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Kees Cook <keescook@chromium.org>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	LKML <linux-kernel@vger.kernel.org>,
	James Morse <james.morse@arm.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 2/2] arm64: scs: use vmapped IRQ and SDEI shadow stacks
Date: Tue, 1 Dec 2020 10:18:19 +0000	[thread overview]
Message-ID: <20201201101819.GA26471@willie-the-truck> (raw)
In-Reply-To: <CABCJKud6UiidpqqwJcghnqLKDkqM3pzMUUwe3_HH3ODDqOdANA@mail.gmail.com>

On Mon, Nov 30, 2020 at 01:13:07PM -0800, Sami Tolvanen wrote:
> On Mon, Nov 30, 2020 at 3:49 AM Will Deacon <will@kernel.org> wrote:
> >
> > On Tue, Nov 24, 2020 at 11:59:40AM -0800, Sami Tolvanen wrote:
> > > Use scs_alloc() to allocate also IRQ and SDEI shadow stacks instead of
> > > using statically allocated stacks.
> > >
> > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> > > ---
> > >  arch/arm64/kernel/Makefile |  1 -
> > >  arch/arm64/kernel/entry.S  |  6 ++--
> > >  arch/arm64/kernel/irq.c    | 19 ++++++++++
> > >  arch/arm64/kernel/scs.c    | 16 ---------
> > >  arch/arm64/kernel/sdei.c   | 71 +++++++++++++++++++++++++++++++-------
> > >  include/linux/scs.h        |  4 ---
> > >  6 files changed, 81 insertions(+), 36 deletions(-)
> > >  delete mode 100644 arch/arm64/kernel/scs.c
> >
> > [...]
> >
> > > @@ -70,18 +97,40 @@ static int _init_sdei_stack(unsigned long * __percpu *ptr, int cpu)
> > >       return 0;
> > >  }
> > >
> > > +static int _init_sdei_scs(unsigned long * __percpu *ptr, int cpu)
> > > +{
> > > +     void *s;
> > > +
> > > +     s = scs_alloc(cpu_to_node(cpu));
> > > +     if (!s)
> > > +             return -ENOMEM;
> > > +     per_cpu(*ptr, cpu) = s;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  static int init_sdei_stacks(void)
> > >  {
> > >       int cpu;
> > >       int err = 0;
> > >
> > >       for_each_possible_cpu(cpu) {
> > > -             err = _init_sdei_stack(&sdei_stack_normal_ptr, cpu);
> > > -             if (err)
> > > -                     break;
> > > -             err = _init_sdei_stack(&sdei_stack_critical_ptr, cpu);
> > > -             if (err)
> > > -                     break;
> > > +             if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> > > +                     err = _init_sdei_stack(&sdei_stack_normal_ptr, cpu);
> > > +                     if (err)
> > > +                             break;
> > > +                     err = _init_sdei_stack(&sdei_stack_critical_ptr, cpu);
> > > +                     if (err)
> > > +                             break;
> > > +             }
> > > +             if (IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
> > > +                     err = _init_sdei_scs(&sdei_shadow_call_stack_normal_ptr, cpu);
> > > +                     if (err)
> > > +                             break;
> > > +                     err = _init_sdei_scs(&sdei_shadow_call_stack_critical_ptr, cpu);
> > > +                     if (err)
> > > +                             break;
> >
> > This looks ok to me, but I think it would be better to follow the same
> > approach as you have for the IRQ stacks and instead have a separate
> > init_sdei_scs() function (similarly for the free() path), which means
> > you can simply the IS_ENABLED() checks too.
> 
> OK, I can change this in v3. It makes error handling in
> sdei_arch_get_entry_point() a bit more awkward though. We'll need
> something like this:
> 
>         if (IS_ENABLED(CONFIG_VMAP_STACK)) {
>                 if (init_sdei_stacks())
>                         return 0;
>         }
> 
>         if (IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
>                 if (init_sdei_scs()) {
>                         if (IS_ENABLED(CONFIG_VMAP_STACK))
>                                 free_sdei_stacks();
>                         return 0;
>                 }

Can you push the IS_ENABLED() checks into their respective functions?
Then you can do something like:

	if (init_sdei_stacks())
		return 0;

	if (init_sdei_scs())
		goto out_free_stacks;

	...

out_free_stacks:
	free_sdei_stacks();
	return 0;

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel