* Re: [PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path
2021-02-18 14:03 [PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path Will Deacon
@ 2021-02-18 15:08 ` Paul E. McKenney
2021-02-18 15:37 ` Marc Zyngier
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Paul E. McKenney @ 2021-02-18 15:08 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Rutland, Lorenzo Pieralisi, Saravana Kannan, Peter Zijlstra,
Marc Zyngier, Boqun Feng, Sami Tolvanen, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path
2021-02-18 14:03 [PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path Will Deacon
2021-02-18 15:08 ` Paul E. McKenney
@ 2021-02-18 15:37 ` Marc Zyngier
2021-02-18 15:44 ` Mark Rutland
2021-02-19 19:16 ` Will Deacon
3 siblings, 0 replies; 5+ messages in thread
From: Marc Zyngier @ 2021-02-18 15:37 UTC (permalink / raw)
To: Will Deacon
Cc: Mark Rutland, Lorenzo Pieralisi, Paul E . McKenney,
Saravana Kannan, Peter Zijlstra, Boqun Feng, Sami Tolvanen,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path
2021-02-18 14:03 [PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path Will Deacon
2021-02-18 15:08 ` Paul E. McKenney
2021-02-18 15:37 ` Marc Zyngier
@ 2021-02-18 15:44 ` Mark Rutland
2021-02-19 19:16 ` Will Deacon
3 siblings, 0 replies; 5+ messages in thread
From: Mark Rutland @ 2021-02-18 15:44 UTC (permalink / raw)
To: Will Deacon
Cc: Lorenzo Pieralisi, Paul E . McKenney, Saravana Kannan,
Peter Zijlstra, Marc Zyngier, Boqun Feng, Sami Tolvanen,
linux-arm-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path
2021-02-18 14:03 [PATCH v2] arm64: spectre: Prevent lockdep splat on v4 mitigation enable path Will Deacon
` (2 preceding siblings ...)
2021-02-18 15:44 ` Mark Rutland
@ 2021-02-19 19:16 ` Will Deacon
3 siblings, 0 replies; 5+ messages in thread
From: Will Deacon @ 2021-02-19 19:16 UTC (permalink / raw)
To: Will Deacon, linux-arm-kernel
Cc: Mark Rutland, Lorenzo Pieralisi, Saravana Kannan,
Paul E . McKenney, Peter Zijlstra, catalin.marinas, Boqun Feng,
Sami Tolvanen, Marc Zyngier, kernel-team
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
^ permalink raw reply [flat|nested] 5+ messages in thread