[PATCH] arm64: spectre: Avoid locking on v4 mitigation enable path

[Will Deacon]

Hi Lorenzo,

On Wed, Feb 17, 2021 at 06:45:14PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Feb 17, 2021 at 02:26:17PM +0000, Will Deacon wrote:
> > The Spectre-v4 workaround is re-configured when resuming from suspend,
> > as the firmware may have re-enabled the mitigation despite the user
> > previously asking for it to be disabled.
> > 
> > Enabling or disabling the workaround can result in an undefined
> > instruction exception on CPUs which implement PSTATE.SSBS but only allow
> > it to be configured by adjusting the SPSR on exception return. We handle
> > this by installing an 'undef hook' which effectively emulates the access.
> > 
> > Installing this hook requires us to take a couple of spinlocks both to
> > avoid corrupting the internal list of hooks but also to ensure that we
> > don't run into an unhandled exception. Unfortunately, when resuming from
> > suspend, we haven't yet called rcu_idle_exit() and so lockdep gets angry
> > about "suspicious RCU usage". In doing so, it tries to print a warning,
> > which leads it to get even more suspicious, this time about itself:
> > 
> >  |  rcu_scheduler_active = 2, debug_locks = 1
> >  |  RCU used illegally from extended quiescent state!
> >  |  1 lock held by swapper/0:
> >  |   #0: (logbuf_lock){-.-.}-{2:2}, at: vprintk_emit+0x88/0x198
> >  |
> >  |  Call trace:
> >  |   dump_backtrace+0x0/0x1d8
> >  |   show_stack+0x18/0x24
> >  |   dump_stack+0xe0/0x17c
> >  |   lockdep_rcu_suspicious+0x11c/0x134
> >  |   trace_lock_release+0xa0/0x160
> >  |   lock_release+0x3c/0x290
> >  |   _raw_spin_unlock+0x44/0x80
> >  |   vprintk_emit+0xbc/0x198
> >  |   vprintk_default+0x44/0x6c
> >  |   vprintk_func+0x1f4/0x1fc
> >  |   printk+0x54/0x7c
> >  |   lockdep_rcu_suspicious+0x30/0x134
> >  |   trace_lock_acquire+0xa0/0x188
> >  |   lock_acquire+0x50/0x2fc
> >  |   _raw_spin_lock+0x68/0x80
> >  |   spectre_v4_enable_mitigation+0xa8/0x30c
> >  |   __cpu_suspend_exit+0xd4/0x1a8
> >  |   cpu_suspend+0xa0/0x104
> >  |   psci_cpu_suspend_enter+0x3c/0x5c
> >  |   psci_enter_idle_state+0x44/0x74
> >  |   cpuidle_enter_state+0x148/0x2f8
> >  |   cpuidle_enter+0x38/0x50
> >  |   do_idle+0x1f0/0x2b4
> > 
> > Prevent these splats by avoiding locking entirely if we know that a hook
> > has already been registered for the mitigation.
> > 
> > Cc: Mark Rutland <mark.rutland at arm.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi at arm.com>
> > Cc: Peter Zijlstra <peterz at infradead.org>
> > Cc: Boqun Feng <boqun.feng at gmail.com>
> > Cc: Marc Zyngier <maz at kernel.org>
> > Cc: Saravana Kannan <saravanak at google.com>
> > Reported-by: Sami Tolvanen <samitolvanen at google.com>
> > Fixes: c28762070ca6 ("arm64: Rewrite Spectre-v4 mitigation code")
> > Signed-off-by: Will Deacon <will at kernel.org>
> > ---
> > 
> > I'm not hugely pleased with this patch, as it adds significant complexity
> > to something which is a slow-path and should be straightforward. I also
> > notice that __cpu_suspend_exit() is marked 'notrace' but it makes plenty
> > of calls to functions which are not marked inline or notrace.
> > 
> >  arch/arm64/kernel/proton-pack.c | 27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/proton-pack.c b/arch/arm64/kernel/proton-pack.c
> > index 902e4084c477..cb5b430b2dac 100644
> > --- a/arch/arm64/kernel/proton-pack.c
> > +++ b/arch/arm64/kernel/proton-pack.c
> > @@ -498,10 +498,24 @@ static struct undef_hook ssbs_emulation_hook = {
> >  	.fn		= ssbs_emulation_handler,
> >  };
> >  
> > -static enum mitigation_state spectre_v4_enable_hw_mitigation(void)
> > +static void spectre_v4_register_undef_hook(void)
> >  {
> >  	static bool undef_hook_registered = false;
> >  	static DEFINE_RAW_SPINLOCK(hook_lock);
> > +
> > +	if (READ_ONCE(undef_hook_registered))
> > +		return;
> > +
> > +	raw_spin_lock(&hook_lock);
> > +	if (!undef_hook_registered) {
> > +		register_undef_hook(&ssbs_emulation_hook);
> > +		smp_store_release(&undef_hook_registered, true);
> > +	}
> > +	raw_spin_unlock(&hook_lock);
> > +}
> 
> I have a question: is there ever a use case when we have to call this
> function from suspend exit where undef_hook_registered can be == false ?
> 
> I don't get why a resuming CPU would have to register a hook if on
> suspend the hook was not registered already but I think that's because I
> don't know well enough how spectre v4 mitigations are handled, so I am
> asking.

No, on the resume path we don't need to register the hook and with this
patch we avoid it entirely. However, having a completely different
mitigation path for resume is also quite horrible, especially given how
complicated the control flow is for the mitigation logic already. At least,
I couldn't figure out how to structure it without passing in an extra
parameter to say we were resuming.

However, Paul McKenney suggested using RCU_NONIDLE() as an alternative
solution, so I'll spin a v2 with that.

Will