x86, mce: stop calling del_timer_sync() from interrupt

Use of on_each_cpu() results in calling the function passed as the argument on interrupt context.

Calling del_timer_sync() from interrupt context can cause deadlock if it interrupts the target timer running.

MCE code has some such misuse of del_timer_sync() in parts for sysfs file; bank*, check_interval, cmci_disabled and ignore_ce. Fortunately these files are rare-used but you will be warned on write.

This patch will fix it.

Signed-off-by Hidetoshi Seto <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +11 -0

index ff1ae9b..77df54f 100644
@@ -1170,6 +1170,17 @@ static void mce_start_timer(unsigned long data)
 	add_timer_on(t, smp_processor_id());
 }
 
+/* Must not be called from interrupt where del_timer_sync() can deadlock */
+static void mce_timer_delete_all(void)
+{
+	int cpu;
+
+	for_each_online_cpu(cpu) {
+		if (mce_available(&per_cpu(cpu_info, cpu)))
+			del_timer_sync(&per_cpu(mce_timer, cpu));
+	}
+}
+
 static void mce_do_trigger(struct work_struct *work)
 {
 	call_usermodehelper(mce_helper, mce_helper_argv, NULL, UMH_NO_WAIT);

arch/x86/kernel/cpu/mcheck/mce.c +5 -5

 	__mcheck_cpu_init_generic();

@@ -1778,16 +1788,15 @@ static void mce_cpu_restart(void *data)
 /* Reinit MCEs after user configuration changes */
 static void mce_restart(void)
 {
+	mce_timer_delete_all();
 	on_each_cpu(mce_cpu_restart, NULL, 1);
 }
 
 /* Toggle features for corrected errors */
-static void mce_disable_ce(void *all)
+static void mce_disable_cmci(void *data)
 {
 	if (!mce_available(__this_cpu_ptr(&cpu_info)))
 		return;
-	if (all)
-		del_timer_sync(&__get_cpu_var(mce_timer));
 	cmci_clear();
 }
 

@@ -1870,7 +1879,8 @@ static ssize_t set_ignore_ce(struct sys_device *s,
 	if (mce_ignore_ce ^ !!new) {
 		if (new) {
 			/* disable ce features */
-			on_each_cpu(mce_disable_ce, (void *)1, 1);
+			mce_timer_delete_all();
+			on_each_cpu(mce_disable_cmci, NULL, 1);
 			mce_ignore_ce = 1;
 		} else {
 			/* enable ce features */

@@ -1893,7 +1903,7 @@ static ssize_t set_cmci_disabled(struct sys_device *s,
 	if (mce_cmci_disabled ^ !!new) {
 		if (new) {
 			/* disable cmci */
-			on_each_cpu(mce_disable_ce, NULL, 1);
+			on_each_cpu(mce_disable_cmci, NULL, 1);
 			mce_cmci_disabled = 1;
 		} else {
 			/* enable cmci */

Signature · 1 line

1.7.1

Quoted 2 lines · Hidetoshi Seto

Use of on_each_cpu() results in calling the function passed as the argument on interrupt context.

in

Quoted 3 lines

Calling del_timer_sync() from interrupt context can cause deadlock if it interrupts the target timer running.

Quoted 3 lines

MCE code has some such misuse of del_timer_sync() in parts for sysfs file; bank*, check_interval, cmci_disabled and ignore_ce. Fortunately these files are rare-used but you will be warned on write.

So, you're saying you're hitting the WARN_ON(in_irq()) in del_timer_sync() ?

Quoted 1 line

This patch will fix it.

You could say

"Move timer deletion outside of the interrupt context as a fix"

or something to that effect to make it much more precise what the patch is doing.

Patch idea looks fine, see below for some nitpicking :).

Quoted 29 lines

Signed-off-by Hidetoshi Seto <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +11 -0

index ff1ae9b..77df54f 100644
@@ -1170,6 +1170,17 @@ static void mce_start_timer(unsigned long data)
 	add_timer_on(t, smp_processor_id());
 }
 
+/* Must not be called from interrupt where del_timer_sync() can deadlock */
+static void mce_timer_delete_all(void)
+{
+	int cpu;
+
+	for_each_online_cpu(cpu) {
+		if (mce_available(&per_cpu(cpu_info, cpu)))
+			del_timer_sync(&per_cpu(mce_timer, cpu));
+	}
+}
+
 static void mce_do_trigger(struct work_struct *work)
 {
 	call_usermodehelper(mce_helper, mce_helper_argv, NULL, UMH_NO_WAIT);

arch/x86/kernel/cpu/mcheck/mce.c static struct syscore_ops mce_syscore_ops = { · line 1783

Quoted 6 lines

arch/x86/kernel/cpu/mcheck/mce.c +0 -1

@@ -1768,7 +1779,6 @@ static struct syscore_ops mce_syscore_ops = {
 
 static void mce_cpu_restart(void *data)
 {
-	del_timer_sync(&__get_cpu_var(mce_timer));
 	if (!mce_available(__this_cpu_ptr(&cpu_info)))
 		return;

I'm wondering, was this actually a bug - the fact that we deleted the timer _before_ we do the mce_available() check.

In looking at it a bit more, it looks like mce_restart() is always executed on codepaths behind mce_available() checks so the

	if (mce_available(...))
		del_timer_sync(...);

part in mce_timer_delete_all could be done without the if-check, no?