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

[Lorenzo Pieralisi]

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);
> +}

Hi Will,

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.

Thanks,
Lorenzo

> +
> +static enum mitigation_state spectre_v4_enable_hw_mitigation(void)
> +{
>  	enum mitigation_state state;
>  
>  	/*
> @@ -512,12 +526,11 @@ static enum mitigation_state spectre_v4_enable_hw_mitigation(void)
>  	if (state != SPECTRE_MITIGATED || !this_cpu_has_cap(ARM64_SSBS))
>  		return state;
>  
> -	raw_spin_lock(&hook_lock);
> -	if (!undef_hook_registered) {
> -		register_undef_hook(&ssbs_emulation_hook);
> -		undef_hook_registered = true;
> -	}
> -	raw_spin_unlock(&hook_lock);
> +	/*
> +	 * Toggling PSTATE.SSBS directly may trigger an undefined instruction
> +	 * exception, so ensure we're ready to deal with the emulation.
> +	 */
> +	spectre_v4_register_undef_hook();
>  
>  	if (spectre_v4_mitigations_off()) {
>  		sysreg_clear_set(sctlr_el1, 0, SCTLR_ELx_DSSBS);
> -- 
> 2.30.0.478.g8a0d178c01-goog
>