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 left the tracing infrastructure
in a sane state. The acpi_idle_play_dead() function was recently observed¹
calling 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 lockdep warnings when the CPU came back online, but 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.
[ 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
¹ https://lore.kernel.org/lkml/a079bba5a0e47d6534b307553fc3772d26ce911b.camel@infradead.org/
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---
v3: Add forward declaration of struct task_struct.
v2: Fix spelling. 'Offlone' wasn't quite what I meant to type.
Add reference to ACPI patch.
Fix kerneldoc args for lockdep_cleanup_dead_cpu() (thanks lkp)
Closes: https://lore.kernel.org/oe-kbuild-all/202310290041.L5ndwcQ9-lkp@intel.com/
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 2b665c32f5fe..9b44f8b042a0 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -17,6 +17,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);
@@ -24,12 +26,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 6de7c6bb74ee..225f5bc3708f 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1371,6 +1371,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);
rcutree_migrate_callbacks(cpu);
return 0;
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index e85b5ad3e206..62bfda8991b8 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -4538,6 +4538,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.41.0
On Mon, 2023-10-30 at 08:45 +0000, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > Add a function to check that an offline CPU left the tracing infrastructure > in a sane state. The acpi_idle_play_dead() function was recently observed¹ > calling 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 lockdep warnings when the CPU came back online, but 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. > > [ 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 > > ¹ https://lore.kernel.org/lkml/a079bba5a0e47d6534b307553fc3772d26ce911b.camel@infradead.org/ > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > Reviewed-by: Thomas Gleixner <tglx@linutronix.de> > --- > Ping? Found this lying around in a branch today... > v3: Add forward declaration of struct task_struct. > > v2: Fix spelling. 'Offlone' wasn't quite what I meant to type. > Add reference to ACPI patch. > Fix kerneldoc args for lockdep_cleanup_dead_cpu() (thanks lkp) > Closes: https://lore.kernel.org/oe-kbuild-all/202310290041.L5ndwcQ9-lkp@intel.com/ > > 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 2b665c32f5fe..9b44f8b042a0 100644 > --- a/include/linux/irqflags.h > +++ b/include/linux/irqflags.h > @@ -17,6 +17,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); > @@ -24,12 +26,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 6de7c6bb74ee..225f5bc3708f 100644 > --- a/kernel/cpu.c > +++ b/kernel/cpu.c > @@ -1371,6 +1371,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); > rcutree_migrate_callbacks(cpu); > return 0; > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c > index e85b5ad3e206..62bfda8991b8 100644 > --- a/kernel/locking/lockdep.c > +++ b/kernel/locking/lockdep.c > @@ -4538,6 +4538,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) > {
On Tue, Sep 24, 2024 at 03:20:05PM +0100, David Woodhouse wrote: > On Mon, 2023-10-30 at 08:45 +0000, David Woodhouse wrote: > > From: David Woodhouse <dwmw@amazon.co.uk> > > > > Add a function to check that an offline CPU left the tracing infrastructure > > in a sane state. The acpi_idle_play_dead() function was recently observed¹ > > calling 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 lockdep warnings when the CPU came back online, but 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. > > > > [ 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 > > > > ¹ https://lore.kernel.org/lkml/a079bba5a0e47d6534b307553fc3772d26ce911b.camel@infradead.org/ > > > > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > > Reviewed-by: Thomas Gleixner <tglx@linutronix.de> > > --- > > > > Ping? Found this lying around in a branch today... > I think this is already fixed by: 9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in acpi_idle_play_dead()") , no? Regards, Boqun > > > v3: Add forward declaration of struct task_struct. > > > > v2: Fix spelling. 'Offlone' wasn't quite what I meant to type. > > Add reference to ACPI patch. > > Fix kerneldoc args for lockdep_cleanup_dead_cpu() (thanks lkp) > > Closes: https://lore.kernel.org/oe-kbuild-all/202310290041.L5ndwcQ9-lkp@intel.com/ > > > > 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 2b665c32f5fe..9b44f8b042a0 100644 > > --- a/include/linux/irqflags.h > > +++ b/include/linux/irqflags.h > > @@ -17,6 +17,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); > > @@ -24,12 +26,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 6de7c6bb74ee..225f5bc3708f 100644 > > --- a/kernel/cpu.c > > +++ b/kernel/cpu.c > > @@ -1371,6 +1371,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); > > rcutree_migrate_callbacks(cpu); > > return 0; > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c > > index e85b5ad3e206..62bfda8991b8 100644 > > --- a/kernel/locking/lockdep.c > > +++ b/kernel/locking/lockdep.c > > @@ -4538,6 +4538,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) > > { >
On Thu, 2024-09-26 at 05:09 -0700, Boqun Feng wrote: > > > I think this is already fixed by: > > 9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in acpi_idle_play_dead()") > > , no? That patch fixed the bug. *This* patch fixes the fact that lockdep didn't *tell* us about the bug.
On Thu, Sep 26, 2024 at 01:16:32PM +0100, David Woodhouse wrote: > On Thu, 2024-09-26 at 05:09 -0700, Boqun Feng wrote: > > > > > > I think this is already fixed by: > > > > 9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in acpi_idle_play_dead()") > > > > , no? > > That patch fixed the bug. > > *This* patch fixes the fact that lockdep didn't *tell* us about the bug. But I thought along with the above commit, Peter also made it possible that objtool can detect leaving noinstr section in the offline path? Do you have a case where you can alter hardirqs_enabled flag in offline path but don't hit the objtool warning? Anyway, the commit log needs a rework. Regards, Boqun
On Thu, 2024-09-26 at 05:34 -0700, Boqun Feng wrote: > On Thu, Sep 26, 2024 at 01:16:32PM +0100, David Woodhouse wrote: > > On Thu, 2024-09-26 at 05:09 -0700, Boqun Feng wrote: > > > > > > > > > I think this is already fixed by: > > > > > > 9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in acpi_idle_play_dead()") > > > > > > , no? > > > > That patch fixed the bug. > > > > *This* patch fixes the fact that lockdep didn't *tell* us about the bug. > > But I thought along with the above commit, Peter also made it possible > that objtool can detect leaving noinstr section in the offline path? Do > you have a case where you can alter hardirqs_enabled flag in offline > path but don't hit the objtool warning? I do not recall such. Peter? IIRC the bug fixed by commit 9bb69ba4c177 only showed up under real Xen, as QEMU doesn't expose processor C-states. So I reintroduced the equivalent bug by doing this instead: --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -1390,7 +1390,7 @@ void __noreturn hlt_play_dead(void) wbinvd(); while (1) - native_halt(); + safe_halt(); } Without this patch, I get a triple-fault on bringing the CPU back online as before. With it, as intended, I get a warning, but success: [root@localhost ~]# echo 0 > /sys/devices/system/cpu/cpu1/online [ 42.090839] smpboot: CPU 1 is now offline [ 42.091989] CPU 1 left hardirqs enabled! [ 42.091997] irq event stamp: 144559 [ 42.094155] hardirqs last enabled at (144559): [<ffffffff89098b2e>] hlt_play_dead+0x1e/0x30 [ 42.096196] hardirqs last disabled at (144558): [<ffffffff891800ee>] do_idle+0xae/0x260 [ 42.098062] softirqs last enabled at (144530): [<ffffffff89107260>] __irq_exit_rcu+0xb0/0xd0 [ 42.100056] softirqs last disabled at (144519): [<ffffffff89107260>] __irq_exit_rcu+0xb0/0xd0 [root@localhost ~]# echo 1 > /sys/devices/system/cpu/cpu1/online [ 47.480889] installing Xen timer for CPU 1 [ 47.485308] smpboot: Booting Node 0 Processor 1 APIC 0x1 [ 47.491569] cpu 1 spinlock event irq 35 So I think the patch is still applicable. > Anyway, the commit log needs a rework. Sure. Other than to refer to commit 9bb69ba4c177 instead of the mailing list message, is there anything else that needs changing? I suppose I should drop the word 'recently' from '...was recently observed'? :)
On Thu, Sep 26, 2024 at 03:34:57PM +0100, David Woodhouse wrote: > On Thu, 2024-09-26 at 05:34 -0700, Boqun Feng wrote: > > On Thu, Sep 26, 2024 at 01:16:32PM +0100, David Woodhouse wrote: > > > On Thu, 2024-09-26 at 05:09 -0700, Boqun Feng wrote: > > > > > > > > > > > > I think this is already fixed by: > > > > > > > > 9bb69ba4c177 ("ACPI: processor_idle: use raw_safe_halt() in acpi_idle_play_dead()") > > > > > > > > , no? > > > > > > That patch fixed the bug. > > > > > > *This* patch fixes the fact that lockdep didn't *tell* us about the bug. > > > > But I thought along with the above commit, Peter also made it possible > > that objtool can detect leaving noinstr section in the offline path? Do > > you have a case where you can alter hardirqs_enabled flag in offline > > path but don't hit the objtool warning? > > I do not recall such. Peter? > Oh I mis-read Peter's response here: https://lore.kernel.org/lkml/20231027191435.GF26550@noisy.programming.kicks-ass.net/ , so seems the noinstr annotating for CPU offline path is still a WIP. > IIRC the bug fixed by commit 9bb69ba4c177 only showed up under real > Xen, as QEMU doesn't expose processor C-states. So I reintroduced the > equivalent bug by doing this instead: > > --- a/arch/x86/kernel/smpboot.c > +++ b/arch/x86/kernel/smpboot.c > @@ -1390,7 +1390,7 @@ void __noreturn hlt_play_dead(void) > wbinvd(); > > while (1) > - native_halt(); > + safe_halt(); > } > > > Without this patch, I get a triple-fault on bringing the CPU back > online as before. With it, as intended, I get a warning, but success: > > [root@localhost ~]# echo 0 > /sys/devices/system/cpu/cpu1/online > [ 42.090839] smpboot: CPU 1 is now offline > [ 42.091989] CPU 1 left hardirqs enabled! > [ 42.091997] irq event stamp: 144559 > [ 42.094155] hardirqs last enabled at (144559): [<ffffffff89098b2e>] hlt_play_dead+0x1e/0x30 > [ 42.096196] hardirqs last disabled at (144558): [<ffffffff891800ee>] do_idle+0xae/0x260 > [ 42.098062] softirqs last enabled at (144530): [<ffffffff89107260>] __irq_exit_rcu+0xb0/0xd0 > [ 42.100056] softirqs last disabled at (144519): [<ffffffff89107260>] __irq_exit_rcu+0xb0/0xd0 > [root@localhost ~]# echo 1 > /sys/devices/system/cpu/cpu1/online > [ 47.480889] installing Xen timer for CPU 1 > [ 47.485308] smpboot: Booting Node 0 Processor 1 APIC 0x1 > [ 47.491569] cpu 1 spinlock event irq 35 > > > So I think the patch is still applicable. > Yeah, it was just that I thought we have static checking for the issue (via objtool), and I think that's slightly better, because it covers more problems. > > Anyway, the commit log needs a rework. > > Sure. Other than to refer to commit 9bb69ba4c177 instead of the mailing > list message, is there anything else that needs changing? I suppose I > should drop the word 'recently' from '...was recently observed'? :) > Given that Peter did send a POC for static checking: https://lore.kernel.org/lkml/20231030111724.GA12604@noisy.programming.kicks-ass.net/ Maybe you could explain why this is needed even though static checking is technically possible? Thanks! Regards, Boqun
On Thu, 2024-09-26 at 08:13 -0700, Boqun Feng wrote: > > Given that Peter did send a POC for static checking: > > https://lore.kernel.org/lkml/20231030111724.GA12604@noisy.programming.kicks-ass.net/ > > Maybe you could explain why this is needed even though static checking > is technically possible? Thanks! If Peter wants to finish that approach and get it merged, that's fine by me.
© 2016 - 2024 Red Hat, Inc.