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(-)
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
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 <[email protected]>
> ---
> 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.
(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 <[email protected]>
>> ---
>> 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