From: Will Deacon <will@kernel.org> Date: 2021-02-17 14:28:41
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 <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <redacted>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Saravana Kannan <redacted>
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(-)
From: Sami Tolvanen <samitolvanen@google.com> Date: 2021-02-17 17:05:24
Hi Will,
On Wed, Feb 17, 2021 at 6:26 AM Will Deacon [off-list ref] 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@arm.com>
Cc: Lorenzo Pieralisi <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <redacted>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Saravana Kannan <redacted>
Reported-by: Sami Tolvanen <samitolvanen@google.com>
Fixes: c28762070ca6 ("arm64: Rewrite Spectre-v4 mitigation code")
Signed-off-by: Will Deacon <will@kernel.org>
Thank you for the patch. I can confirm this fixes the warning for me.
Tested-by: Sami Tolvanen <samitolvanen@google.com>
Sami
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org> Date: 2021-02-17 18:03:13
On Wed, Feb 17, 2021 at 09:02:56AM -0800, Sami Tolvanen wrote:
On Wed, Feb 17, 2021 at 6:26 AM Will Deacon [off-list ref] wrote:
quoted
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 <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <redacted>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Saravana Kannan <redacted>
Reported-by: Sami Tolvanen <samitolvanen@google.com>
Fixes: c28762070ca6 ("arm64: Rewrite Spectre-v4 mitigation code")
Signed-off-by: Will Deacon <will@kernel.org>
Thank you for the patch. I can confirm this fixes the warning for me.
Tested-by: Sami Tolvanen <samitolvanen@google.com>
Thanks, Sami.
If nobody has any better ideas, I'll queue this as a fix for later in the
merge window.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Lorenzo Pieralisi <hidden> Date: 2021-02-17 18:47:37
On Wed, Feb 17, 2021 at 02:26:17PM +0000, Will Deacon wrote:
quoted hunk
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 <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <redacted>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Saravana Kannan <redacted>
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(-)
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
From: Will Deacon <will@kernel.org> Date: 2021-02-18 09:12:03
Hi Lorenzo,
On Wed, Feb 17, 2021 at 06:45:14PM +0000, Lorenzo Pieralisi wrote:
On Wed, Feb 17, 2021 at 02:26:17PM +0000, Will Deacon wrote:
quoted
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 <redacted>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Boqun Feng <redacted>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Saravana Kannan <redacted>
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(-)
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.
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.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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.
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.
Agreed - that's what I was alluding to but it is not prettier.
However, Paul McKenney suggested using RCU_NONIDLE() as an alternative
solution, so I'll spin a v2 with that.