[PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path

[PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path

[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 running __cpu_suspend_exit() with RCU watching.

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>
Suggested-by: "Paul E . McKenney" <paulmck@kernel.org>
Reported-by: Sami Tolvanen <samitolvanen@google.com>
Fixes: c28762070ca6 ("arm64: Rewrite Spectre-v4 mitigation code")
Signed-off-by: Will Deacon <will@kernel.org>
---

v2: Use RCU_NONIDLE() instead of eliding the spinlock

 arch/arm64/kernel/suspend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
index a67b37a7a47e..d7564891ffe1 100644
--- a/arch/arm64/kernel/suspend.c
+++ b/arch/arm64/kernel/suspend.c
@@ -119,7 +119,7 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
 		if (!ret)
 			ret = -EOPNOTSUPP;
 	} else {
-		__cpu_suspend_exit();
+		RCU_NONIDLE(__cpu_suspend_exit());
 	}
 
 	unpause_graph_tracing();
-- 
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

Re: [PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path

[Paul E. McKenney]

On Thu, Feb 18, 2021 at 02:03:46PM +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 running __cpu_suspend_exit() with RCU watching.
> 
> 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>
> Suggested-by: "Paul E . McKenney" <paulmck@kernel.org>
> Reported-by: Sami Tolvanen <samitolvanen@google.com>
> Fixes: c28762070ca6 ("arm64: Rewrite Spectre-v4 mitigation code")
> Signed-off-by: Will Deacon <will@kernel.org>

Looks good from an RCU perspective.

Acked-by: Paul E. McKenney <paulmck@kernel.org>

> ---
> 
> v2: Use RCU_NONIDLE() instead of eliding the spinlock
> 
>  arch/arm64/kernel/suspend.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
> index a67b37a7a47e..d7564891ffe1 100644
> --- a/arch/arm64/kernel/suspend.c
> +++ b/arch/arm64/kernel/suspend.c
> @@ -119,7 +119,7 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
>  		if (!ret)
>  			ret = -EOPNOTSUPP;
>  	} else {
> -		__cpu_suspend_exit();
> +		RCU_NONIDLE(__cpu_suspend_exit());
>  	}
>  
>  	unpause_graph_tracing();
> -- 
> 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

Re: [PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path

[Marc Zyngier]

On 2021-02-18 14:03, 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 running __cpu_suspend_exit() with RCU watching.
> 
> 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>
> Suggested-by: "Paul E . McKenney" <paulmck@kernel.org>
> Reported-by: Sami Tolvanen <samitolvanen@google.com>
> Fixes: c28762070ca6 ("arm64: Rewrite Spectre-v4 mitigation code")
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
> 
> v2: Use RCU_NONIDLE() instead of eliding the spinlock
> 
>  arch/arm64/kernel/suspend.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
> index a67b37a7a47e..d7564891ffe1 100644
> --- a/arch/arm64/kernel/suspend.c
> +++ b/arch/arm64/kernel/suspend.c
> @@ -119,7 +119,7 @@ int cpu_suspend(unsigned long arg, int 
> (*fn)(unsigned long))
>  		if (!ret)
>  			ret = -EOPNOTSUPP;
>  	} else {
> -		__cpu_suspend_exit();
> +		RCU_NONIDLE(__cpu_suspend_exit());
>  	}
> 
>  	unpause_graph_tracing();

Ah, looks so much better!

Acked-by: Marc Zyngier <maz@kernel.org>

         M.
-- 
Jazz is not dead. It just smells funny...

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

Re: [PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path

[Mark Rutland]

On Thu, Feb 18, 2021 at 02:03:46PM +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 running __cpu_suspend_exit() with RCU watching.
> 
> 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>
> Suggested-by: "Paul E . McKenney" <paulmck@kernel.org>
> Reported-by: Sami Tolvanen <samitolvanen@google.com>
> Fixes: c28762070ca6 ("arm64: Rewrite Spectre-v4 mitigation code")
> Signed-off-by: Will Deacon <will@kernel.org>

FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

I reckon we might want to restructure the SSBS emulation in future as a
cleaner fix; the oddity here is that we transiently register the hook,
where we could instead unconditionally register it at boot time and have
the hook itself decide whether it should apply a fixup. When entering an
exception from the idle task enter_from_kernel_mode() will handle the
RCU entry, so the hook would be able to use that without issue.

Thanks,
Mark.

> ---
> 
> v2: Use RCU_NONIDLE() instead of eliding the spinlock
> 
>  arch/arm64/kernel/suspend.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
> index a67b37a7a47e..d7564891ffe1 100644
> --- a/arch/arm64/kernel/suspend.c
> +++ b/arch/arm64/kernel/suspend.c
> @@ -119,7 +119,7 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
>  		if (!ret)
>  			ret = -EOPNOTSUPP;
>  	} else {
> -		__cpu_suspend_exit();
> +		RCU_NONIDLE(__cpu_suspend_exit());
>  	}
>  
>  	unpause_graph_tracing();
> -- 
> 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

Re: [PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path

[Will Deacon]

On Thu, 18 Feb 2021 14:03:46 +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.
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path
      https://git.kernel.org/arm64/c/a2c42bbabbe2

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

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