[PATCH] x86, mce: stop calling del_timer_sync() from interrupt

[PATCH] x86, mce: stop calling del_timer_sync() from interrupt

[Hidetoshi Seto]

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 <seto.hidetoshi@jp.fujitsu.com>
---
 arch/x86/kernel/cpu/mcheck/mce.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index ff1ae9b..77df54f 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -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);
@@ -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;
 	__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 */
-- 
1.7.1

Re: [PATCH] x86, mce: stop calling del_timer_sync() from interrupt

[Borislav Petkov]

On Thu, Jun 16, 2011 at 04:28:35PM +0900, Hidetoshi Seto wrote:
> Use of on_each_cpu() results in calling the function passed as the
> argument on interrupt context.

	   in

> 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.

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

> 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 :).

> 
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c |   22 ++++++++++++++++------
>  1 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index ff1ae9b..77df54f 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -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);
> @@ -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?

>  	__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 */
> -- 
> 1.7.1

Thanks.

Re: [PATCH] x86, mce: stop calling del_timer_sync() from interrupt

[Hidetoshi Seto]

(2011/06/16 23:09), Borislav Petkov wrote:
> On Thu, Jun 16, 2011 at 04:28:35PM +0900, Hidetoshi Seto wrote:
>> Use of on_each_cpu() results in calling the function passed as the
>> argument on interrupt context.
> 
> 	   in
> 
>> 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.
> 
> So, you're saying you're hitting the WARN_ON(in_irq()) in
> del_timer_sync() ?

Yes.  It is terrible that there are simultaneous WARN_ON() * online_cpus.
I should have write something clear about that here in patch description.

>> 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.

Sure, I'll update it.
 
> Patch idea looks fine, see below for some nitpicking :).
> 
>>
>> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
>> ---
>>  arch/x86/kernel/cpu/mcheck/mce.c |   22 ++++++++++++++++------
>>  1 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
>> index ff1ae9b..77df54f 100644
>> --- a/arch/x86/kernel/cpu/mcheck/mce.c
>> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
>> @@ -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);
>> @@ -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?

Yes, I've noticed it too.
Notable point is that these sysfs files are not created when
!mce_available(). :-P

However it is an entirely different matter than what this patch is
addressed.  Just to keep old logic in mce_disable_ce(), I've added
mce_available() in the mce_timer_delete_all().

I think we can remove many redundant if-checks w/ mce_available()
around here and there.

Now I'm writing a patch for that... Please wait.


Thanks,
H.Seto