arm64: spectre: Avoid locking on v4 mitigation enable path

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 <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Boqun Feng <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Saravana Kannan <[email protected]>

Reported-by Sami Tolvanen <[email protected]>

Fixes c28762070ca6 ("arm64: Rewrite Spectre-v4 mitigation code")

Signed-off-by Will Deacon <[email protected]>

Quoted 50 lines · 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.

Quoted 9 lines

Cc Mark Rutland <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Boqun Feng <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Saravana Kannan <[email protected]>

Reported-by Sami Tolvanen <[email protected]>

Fixes c28762070ca6 ("arm64: Rewrite Spectre-v4 mitigation code")

Signed-off-by Will Deacon <[email protected]>

Thank you for the patch. I can confirm this fixes the warning for me.

Tested-by Sami Tolvanen <[email protected]>

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 <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Boqun Feng <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Saravana Kannan <[email protected]>

Reported-by Sami Tolvanen <[email protected]>

Fixes c28762070ca6 ("arm64: Rewrite Spectre-v4 mitigation code")

Signed-off-by Will Deacon <[email protected]>

---

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(-)

arch/arm64/kernel/proton-pack.c +2 -1

index 902e4084c477..cb5b430b2dac 100644
@@ -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);
+

arch/arm64/kernel/proton-pack.c +8 -6

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

Signature · 1 line

2.30.0.478.g8a0d178c01-goog

Quoted 82 lines · 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 <[email protected]>

Cc Lorenzo Pieralisi <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Boqun Feng <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Saravana Kannan <[email protected]>

Reported-by Sami Tolvanen <[email protected]>

Fixes c28762070ca6 ("arm64: Rewrite Spectre-v4 mitigation code")

Signed-off-by Will Deacon <[email protected]>

---

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(-)

arch/arm64/kernel/proton-pack.c +2 -1

index 902e4084c477..cb5b430b2dac 100644
@@ -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);
+

arch/arm64/kernel/proton-pack.c static struct undef_hook ssbs_emulation_hook = { · line 515

Quoted 10 lines

arch/arm64/kernel/proton-pack.c +10 -0

+	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.

Quoted 20 lines

Quoted 6 lines

arch/arm64/kernel/proton-pack.c +2 -1

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

arch/arm64/kernel/proton-pack.c static struct undef_hook ssbs_emulation_hook = { · line 515

Quoted 10 lines

arch/arm64/kernel/proton-pack.c +10 -0

+	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 ?

Quoted 4 lines

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.