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

* [PATCH] arm64: spectre: Avoid locking on v4 mitigation enable path
@ 2021-02-17 14:26 Will Deacon
  2021-02-17 17:02 ` Sami Tolvanen
  2021-02-17 18:45 ` Lorenzo Pieralisi
  0 siblings, 2 replies; 6+ messages in thread
From: Will Deacon @ 2021-02-17 14:26 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Mark Rutland, Lorenzo Pieralisi, Saravana Kannan, Peter Zijlstra,
	Marc Zyngier, Boqun Feng, Sami Tolvanen, Will Deacon

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@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Saravana Kannan <saravanak@google.com>
Reported-by: Sami Tolvanen <samitolvanen@google.com>
Fixes: c28762070ca6 ("arm64: Rewrite Spectre-v4 mitigation code")
Signed-off-by: Will Deacon <will@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);
+}
+
+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


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread