include/linux/irqflags.h | 4 ++++ kernel/cpu.c | 1 + kernel/locking/lockdep.c | 24 ++++++++++++++++++++++++ 3 files changed, 29 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>
---
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 | 4 ++++
kernel/cpu.c | 1 +
kernel/locking/lockdep.c | 24 ++++++++++++++++++++++++
3 files changed, 29 insertions(+)
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 2b665c32f5fe..547ed55fc7ff 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -24,12 +24,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
Hi David,
kernel test robot noticed the following build errors:
[auto build test ERROR on tip/smp/core]
[also build test ERROR on tip/locking/core linus/master v6.6 next-20231030]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/David-Woodhouse/lockdep-add-lockdep_cleanup_dead_cpu/20231029-032722
base: tip/smp/core
patch link: https://lore.kernel.org/r/635fa006e8f3816b4a36b964d6281f0d8efa789b.camel%40infradead.org
patch subject: [PATCH v2] lockdep: add lockdep_cleanup_dead_cpu()
config: sparc64-defconfig (https://download.01.org/0day-ci/archive/20231031/202310310038.MkdWejfv-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231031/202310310038.MkdWejfv-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310310038.MkdWejfv-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/sparc/include/asm/cmpxchg_64.h:111,
from arch/sparc/include/asm/cmpxchg.h:5,
from arch/sparc/include/asm/atomic_64.h:12,
from arch/sparc/include/asm/atomic.h:5,
from include/linux/atomic.h:7,
from include/asm-generic/bitops/lock.h:5,
from arch/sparc/include/asm/bitops_64.h:52,
from arch/sparc/include/asm/bitops.h:5,
from include/linux/bitops.h:68,
from include/linux/kernel.h:22,
from arch/sparc/kernel/unaligned_64.c:12:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
36 | struct task_struct *idle) {}
| ^~~~~~~~~~~
cc1: all warnings being treated as errors
--
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/sparc/include/asm/cmpxchg_64.h:111,
from arch/sparc/include/asm/cmpxchg.h:5,
from arch/sparc/include/asm/atomic_64.h:12,
from arch/sparc/include/asm/atomic.h:5,
from include/linux/atomic.h:7,
from include/asm-generic/bitops/lock.h:5,
from arch/sparc/include/asm/bitops_64.h:52,
from arch/sparc/include/asm/bitops.h:5,
from include/linux/bitops.h:68,
from include/linux/thread_info.h:27,
from include/asm-generic/preempt.h:5,
from ./arch/sparc/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:79,
from include/linux/spinlock.h:56,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/slab.h:16,
from arch/sparc/kernel/adi_64.c:10:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
36 | struct task_struct *idle) {}
| ^~~~~~~~~~~
arch/sparc/kernel/adi_64.c:124:21: error: no previous prototype for 'find_tag_store' [-Werror=missing-prototypes]
124 | tag_storage_desc_t *find_tag_store(struct mm_struct *mm,
| ^~~~~~~~~~~~~~
arch/sparc/kernel/adi_64.c:156:21: error: no previous prototype for 'alloc_tag_store' [-Werror=missing-prototypes]
156 | tag_storage_desc_t *alloc_tag_store(struct mm_struct *mm,
| ^~~~~~~~~~~~~~~
arch/sparc/kernel/adi_64.c:299:6: error: no previous prototype for 'del_tag_store' [-Werror=missing-prototypes]
299 | void del_tag_store(tag_storage_desc_t *tag_desc, struct mm_struct *mm)
| ^~~~~~~~~~~~~
cc1: all warnings being treated as errors
--
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/sparc/include/asm/cmpxchg_64.h:111,
from arch/sparc/include/asm/cmpxchg.h:5,
from arch/sparc/include/asm/atomic_64.h:12,
from arch/sparc/include/asm/atomic.h:5,
from include/linux/atomic.h:7,
from include/asm-generic/bitops/lock.h:5,
from arch/sparc/include/asm/bitops_64.h:52,
from arch/sparc/include/asm/bitops.h:5,
from include/linux/bitops.h:68,
from include/linux/kernel.h:22,
from arch/sparc/kernel/pcr.c:6:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
36 | struct task_struct *idle) {}
| ^~~~~~~~~~~
arch/sparc/kernel/pcr.c:47:6: error: no previous prototype for 'arch_irq_work_raise' [-Werror=missing-prototypes]
47 | void arch_irq_work_raise(void)
| ^~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
--
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/sparc/include/asm/cmpxchg_64.h:111,
from arch/sparc/include/asm/cmpxchg.h:5,
from arch/sparc/include/asm/atomic_64.h:12,
from arch/sparc/include/asm/atomic.h:5,
from include/linux/atomic.h:7,
from include/linux/mm_types_task.h:13,
from include/linux/mm_types.h:5,
from include/linux/buildid.h:5,
from include/linux/module.h:14,
from include/linux/moduleloader.h:6,
from arch/sparc/kernel/module.c:8:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
36 | struct task_struct *idle) {}
| ^~~~~~~~~~~
arch/sparc/kernel/module.c: In function 'module_frob_arch_sections':
arch/sparc/kernel/module.c:62:15: error: variable 'strtab' set but not used [-Werror=unused-but-set-variable]
62 | char *strtab;
| ^~~~~~
cc1: all warnings being treated as errors
--
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/sparc/include/asm/cmpxchg_64.h:111,
from arch/sparc/include/asm/cmpxchg.h:5,
from arch/sparc/include/asm/atomic_64.h:12,
from arch/sparc/include/asm/atomic.h:5,
from include/linux/atomic.h:7,
from include/asm-generic/bitops/lock.h:5,
from arch/sparc/include/asm/bitops_64.h:52,
from arch/sparc/include/asm/bitops.h:5,
from include/linux/bitops.h:68,
from include/linux/kernel.h:22,
from arch/sparc/kernel/pci_sun4v.c:7:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
36 | struct task_struct *idle) {}
| ^~~~~~~~~~~
arch/sparc/kernel/pci_sun4v.c:258:15: error: no previous prototype for 'dma_4v_iotsb_bind' [-Werror=missing-prototypes]
258 | unsigned long dma_4v_iotsb_bind(unsigned long devhandle,
| ^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
--
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/sparc/include/asm/cmpxchg_64.h:111,
from arch/sparc/include/asm/cmpxchg.h:5,
from arch/sparc/include/asm/atomic_64.h:12,
from arch/sparc/include/asm/atomic.h:5,
from include/linux/atomic.h:7,
from include/asm-generic/bitops/lock.h:5,
from arch/sparc/include/asm/bitops_64.h:52,
from arch/sparc/include/asm/bitops.h:5,
from include/linux/bitops.h:68,
from include/linux/kernel.h:22,
from arch/sparc/kernel/uprobes.c:12:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
36 | struct task_struct *idle) {}
| ^~~~~~~~~~~
arch/sparc/kernel/uprobes.c:237:17: error: no previous prototype for 'uprobe_trap' [-Werror=missing-prototypes]
237 | asmlinkage void uprobe_trap(struct pt_regs *regs,
| ^~~~~~~~~~~
cc1: all warnings being treated as errors
--
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/sparc/include/asm/cmpxchg_64.h:111,
from arch/sparc/include/asm/cmpxchg.h:5,
from arch/sparc/include/asm/atomic_64.h:12,
from arch/sparc/include/asm/atomic.h:5,
from include/linux/atomic.h:7,
from include/asm-generic/bitops/lock.h:5,
from arch/sparc/include/asm/bitops_64.h:52,
from arch/sparc/include/asm/bitops.h:5,
from include/linux/bitops.h:68,
from include/linux/kernel.h:22,
from include/linux/sched/mm.h:5,
from arch/sparc/kernel/traps_64.c:13:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
36 | struct task_struct *idle) {}
| ^~~~~~~~~~~
arch/sparc/kernel/traps_64.c:252:6: error: no previous prototype for 'is_no_fault_exception' [-Werror=missing-prototypes]
252 | bool is_no_fault_exception(struct pt_regs *regs)
| ^~~~~~~~~~~~~~~~~~~~~
arch/sparc/kernel/traps_64.c:2034:6: error: no previous prototype for 'do_mcd_err' [-Werror=missing-prototypes]
2034 | void do_mcd_err(struct pt_regs *regs, struct sun4v_error_entry ent)
| ^~~~~~~~~~
arch/sparc/kernel/traps_64.c:2152:6: error: no previous prototype for 'sun4v_nonresum_error_user_handled' [-Werror=missing-prototypes]
2152 | bool sun4v_nonresum_error_user_handled(struct pt_regs *regs,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/sparc/kernel/traps_64.c:2839:13: error: no previous prototype for 'trap_init' [-Werror=missing-prototypes]
2839 | void __init trap_init(void)
| ^~~~~~~~~
cc1: all warnings being treated as errors
--
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/sparc/include/asm/cmpxchg_64.h:111,
from arch/sparc/include/asm/cmpxchg.h:5,
from arch/sparc/include/asm/atomic_64.h:12,
from arch/sparc/include/asm/atomic.h:5,
from include/linux/atomic.h:7,
from include/asm-generic/bitops/lock.h:5,
from arch/sparc/include/asm/bitops_64.h:52,
from arch/sparc/include/asm/bitops.h:5,
from include/linux/bitops.h:68,
from include/linux/thread_info.h:27,
from arch/sparc/include/asm/current.h:15,
from include/linux/sched.h:12,
from arch/sparc/kernel/setup_64.c:10:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
36 | struct task_struct *idle) {}
| ^~~~~~~~~~~
arch/sparc/kernel/setup_64.c:615:13: error: no previous prototype for 'alloc_irqstack_bootmem' [-Werror=missing-prototypes]
615 | void __init alloc_irqstack_bootmem(void)
| ^~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
--
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/sparc/include/asm/cmpxchg_64.h:111,
from arch/sparc/include/asm/cmpxchg.h:5,
from arch/sparc/include/asm/atomic_64.h:12,
from arch/sparc/include/asm/atomic.h:5,
from include/linux/atomic.h:7,
from include/asm-generic/bitops/lock.h:5,
from arch/sparc/include/asm/bitops_64.h:52,
from arch/sparc/include/asm/bitops.h:5,
from include/linux/bitops.h:68,
from include/linux/thread_info.h:27,
from arch/sparc/include/asm/current.h:15,
from include/linux/sched.h:12,
from arch/sparc/kernel/time_64.c:14:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
36 | struct task_struct *idle) {}
| ^~~~~~~~~~~
arch/sparc/kernel/time_64.c:880:20: error: no previous prototype for 'sched_clock' [-Werror=missing-prototypes]
880 | unsigned long long sched_clock(void)
| ^~~~~~~~~~~
cc1: all warnings being treated as errors
--
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/sparc/include/asm/cmpxchg_64.h:111,
from arch/sparc/include/asm/cmpxchg.h:5,
from arch/sparc/include/asm/atomic_64.h:12,
from arch/sparc/include/asm/atomic.h:5,
from include/linux/atomic.h:7,
from include/asm-generic/bitops/lock.h:5,
from arch/sparc/include/asm/bitops_64.h:52,
from arch/sparc/include/asm/bitops.h:5,
from include/linux/bitops.h:68,
from include/linux/kernel.h:22,
from arch/sparc/mm/init_64.c:10:
>> include/linux/irqflags.h:36:54: error: 'struct task_struct' declared inside parameter list will not be visible outside of this definition or declaration [-Werror]
36 | struct task_struct *idle) {}
| ^~~~~~~~~~~
arch/sparc/mm/init_64.c: In function 'arch_hugetlb_valid_size':
arch/sparc/mm/init_64.c:355:24: error: variable 'hv_pgsz_idx' set but not used [-Werror=unused-but-set-variable]
355 | unsigned short hv_pgsz_idx;
| ^~~~~~~~~~~
arch/sparc/mm/init_64.c: At top level:
arch/sparc/mm/init_64.c:2630:6: error: no previous prototype for 'vmemmap_free' [-Werror=missing-prototypes]
2630 | void vmemmap_free(unsigned long start, unsigned long end,
| ^~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +36 include/linux/irqflags.h
19
20 /* Currently lockdep_softirqs_on/off is used only by lockdep */
21 #ifdef CONFIG_PROVE_LOCKING
22 extern void lockdep_softirqs_on(unsigned long ip);
23 extern void lockdep_softirqs_off(unsigned long ip);
24 extern void lockdep_hardirqs_on_prepare(void);
25 extern void lockdep_hardirqs_on(unsigned long ip);
26 extern void lockdep_hardirqs_off(unsigned long ip);
27 extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
28 struct task_struct *idle);
29 #else
30 static inline void lockdep_softirqs_on(unsigned long ip) { }
31 static inline void lockdep_softirqs_off(unsigned long ip) { }
32 static inline void lockdep_hardirqs_on_prepare(void) { }
33 static inline void lockdep_hardirqs_on(unsigned long ip) { }
34 static inline void lockdep_hardirqs_off(unsigned long ip) { }
35 static inline void lockdep_cleanup_dead_cpu(unsigned int cpu,
> 36 struct task_struct *idle) {}
37 #endif
38
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Sat, Oct 28 2023 at 20:24, David Woodhouse wrote: > @@ -24,12 +24,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); Lacks a forward declaration of 'struct task_struct'
On Sun, 2023-10-29 at 18:33 +0100, Thomas Gleixner wrote: > On Sat, Oct 28 2023 at 20:24, David Woodhouse wrote: > > @@ -24,12 +24,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); > > Lacks a forward declaration of 'struct task_struct' > Apparently so; I thought that was fairly much ubiquitous. Was debating spamming you with a v3 in the space of as many days, or perhaps revisiting my decision to *pass* the idle task out of kernel/cpu.c. We could always shift the declaration of idle_thread_get() out to linux/smpboot.h and let the lockdep code call it directly. You already reviewed my patch to do that, although it was dropped in the end. https://lore.kernel.org/lkml/20230321194008.785922-2-usama.arif@bytedance.com/
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tip/smp/core]
[also build test WARNING on tip/locking/core linus/master v6.6-rc7 next-20231027]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/David-Woodhouse/lockdep-add-lockdep_cleanup_dead_cpu/20231029-032722
base: tip/smp/core
patch link: https://lore.kernel.org/r/635fa006e8f3816b4a36b964d6281f0d8efa789b.camel%40infradead.org
patch subject: [PATCH v2] lockdep: add lockdep_cleanup_dead_cpu()
config: um-allnoconfig (https://download.01.org/0day-ci/archive/20231029/202310291748.ld6JE1m3-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231029/202310291748.ld6JE1m3-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310291748.ld6JE1m3-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from init/init_task.c:2:
In file included from include/linux/init_task.h:5:
In file included from include/linux/rcupdate.h:26:
>> include/linux/irqflags.h:36:19: warning: declaration of 'struct task_struct' will not be visible outside of this function [-Wvisibility]
36 | struct task_struct *idle) {}
| ^
In file included from init/init_task.c:2:
In file included from include/linux/init_task.h:9:
In file included from include/linux/ftrace.h:10:
In file included from include/linux/trace_recursion.h:5:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from init/init_task.c:2:
In file included from include/linux/init_task.h:9:
In file included from include/linux/ftrace.h:10:
In file included from include/linux/trace_recursion.h:5:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from init/init_task.c:2:
In file included from include/linux/init_task.h:9:
In file included from include/linux/ftrace.h:10:
In file included from include/linux/trace_recursion.h:5:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
692 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
700 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
708 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
717 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
726 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
735 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
13 warnings generated.
--
In file included from arch/um/kernel/reboot.c:6:
In file included from include/linux/sched/signal.h:5:
In file included from include/linux/rculist.h:11:
In file included from include/linux/rcupdate.h:26:
>> include/linux/irqflags.h:36:19: warning: declaration of 'struct task_struct' will not be visible outside of this function [-Wvisibility]
36 | struct task_struct *idle) {}
| ^
arch/um/kernel/reboot.c:45:6: warning: no previous prototype for function 'machine_restart' [-Wmissing-prototypes]
45 | void machine_restart(char * __unused)
| ^
arch/um/kernel/reboot.c:45:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
45 | void machine_restart(char * __unused)
| ^
| static
arch/um/kernel/reboot.c:51:6: warning: no previous prototype for function 'machine_power_off' [-Wmissing-prototypes]
51 | void machine_power_off(void)
| ^
arch/um/kernel/reboot.c:51:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
51 | void machine_power_off(void)
| ^
| static
arch/um/kernel/reboot.c:57:6: warning: no previous prototype for function 'machine_halt' [-Wmissing-prototypes]
57 | void machine_halt(void)
| ^
arch/um/kernel/reboot.c:57:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
57 | void machine_halt(void)
| ^
| static
4 warnings generated.
--
In file included from arch/um/kernel/early_printk.c:7:
In file included from include/linux/console.h:19:
In file included from include/linux/rculist.h:11:
In file included from include/linux/rcupdate.h:26:
>> include/linux/irqflags.h:36:19: warning: declaration of 'struct task_struct' will not be visible outside of this function [-Wvisibility]
36 | struct task_struct *idle) {}
| ^
1 warning generated.
--
In file included from kernel/irq_work.c:12:
In file included from include/linux/irq_work.h:6:
In file included from include/linux/rcuwait.h:5:
In file included from include/linux/rcupdate.h:26:
>> include/linux/irqflags.h:36:19: warning: declaration of 'struct task_struct' will not be visible outside of this function [-Wvisibility]
36 | struct task_struct *idle) {}
| ^
In file included from kernel/irq_work.c:14:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from kernel/irq_work.c:14:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from kernel/irq_work.c:14:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
692 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
700 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
708 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
717 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
726 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
735 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
kernel/irq_work.c:72:13: warning: no previous prototype for function 'arch_irq_work_raise' [-Wmissing-prototypes]
72 | void __weak arch_irq_work_raise(void)
| ^
kernel/irq_work.c:72:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
72 | void __weak arch_irq_work_raise(void)
| ^
| static
14 warnings generated.
--
In file included from lib/maple_tree.c:54:
In file included from include/linux/maple_tree.h:12:
In file included from include/linux/rcupdate.h:26:
>> include/linux/irqflags.h:36:19: warning: declaration of 'struct task_struct' will not be visible outside of this function [-Wvisibility]
36 | struct task_struct *idle) {}
| ^
lib/maple_tree.c:331:21: warning: unused function 'mte_set_full' [-Wunused-function]
331 | static inline void *mte_set_full(const struct maple_enode *node)
| ^
lib/maple_tree.c:336:21: warning: unused function 'mte_clear_full' [-Wunused-function]
336 | static inline void *mte_clear_full(const struct maple_enode *node)
| ^
lib/maple_tree.c:341:20: warning: unused function 'mte_has_null' [-Wunused-function]
341 | static inline bool mte_has_null(const struct maple_enode *node)
| ^
lib/maple_tree.c:672:29: warning: unused function 'mas_pivot' [-Wunused-function]
672 | static inline unsigned long mas_pivot(struct ma_state *mas, unsigned char piv)
| ^
lib/maple_tree.c:4319:20: warning: stack frame size (1064) exceeds limit (1024) in 'mas_wr_modify' [-Wframe-larger-than]
4319 | static inline void mas_wr_modify(struct ma_wr_state *wr_mas)
| ^
6 warnings generated.
vim +36 include/linux/irqflags.h
19
20 /* Currently lockdep_softirqs_on/off is used only by lockdep */
21 #ifdef CONFIG_PROVE_LOCKING
22 extern void lockdep_softirqs_on(unsigned long ip);
23 extern void lockdep_softirqs_off(unsigned long ip);
24 extern void lockdep_hardirqs_on_prepare(void);
25 extern void lockdep_hardirqs_on(unsigned long ip);
26 extern void lockdep_hardirqs_off(unsigned long ip);
27 extern void lockdep_cleanup_dead_cpu(unsigned int cpu,
28 struct task_struct *idle);
29 #else
30 static inline void lockdep_softirqs_on(unsigned long ip) { }
31 static inline void lockdep_softirqs_off(unsigned long ip) { }
32 static inline void lockdep_hardirqs_on_prepare(void) { }
33 static inline void lockdep_hardirqs_on(unsigned long ip) { }
34 static inline void lockdep_hardirqs_off(unsigned long ip) { }
35 static inline void lockdep_cleanup_dead_cpu(unsigned int cpu,
> 36 struct task_struct *idle) {}
37 #endif
38
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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 - 2025 Red Hat, Inc.