Re: [PATCH V2] x86/mce: Serialize sysfs controls against CPU hotplug

[Yazen Ghannam <yazen.ghannam@amd.com>]

On Wed, Oct 28, 2026 at 08:45:07PM +0100, Thomas Gleixner wrote:
> On Fri, Oct 23 2026 at 11:18, Thomas Gleixner wrote:
> syzbot reported a NULL pointer dereference in the CPU hotplug code. It
> is caused by the lack of synchronizaton between CPU hotplug and MCE
> sysfs controls, which allows the sysfs controls to reenable not only the
> timer, but also the CMCI reporting on a CPU which is about to go out. A
> similar issue exists in the CPU online path where the timer can be
> enabled via sysfs control before the CPU runs the initialization of the
> timer.
> 
> Prevent this by:
> 
>    1) Serializing the sysfs writes against CPU hotplug.
> 
>    2) Removing the redundant timer_setup() call in
>       __mcheck_cpu_init_timer(), which can end up in initializing an
>       active timer.
> 
>    3) Splitting the hotplug callbacks into starting/dying and
>       online/offline parts.
>    
>       The starting/dying callbacks control the timer and the CMCI
>       setup/teardown. The online/offline callbacks handle the device
>       creation and teardown.
> 
> This ensures that the sysfs operations always see consistent CPU online
> state and the timer and CMCI are shut down at the time when the CPU goes
> offline and re-enabled when the CPU starts without interfering with the
> sysfs operations.
> 
> Fixes: 9af43b54ab45 ("x86, mce: sysfs entries for new mce options")
> Reported-by: syzbot+74de56995244fe32ffe2@syzkaller.appspotmail.com
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>
> Closes: https://lore.kernel.org/all/6a7ec39f.5b0d2c79.2ef4ef.0002.GAE@google.com

There are three others that seem to be the same issue.

Reported-by: syzbot+9d17de59c48d71bd6824@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/6a6cd87b.2d659fcc.1d46f5.019c.GAE@google.com/
Reported-by: syzbot+553c9f2c5781192c2214@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/6ad23c08.7952420f.283480.0348.GAE@google.com/
Reported-by: syzbot+edc6b57cbed1fb72d0f9@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/6a6cd832.1aa927e4.17d4bf.0008.GAE@google.com/

> ---
> V2: Use the seperate hotplug state as sashiko rightfully pointed out
>     that taking the sysfs mutex in the hotplug callback can deadlock
>     against the device creation/deletion.
> ---

I was looking at this issue before and have some QEMU test cases.

These were able to hit the new Sashiko findings.

There are two separate ones.
1) Another deadlock.
2) PREEMPT_RT warning.

The diff below fixes both. Though I'm not sure the if the msleep() is
okay.

Thanks,
Yazen

=====================

diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index bcf4cba2b7ab..6d74e393eb6a 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -2512,12 +2512,36 @@ static ssize_t show_bank(struct device *s, struct device_attribute *attr,
 	return sprintf(buf, "%llx\n", b->ctl);
 }
 
+/*
+ * The stores run with the kernfs active reference of their attribute held.
+ * CPU hotplug removes the attributes with cpu_hotplug_lock held and waits
+ * for that reference to drop. So never wait for cpu_hotplug_lock here.
+ * Restart the syscall instead, like lock_device_hotplug_sysfs().
+ */
+static int mce_sysfs_lock(void)
+{
+	if (cpus_read_trylock()) {
+		mutex_lock(&mce_sysfs_mutex);
+		return 0;
+	}
+	/* Avoid busy looping (5 ms of sleep should do). */
+	msleep(5);
+	return restart_syscall();
+}
+
+static void mce_sysfs_unlock(void)
+{
+	mutex_unlock(&mce_sysfs_mutex);
+	cpus_read_unlock();
+}
+
 static ssize_t set_bank(struct device *s, struct device_attribute *attr,
 			const char *buf, size_t size)
 {
 	u8 bank = attr_to_bank(attr)->bank;
 	struct mce_bank *b;
 	u64 new;
+	int err;
 
 	if (kstrtou64(buf, 0, &new) < 0)
 		return -EINVAL;
@@ -2529,12 +2553,14 @@ static ssize_t set_bank(struct device *s, struct device_attribute *attr,
 	if (!b->init)
 		return -ENODEV;
 
-	b->ctl = new;
+	err = mce_sysfs_lock();
+	if (err)
+		return err;
 
-	guard(cpus_read_lock)();
-	guard(mutex)(&mce_sysfs_mutex);
+	b->ctl = new;
 	mce_restart();
 
+	mce_sysfs_unlock();
 	return size;
 }
 
@@ -2543,12 +2569,14 @@ static ssize_t set_ignore_ce(struct device *s,
 			     const char *buf, size_t size)
 {
 	u64 new;
+	int err;
 
 	if (kstrtou64(buf, 0, &new) < 0)
 		return -EINVAL;
 
-	guard(cpus_read_lock)();
-	guard(mutex)(&mce_sysfs_mutex);
+	err = mce_sysfs_lock();
+	if (err)
+		return err;
 
 	if (mca_cfg.ignore_ce ^ !!new) {
 		if (new) {
@@ -2563,6 +2591,7 @@ static ssize_t set_ignore_ce(struct device *s,
 		}
 	}
 
+	mce_sysfs_unlock();
 	return size;
 }
 
@@ -2571,12 +2600,14 @@ static ssize_t set_cmci_disabled(struct device *s,
 				 const char *buf, size_t size)
 {
 	u64 new;
+	int err;
 
 	if (kstrtou64(buf, 0, &new) < 0)
 		return -EINVAL;
 
-	guard(cpus_read_lock)();
-	guard(mutex)(&mce_sysfs_mutex);
+	err = mce_sysfs_lock();
+	if (err)
+		return err;
 
 	if (mca_cfg.cmci_disabled ^ !!new) {
 		if (new) {
@@ -2590,6 +2621,7 @@ static ssize_t set_cmci_disabled(struct device *s,
 		}
 	}
 
+	mce_sysfs_unlock();
 	return size;
 }
 
@@ -2597,15 +2629,20 @@ static ssize_t store_int_with_restart(struct device *s,
 				      struct device_attribute *attr,
 				      const char *buf, size_t size)
 {
-	unsigned long old_check_interval = check_interval;
-	ssize_t ret = device_store_ulong(s, attr, buf, size);
+	unsigned long old_check_interval;
+	ssize_t ret;
+	int err;
 
-	if (check_interval == old_check_interval)
-		return ret;
+	err = mce_sysfs_lock();
+	if (err)
+		return err;
 
-	guard(cpus_read_lock)();
-	guard(mutex)(&mce_sysfs_mutex);
-	mce_restart();
+	old_check_interval = check_interval;
+	ret = device_store_ulong(s, attr, buf, size);
+	if (check_interval != old_check_interval)
+		mce_restart();
+
+	mce_sysfs_unlock();
 	return ret;
 }
 
@@ -2769,7 +2806,8 @@ static int mce_cpu_dying(unsigned int cpu)
 	struct timer_list *t = this_cpu_ptr(&mce_timer);
 
 	mce_disable_cpu();
-	timer_delete_sync(t);
+	/* Pinned to this dying CPU, so the callback can't be running. */
+	timer_delete(t);
 	return 0;
 }