include/linux/irqflags.h | 6 ++++++ kernel/cpu.c | 1 + kernel/locking/lockdep.c | 24 ++++++++++++++++++++++++ 3 files changed, 31 insertions(+)
From: David Woodhouse <dwmw@amazon.co.uk>
Add a function to check that an offline CPU has left the tracing
infrastructure in a sane state.
Commit 9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in
acpi_idle_play_dead()") fixed an issue where the acpi_idle_play_dead()
function called safe_halt() instead of raw_safe_halt(), which had the
side-effect of setting the hardirqs_enabled flag for the offline CPU.
On x86 this triggered warnings from lockdep_assert_irqs_disabled() when
the CPU was brought back online again later. These warnings were too
early for the exception to be handled correctly.
So lockdep turned a perfectly harmless bug into a system crash with a
triple-fault.
Add lockdep_cleanup_dead_cpu() to check for this kind of failure mode,
print the events leading up to it, and correct it so that the CPU can
come online again correctly. Re-introducing the original bug now merely
results in this warning instead:
[ 61.556652] smpboot: CPU 1 is now offline
[ 61.556769] CPU 1 left hardirqs enabled!
[ 61.556915] irq event stamp: 128149
[ 61.556965] hardirqs last enabled at (128149): [<ffffffff81720a36>] acpi_idle_play_dead+0x46/0x70
[ 61.557055] hardirqs last disabled at (128148): [<ffffffff81124d50>] do_idle+0x90/0xe0
[ 61.557117] softirqs last enabled at (128078): [<ffffffff81cec74c>] __do_softirq+0x31c/0x423
[ 61.557199] softirqs last disabled at (128065): [<ffffffff810baae1>] __irq_exit_rcu+0x91/0x100
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---
v2: Update commit message to reflect the fact that the original bug
fix is now merged as commit 9bb69ba4c177.
include/linux/irqflags.h | 6 ++++++
kernel/cpu.c | 1 +
kernel/locking/lockdep.c | 24 ++++++++++++++++++++++++
3 files changed, 31 insertions(+)
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 3f003d5fde53..57b074e0cfbb 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -18,6 +18,8 @@
#include <asm/irqflags.h>
#include <asm/percpu.h>
+struct task_struct;
+
/* Currently lockdep_softirqs_on/off is used only by lockdep */
#ifdef CONFIG_PROVE_LOCKING
extern void lockdep_softirqs_on(unsigned long ip);
@@ -25,12 +27,16 @@
extern void lockdep_hardirqs_on_prepare(void);
extern void lockdep_hardirqs_on(unsigned long ip);
extern void lockdep_hardirqs_off(unsigned long ip);
+ extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
+ struct task_struct *idle);
#else
static inline void lockdep_softirqs_on(unsigned long ip) { }
static inline void lockdep_softirqs_off(unsigned long ip) { }
static inline void lockdep_hardirqs_on_prepare(void) { }
static inline void lockdep_hardirqs_on(unsigned long ip) { }
static inline void lockdep_hardirqs_off(unsigned long ip) { }
+ static inline void lockdep_cleanup_dead_cpu(unsigned int cpu,
+ struct task_struct *idle) {}
#endif
#ifdef CONFIG_TRACE_IRQFLAGS
diff --git a/kernel/cpu.c b/kernel/cpu.c
index d293d52a3e00..c4aaf73dec9e 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1338,6 +1338,7 @@ static int takedown_cpu(unsigned int cpu)
cpuhp_bp_sync_dead(cpu);
+ lockdep_cleanup_dead_cpu(cpu, idle_thread_get(cpu));
tick_cleanup_dead_cpu(cpu);
/*
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 7963deac33c3..42b07c3b8862 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4583,6 +4583,30 @@ void lockdep_softirqs_off(unsigned long ip)
debug_atomic_inc(redundant_softirqs_off);
}
+/**
+ * lockdep_cleanup_dead_cpu - Ensure CPU lockdep state is cleanly stopped
+ *
+ * @cpu: index of offlined CPU
+ * @idle: task pointer for offlined CPU's idle thread
+ *
+ * Invoked after the CPU is dead. Ensures that the tracing infrastructure
+ * is left in a suitable state for the CPU to be subsequently brought
+ * online again.
+ */
+void lockdep_cleanup_dead_cpu(unsigned int cpu, struct task_struct *idle)
+{
+ if (unlikely(!debug_locks))
+ return;
+
+ if (unlikely(per_cpu(hardirqs_enabled, cpu))) {
+ pr_warn("CPU %u left hardirqs enabled!", cpu);
+ if (idle)
+ print_irqtrace_events(idle);
+ /* Clean it up for when the CPU comes online again. */
+ per_cpu(hardirqs_enabled, cpu) = 0;
+ }
+}
+
static int
mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
{
--
2.44.0
On Thu, Sep 26, 2024 at 04:17:37PM +0100, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> Add a function to check that an offline CPU has left the tracing
> infrastructure in a sane state.
>
> Commit 9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in
> acpi_idle_play_dead()") fixed an issue where the acpi_idle_play_dead()
> function called safe_halt() instead of raw_safe_halt(), which had the
> side-effect of setting the hardirqs_enabled flag for the offline CPU.
>
> On x86 this triggered warnings from lockdep_assert_irqs_disabled() when
> the CPU was brought back online again later. These warnings were too
> early for the exception to be handled correctly.
>
> So lockdep turned a perfectly harmless bug into a system crash with a
> triple-fault.
>
I won't call this a "perfectly harmless bug", safe_halt() also contains
tracepoints, which are not supposed to work in offline path IIUC, for
example, you may incorrectly use RCU when RCU is not watching, that
could mean reading garbage memory (surely it won't crash the system, but
I hope I never need to debug such a system ;-)).
Otherwise this patch looks good to me. Thanks!
Regards,
Boqun
> Add lockdep_cleanup_dead_cpu() to check for this kind of failure mode,
> print the events leading up to it, and correct it so that the CPU can
> come online again correctly. Re-introducing the original bug now merely
> results in this warning instead:
>
> [ 61.556652] smpboot: CPU 1 is now offline
> [ 61.556769] CPU 1 left hardirqs enabled!
> [ 61.556915] irq event stamp: 128149
> [ 61.556965] hardirqs last enabled at (128149): [<ffffffff81720a36>] acpi_idle_play_dead+0x46/0x70
> [ 61.557055] hardirqs last disabled at (128148): [<ffffffff81124d50>] do_idle+0x90/0xe0
> [ 61.557117] softirqs last enabled at (128078): [<ffffffff81cec74c>] __do_softirq+0x31c/0x423
> [ 61.557199] softirqs last disabled at (128065): [<ffffffff810baae1>] __irq_exit_rcu+0x91/0x100
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> v2: Update commit message to reflect the fact that the original bug
> fix is now merged as commit 9bb69ba4c177.
>
>
> include/linux/irqflags.h | 6 ++++++
> kernel/cpu.c | 1 +
> kernel/locking/lockdep.c | 24 ++++++++++++++++++++++++
> 3 files changed, 31 insertions(+)
>
> diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
> index 3f003d5fde53..57b074e0cfbb 100644
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> @@ -18,6 +18,8 @@
> #include <asm/irqflags.h>
> #include <asm/percpu.h>
>
> +struct task_struct;
> +
> /* Currently lockdep_softirqs_on/off is used only by lockdep */
> #ifdef CONFIG_PROVE_LOCKING
> extern void lockdep_softirqs_on(unsigned long ip);
> @@ -25,12 +27,16 @@
> extern void lockdep_hardirqs_on_prepare(void);
> extern void lockdep_hardirqs_on(unsigned long ip);
> extern void lockdep_hardirqs_off(unsigned long ip);
> + extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
> + struct task_struct *idle);
> #else
> static inline void lockdep_softirqs_on(unsigned long ip) { }
> static inline void lockdep_softirqs_off(unsigned long ip) { }
> static inline void lockdep_hardirqs_on_prepare(void) { }
> static inline void lockdep_hardirqs_on(unsigned long ip) { }
> static inline void lockdep_hardirqs_off(unsigned long ip) { }
> + static inline void lockdep_cleanup_dead_cpu(unsigned int cpu,
> + struct task_struct *idle) {}
> #endif
>
> #ifdef CONFIG_TRACE_IRQFLAGS
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index d293d52a3e00..c4aaf73dec9e 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -1338,6 +1338,7 @@ static int takedown_cpu(unsigned int cpu)
>
> cpuhp_bp_sync_dead(cpu);
>
> + lockdep_cleanup_dead_cpu(cpu, idle_thread_get(cpu));
> tick_cleanup_dead_cpu(cpu);
>
> /*
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 7963deac33c3..42b07c3b8862 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -4583,6 +4583,30 @@ void lockdep_softirqs_off(unsigned long ip)
> debug_atomic_inc(redundant_softirqs_off);
> }
>
> +/**
> + * lockdep_cleanup_dead_cpu - Ensure CPU lockdep state is cleanly stopped
> + *
> + * @cpu: index of offlined CPU
> + * @idle: task pointer for offlined CPU's idle thread
> + *
> + * Invoked after the CPU is dead. Ensures that the tracing infrastructure
> + * is left in a suitable state for the CPU to be subsequently brought
> + * online again.
> + */
> +void lockdep_cleanup_dead_cpu(unsigned int cpu, struct task_struct *idle)
> +{
> + if (unlikely(!debug_locks))
> + return;
> +
> + if (unlikely(per_cpu(hardirqs_enabled, cpu))) {
> + pr_warn("CPU %u left hardirqs enabled!", cpu);
> + if (idle)
> + print_irqtrace_events(idle);
> + /* Clean it up for when the CPU comes online again. */
> + per_cpu(hardirqs_enabled, cpu) = 0;
> + }
> +}
> +
> static int
> mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
> {
> --
> 2.44.0
>
>
On Thu, 2024-09-26 at 09:09 -0700, Boqun Feng wrote: > > I won't call this a "perfectly harmless bug", safe_halt() also contains > tracepoints, which are not supposed to work in offline path IIUC, for > example, you may incorrectly use RCU when RCU is not watching, that > could mean reading garbage memory (surely it won't crash the system, but > I hope I never need to debug such a system ;-)). > > Otherwise this patch looks good to me. Thanks! Apart from the fact that I can't count. Apparently I got up to v3 of it last time, so this one should have been v4. I just mostly forgot all about it, and found it lying around in a git tree a year later, and it still seemed relevant.
On Thu, Sep 26, 2024 at 05:37:12PM +0100, David Woodhouse wrote: > On Thu, 2024-09-26 at 09:09 -0700, Boqun Feng wrote: > > > > I won't call this a "perfectly harmless bug", safe_halt() also contains > > tracepoints, which are not supposed to work in offline path IIUC, for > > example, you may incorrectly use RCU when RCU is not watching, that > > could mean reading garbage memory (surely it won't crash the system, but > > I hope I never need to debug such a system ;-)). > > > > Otherwise this patch looks good to me. Thanks! > > Apart from the fact that I can't count. Apparently I got up to v3 of it > last time, so this one should have been v4. I just mostly forgot all > about it, and found it lying around in a git tree a year later, and it > still seemed relevant. My point is calling a non-noinstr function in the offline path is not a "perfectly harmless" bug, it can cause serious results, so that line in the commit log is not true. Of course, lockdep should handle buggy code gracefully, but buggy code is still buggy code. Anyway, I've taken it into my tree (I removed the "perfectly harmless bug" part because of the reason above): git://git.kernel.org/pub/scm/linux/kernel/git/boqun/linux.git lockdep-for-tip and will send it in a PR to tip around -rc2 to -rc4, so it will goes into v6.13 if things went well. Feel free to send a new version, if the one in my tree needs some changes. Again, thanks for the patch! Regards, Boqun (you can refer some context here [1], in case you wonder who's this Boqun guy and why is he doing this ;-)) [1]: https://lore.kernel.org/lkml/Zq5KmTEnalIOHf6a@boqun-archlinux/
On Thu, 2024-09-26 at 17:45 -0700, Boqun Feng wrote: > On Thu, Sep 26, 2024 at 05:37:12PM +0100, David Woodhouse wrote: > > On Thu, 2024-09-26 at 09:09 -0700, Boqun Feng wrote: > > > > > > I won't call this a "perfectly harmless bug", safe_halt() also contains > > > tracepoints, which are not supposed to work in offline path IIUC, for > > > example, you may incorrectly use RCU when RCU is not watching, that > > > could mean reading garbage memory (surely it won't crash the system, but > > > I hope I never need to debug such a system ;-)). > > > > > > Otherwise this patch looks good to me. Thanks! > > > > Apart from the fact that I can't count. Apparently I got up to v3 of it > > last time, so this one should have been v4. I just mostly forgot all > > about it, and found it lying around in a git tree a year later, and it > > still seemed relevant. > > My point is calling a non-noinstr function in the offline path is not a > "perfectly harmless" bug, it can cause serious results, so that line in > the commit log is not true. Of course, lockdep should handle buggy code > gracefully, but buggy code is still buggy code. As you wish. It was very much *less* harmful than a triple-fault, and I would at least have left something in the commit comment about lockdep making the user-visible symptoms much worse (and hard to debug; that's a few days of my life I want back!) — but your point is valid, and it's not a hill to die on. > Anyway, I've taken it into my tree (I removed the "perfectly harmless > bug" part because of the reason above): > > git://git.kernel.org/pub/scm/linux/kernel/git/boqun/linux.git lockdep-for-tip > > and will send it in a PR to tip around -rc2 to -rc4, so it will goes > into v6.13 if things went well. > > Feel free to send a new version, if the one in my tree needs some > changes. Again, thanks for the patch! I think it'll do. Thanks for picking it up.
The following commit has been merged into the locking/core branch of tip:
Commit-ID: 0784181b44af831a3fa52e1e5ff77c388d699dba
Gitweb: https://git.kernel.org/tip/0784181b44af831a3fa52e1e5ff77c388d699dba
Author: David Woodhouse <dwmw@amazon.co.uk>
AuthorDate: Thu, 26 Sep 2024 16:17:37 +01:00
Committer: Boqun Feng <boqun.feng@gmail.com>
CommitterDate: Thu, 17 Oct 2024 20:07:22 -07:00
lockdep: Add lockdep_cleanup_dead_cpu()
Add a function to check that an offline CPU has left the tracing
infrastructure in a sane state.
Commit 9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in
acpi_idle_play_dead()") fixed an issue where the acpi_idle_play_dead()
function called safe_halt() instead of raw_safe_halt(), which had the
side-effect of setting the hardirqs_enabled flag for the offline CPU.
On x86 this triggered warnings from lockdep_assert_irqs_disabled() when
the CPU was brought back online again later. These warnings were too
early for the exception to be handled correctly, leading to a
triple-fault.
Add lockdep_cleanup_dead_cpu() to check for this kind of failure mode,
print the events leading up to it, and correct it so that the CPU can
come online again correctly. Re-introducing the original bug now merely
results in this warning instead:
[ 61.556652] smpboot: CPU 1 is now offline
[ 61.556769] CPU 1 left hardirqs enabled!
[ 61.556915] irq event stamp: 128149
[ 61.556965] hardirqs last enabled at (128149): [<ffffffff81720a36>] acpi_idle_play_dead+0x46/0x70
[ 61.557055] hardirqs last disabled at (128148): [<ffffffff81124d50>] do_idle+0x90/0xe0
[ 61.557117] softirqs last enabled at (128078): [<ffffffff81cec74c>] __do_softirq+0x31c/0x423
[ 61.557199] softirqs last disabled at (128065): [<ffffffff810baae1>] __irq_exit_rcu+0x91/0x100
[boqun: Capitalize the title and reword the message a bit]
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Link: https://lore.kernel.org/r/f7bd2b3b999051bb3ef4be34526a9262008285f5.camel@infradead.org
---
include/linux/irqflags.h | 6 ++++++
kernel/cpu.c | 1 +
kernel/locking/lockdep.c | 24 ++++++++++++++++++++++++
3 files changed, 31 insertions(+)
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 3f003d5..57b074e 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -18,6 +18,8 @@
#include <asm/irqflags.h>
#include <asm/percpu.h>
+struct task_struct;
+
/* Currently lockdep_softirqs_on/off is used only by lockdep */
#ifdef CONFIG_PROVE_LOCKING
extern void lockdep_softirqs_on(unsigned long ip);
@@ -25,12 +27,16 @@
extern void lockdep_hardirqs_on_prepare(void);
extern void lockdep_hardirqs_on(unsigned long ip);
extern void lockdep_hardirqs_off(unsigned long ip);
+ extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
+ struct task_struct *idle);
#else
static inline void lockdep_softirqs_on(unsigned long ip) { }
static inline void lockdep_softirqs_off(unsigned long ip) { }
static inline void lockdep_hardirqs_on_prepare(void) { }
static inline void lockdep_hardirqs_on(unsigned long ip) { }
static inline void lockdep_hardirqs_off(unsigned long ip) { }
+ static inline void lockdep_cleanup_dead_cpu(unsigned int cpu,
+ struct task_struct *idle) {}
#endif
#ifdef CONFIG_TRACE_IRQFLAGS
diff --git a/kernel/cpu.c b/kernel/cpu.c
index d293d52..c4aaf73 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1338,6 +1338,7 @@ static int takedown_cpu(unsigned int cpu)
cpuhp_bp_sync_dead(cpu);
+ lockdep_cleanup_dead_cpu(cpu, idle_thread_get(cpu));
tick_cleanup_dead_cpu(cpu);
/*
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 536bd47..6fd4af2 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4586,6 +4586,30 @@ void lockdep_softirqs_off(unsigned long ip)
debug_atomic_inc(redundant_softirqs_off);
}
+/**
+ * lockdep_cleanup_dead_cpu - Ensure CPU lockdep state is cleanly stopped
+ *
+ * @cpu: index of offlined CPU
+ * @idle: task pointer for offlined CPU's idle thread
+ *
+ * Invoked after the CPU is dead. Ensures that the tracing infrastructure
+ * is left in a suitable state for the CPU to be subsequently brought
+ * online again.
+ */
+void lockdep_cleanup_dead_cpu(unsigned int cpu, struct task_struct *idle)
+{
+ if (unlikely(!debug_locks))
+ return;
+
+ if (unlikely(per_cpu(hardirqs_enabled, cpu))) {
+ pr_warn("CPU %u left hardirqs enabled!", cpu);
+ if (idle)
+ print_irqtrace_events(idle);
+ /* Clean it up for when the CPU comes online again. */
+ per_cpu(hardirqs_enabled, cpu) = 0;
+ }
+}
+
static int
mark_usage(struct task_struct *curr, struct held_lock *hlock, int check)
{© 2016 - 2024 Red Hat, Inc.