[PATCH] x86, microcode: Add local mutex to not hit a deadlock.

[PATCH] x86, microcode: Add local mutex to not hit a deadlock.

[Konrad Rzeszutek Wilk]

This can easily be triggered if a new CPU is added (via
ACPI hotplug mechanism) and from user-space do:

echo 1 > /sys/devices/system/cpu/cpu3/online

(or wait for UDEV to do it) on a newly appeared CPU.

The deadlock is that the "store_online" in drivers/base/cpu.c
takes the cpu_hotplug_driver_lock() lock, then calls "cpu_up".
"cpu_up" eventually ends up calling "save_mc_for_early"
which also takes the cpu_hotplug_driver_lock() lock.

And here is that kernel thinks of it:

smpboot: Stack at about ffff880075c39f44
smpboot: CPU3: has booted.
microcode: CPU3 sig=0x206a7, pf=0x2, revision=0x25

=============================================
[ INFO: possible recursive locking detected ]
3.9.0upstream-10129-g167af0e #1 Not tainted
---------------------------------------------
sh/2487 is trying to acquire lock:
 (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20

but task is already holding lock:
 (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(x86_cpu_hotplug_driver_mutex);
  lock(x86_cpu_hotplug_driver_mutex);

 *** DEADLOCK ***

 May be due to missing lock nesting notation

6 locks held by sh/2487:
 #0:  (sb_writers#5){.+.+.+}, at: [<ffffffff811ca48d>] vfs_write+0x17d/0x190
 #1:  (&buffer->mutex){+.+.+.}, at: [<ffffffff812464ef>] sysfs_write_file+0x3f/0x160
 #2:  (s_active#20){.+.+.+}, at: [<ffffffff81246578>] sysfs_write_file+0xc8/0x160
 #3:  (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20
 #4:  (cpu_add_remove_lock){+.+.+.}, at: [<ffffffff810961c2>] cpu_maps_update_begin+0x12/0x20
 #5:  (cpu_hotplug.lock){+.+.+.}, at: [<ffffffff810962a7>] cpu_hotplug_begin+0x27/0x60

Suggested-by: Borislav Petkov <bp@alien8.de>
CC: stable@vger.kernel.org # for v3.9
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/kernel/microcode_intel_early.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/microcode_intel_early.c b/arch/x86/kernel/microcode_intel_early.c
index d893e8e..2e9e128 100644
--- a/arch/x86/kernel/microcode_intel_early.c
+++ b/arch/x86/kernel/microcode_intel_early.c
@@ -487,6 +487,7 @@ static inline void show_saved_mc(void)
 #endif
 
 #if defined(CONFIG_MICROCODE_INTEL_EARLY) && defined(CONFIG_HOTPLUG_CPU)
+static DEFINE_MUTEX(x86_cpu_microcode_mutex);
 /*
  * Save this mc into mc_saved_data. So it will be loaded early when a CPU is
  * hot added or resumes.
@@ -507,7 +508,7 @@ int save_mc_for_early(u8 *mc)
 	 * Hold hotplug lock so mc_saved_data is not accessed by a CPU in
 	 * hotplug.
 	 */
-	cpu_hotplug_driver_lock();
+	mutex_lock(&x86_cpu_microcode_mutex);
 
 	mc_saved_count_init = mc_saved_data.mc_saved_count;
 	mc_saved_count = mc_saved_data.mc_saved_count;
@@ -544,7 +545,7 @@ int save_mc_for_early(u8 *mc)
 	}
 
 out:
-	cpu_hotplug_driver_unlock();
+	mutex_unlock(&x86_cpu_microcode_mutex);
 
 	return ret;
 }
-- 
1.7.7.6

Re: [PATCH] x86, microcode: Add local mutex to not hit a deadlock.

[Borislav Petkov]

On Wed, May 08, 2013 at 12:13:03PM -0400, Konrad Rzeszutek Wilk wrote:
> This can easily be triggered if a new CPU is added (via
> ACPI hotplug mechanism) and from user-space do:
> 
> echo 1 > /sys/devices/system/cpu/cpu3/online
> 
> (or wait for UDEV to do it) on a newly appeared CPU.
> 
> The deadlock is that the "store_online" in drivers/base/cpu.c
> takes the cpu_hotplug_driver_lock() lock, then calls "cpu_up".
> "cpu_up" eventually ends up calling "save_mc_for_early"
> which also takes the cpu_hotplug_driver_lock() lock.
> 
> And here is that kernel thinks of it:
> 
> smpboot: Stack at about ffff880075c39f44
> smpboot: CPU3: has booted.
> microcode: CPU3 sig=0x206a7, pf=0x2, revision=0x25
> 
> =============================================
> [ INFO: possible recursive locking detected ]
> 3.9.0upstream-10129-g167af0e #1 Not tainted
> ---------------------------------------------
> sh/2487 is trying to acquire lock:
>  (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20
> 
> but task is already holding lock:
>  (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>        ----
>   lock(x86_cpu_hotplug_driver_mutex);
>   lock(x86_cpu_hotplug_driver_mutex);
> 
>  *** DEADLOCK ***

Ok, just for my own understanding: is this something which can actually
happen now?

Judging by the presence of traces, it can be triggered in a guest,
correct?

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] x86, microcode: Add local mutex to not hit a deadlock.

[Konrad Rzeszutek Wilk]

On Wed, May 08, 2013 at 08:22:35PM +0200, Borislav Petkov wrote:
> On Wed, May 08, 2013 at 12:13:03PM -0400, Konrad Rzeszutek Wilk wrote:
> > This can easily be triggered if a new CPU is added (via
> > ACPI hotplug mechanism) and from user-space do:
> > 
> > echo 1 > /sys/devices/system/cpu/cpu3/online
> > 
> > (or wait for UDEV to do it) on a newly appeared CPU.
> > 
> > The deadlock is that the "store_online" in drivers/base/cpu.c
> > takes the cpu_hotplug_driver_lock() lock, then calls "cpu_up".
> > "cpu_up" eventually ends up calling "save_mc_for_early"
> > which also takes the cpu_hotplug_driver_lock() lock.
> > 
> > And here is that kernel thinks of it:
> > 
> > smpboot: Stack at about ffff880075c39f44
> > smpboot: CPU3: has booted.
> > microcode: CPU3 sig=0x206a7, pf=0x2, revision=0x25
> > 
> > =============================================
> > [ INFO: possible recursive locking detected ]
> > 3.9.0upstream-10129-g167af0e #1 Not tainted
> > ---------------------------------------------
> > sh/2487 is trying to acquire lock:
> >  (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20
> > 
> > but task is already holding lock:
> >  (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20
> > 
> > other info that might help us debug this:
> >  Possible unsafe locking scenario:
> > 
> >        CPU0
> >        ----
> >   lock(x86_cpu_hotplug_driver_mutex);
> >   lock(x86_cpu_hotplug_driver_mutex);
> > 
> >  *** DEADLOCK ***
> 
> Ok, just for my own understanding: is this something which can actually
> happen now?

Yes. Since v3.9 I've been getting this.
> 
> Judging by the presence of traces, it can be triggered in a guest,
> correct?

Correct. I don't have the hardware to test this on baremetal but looking
at how processor-driver.c works I would think this would be hit there as well.

> 
> Thanks.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --

RE: [PATCH] x86, microcode: Add local mutex to not hit a deadlock.

[Yu, Fenghua]

> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> 
> This can easily be triggered if a new CPU is added (via
> ACPI hotplug mechanism) and from user-space do:
> 
> echo 1 > /sys/devices/system/cpu/cpu3/online
> 
> (or wait for UDEV to do it) on a newly appeared CPU.
> 
> The deadlock is that the "store_online" in drivers/base/cpu.c
> takes the cpu_hotplug_driver_lock() lock, then calls "cpu_up".
> "cpu_up" eventually ends up calling "save_mc_for_early"
> which also takes the cpu_hotplug_driver_lock() lock.
> 
> And here is that kernel thinks of it:
> 
> smpboot: Stack at about ffff880075c39f44
> smpboot: CPU3: has booted.
> microcode: CPU3 sig=0x206a7, pf=0x2, revision=0x25
> 
> =============================================
> [ INFO: possible recursive locking detected ]
> 3.9.0upstream-10129-g167af0e #1 Not tainted
> ---------------------------------------------
> sh/2487 is trying to acquire lock:
>  (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>]
> cpu_hotplug_driver_lock+0x12/0x20
> 
> but task is already holding lock:
>  (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>]
> cpu_hotplug_driver_lock+0x12/0x20
> 
> other info that might help us debug this:
>  Possible unsafe locking scenario:
> 
>        CPU0
>        ----
>   lock(x86_cpu_hotplug_driver_mutex);
>   lock(x86_cpu_hotplug_driver_mutex);
> 
>  *** DEADLOCK ***
> 
>  May be due to missing lock nesting notation
> 
> 6 locks held by sh/2487:
>  #0:  (sb_writers#5){.+.+.+}, at: [<ffffffff811ca48d>]
> vfs_write+0x17d/0x190
>  #1:  (&buffer->mutex){+.+.+.}, at: [<ffffffff812464ef>]
> sysfs_write_file+0x3f/0x160
>  #2:  (s_active#20){.+.+.+}, at: [<ffffffff81246578>]
> sysfs_write_file+0xc8/0x160
>  #3:  (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>]
> cpu_hotplug_driver_lock+0x12/0x20
>  #4:  (cpu_add_remove_lock){+.+.+.}, at: [<ffffffff810961c2>]
> cpu_maps_update_begin+0x12/0x20
>  #5:  (cpu_hotplug.lock){+.+.+.}, at: [<ffffffff810962a7>]
> cpu_hotplug_begin+0x27/0x60
> 
> Suggested-by: Borislav Petkov <bp@alien8.de>
> CC: stable@vger.kernel.org # for v3.9
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  arch/x86/kernel/microcode_intel_early.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/microcode_intel_early.c
> b/arch/x86/kernel/microcode_intel_early.c
> index d893e8e..2e9e128 100644
> --- a/arch/x86/kernel/microcode_intel_early.c
> +++ b/arch/x86/kernel/microcode_intel_early.c
> @@ -487,6 +487,7 @@ static inline void show_saved_mc(void)
>  #endif
> 
>  #if defined(CONFIG_MICROCODE_INTEL_EARLY) &&
> defined(CONFIG_HOTPLUG_CPU)
> +static DEFINE_MUTEX(x86_cpu_microcode_mutex);
>  /*
>   * Save this mc into mc_saved_data. So it will be loaded early when a
> CPU is
>   * hot added or resumes.
> @@ -507,7 +508,7 @@ int save_mc_for_early(u8 *mc)
>  	 * Hold hotplug lock so mc_saved_data is not accessed by a CPU in
>  	 * hotplug.
>  	 */

Could you please change the comment to use mutex instead? I think the mutex
is good way to handle race here.

> -	cpu_hotplug_driver_lock();
> +	mutex_lock(&x86_cpu_microcode_mutex);
> 
>  	mc_saved_count_init = mc_saved_data.mc_saved_count;
>  	mc_saved_count = mc_saved_data.mc_saved_count;
> @@ -544,7 +545,7 @@ int save_mc_for_early(u8 *mc)
>  	}
> 
>  out:
> -	cpu_hotplug_driver_unlock();
> +	mutex_unlock(&x86_cpu_microcode_mutex);
> 
>  	return ret;
>  }
> --
> 1.7.7.6