| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZvVO3F7BJW7OwbEg@boqun-archlinux>
Date: Thu, 26 Sep 2024 05:09:00 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: David Woodhouse <dwmw2@...radead.org>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
Waiman Long <longman@...hat.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
Juergen Gross <jgross@...e.com>
Subject: Re: [PATCH v3] lockdep: add lockdep_cleanup_dead_cpu()
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@...zon.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@...zon.co.uk>
> > Reviewed-by: Thomas Gleixner <tglx@...utronix.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)
> > {
>
Powered by blists - more mailing lists