[PATCH] xen/kexec: Prevent deadlock because of one_cpu_only()

[PATCH] xen/kexec: Prevent deadlock because of one_cpu_only()

[Andrew Cooper]

During automated testing of the Xen crash path via fatal NMIs, we discovered
the following deadlock:

[22:47:04 UTC] (XEN) ****************************************
[22:47:04 UTC] (XEN) Panic on CPU 0:
[22:47:04 UTC] (XEN) FATAL TRAP: vector = 2 (nmi)
[22:47:04 UTC] (XEN) [error_code=0000] , IN INTERRUPT CONTEXT
[22:47:04 UTC] (XEN) ****************************************
[22:47:04 UTC] (XEN)
[22:47:04 UTC] (XEN) Reboot in five seconds...
[22:47:05 UTC] (XEN) DMAR_IQA_REG = 839b34002
[22:47:05 UTC] (XEN) DMAR_IQH_REG = 0
[22:47:05 UTC] (XEN) DMAR_IQT_REG = 26b0
[22:47:05 UTC] (XEN)
[22:47:05 UTC] (XEN) ****************************************
[22:47:05 UTC] (XEN) Panic on CPU 0:
[22:47:05 UTC] (XEN) queue invalidate wait descriptor was not executed
[22:47:05 UTC] (XEN) ****************************************
[22:47:05 UTC] (XEN)
[22:47:05 UTC] (XEN) Reboot in five seconds...
[23:09:54 UTC]  The server is not powered on.  The Virtual Serial Port is not available.

The problem is quite difficult to reproduce (seen twice, unable to reproduce
manually), but inspection of kexec_crash() path shows a certain deadlock
because of one_cpu_only().


This patch replaces the use of one_cpu_only() with a recursive spinlock which
provides exclusion between different CPUs racing into kexec_crash(), but
prevents deadlock if the same CPU reenters kexec_crash() again.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 996fbd7657bb -r a559ec5225fc xen/common/kexec.c
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -234,13 +234,6 @@ void __init set_kexec_crash_area_size(u6
     }
 }
 
-static void one_cpu_only(void)
-{
-    /* Only allow the first cpu to continue - force other cpus to spin */
-    if ( test_and_set_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags) )
-        for ( ; ; ) ;
-}
-
 /* Save the registers in the per-cpu crash note buffer. */
 void kexec_crash_save_cpu(void)
 {
@@ -294,19 +287,25 @@ static void kexec_common_shutdown(void)
     watchdog_disable();
     console_start_sync();
     spin_debug_disable();
-    one_cpu_only();
     acpi_dmar_reinstate();
 }
 
 void kexec_crash(void)
 {
+    static DEFINE_SPINLOCK(crash_lock);
     int pos;
 
     pos = (test_bit(KEXEC_FLAG_CRASH_POS, &kexec_flags) != 0);
     if ( !test_bit(KEXEC_IMAGE_CRASH_BASE + pos, &kexec_flags) )
         return;
 
+    /* In a cascade failure, it is possible to re-enter kexec_crash on the same
+     * CPU.  This spinlock is deliberately never unlocked, to allow reentrance
+     * on the same CPU, but provides excusion from different CPUs racing. */
+    spin_lock_recursive(&crash_lock);
+
     kexecing = TRUE;
+    set_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags);
 
     kexec_common_shutdown();
     kexec_crash_save_cpu();

Re: [PATCH] xen/kexec: Prevent deadlock because of one_cpu_only()

[Keir Fraser]

On 05/04/2013 20:52, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

> During automated testing of the Xen crash path via fatal NMIs, we discovered
> the following deadlock:
> 
> [22:47:04 UTC] (XEN) ****************************************
> [22:47:04 UTC] (XEN) Panic on CPU 0:
> [22:47:04 UTC] (XEN) FATAL TRAP: vector = 2 (nmi)
> [22:47:04 UTC] (XEN) [error_code=0000] , IN INTERRUPT CONTEXT
> [22:47:04 UTC] (XEN) ****************************************
> [22:47:04 UTC] (XEN)
> [22:47:04 UTC] (XEN) Reboot in five seconds...
> [22:47:05 UTC] (XEN) DMAR_IQA_REG = 839b34002
> [22:47:05 UTC] (XEN) DMAR_IQH_REG = 0
> [22:47:05 UTC] (XEN) DMAR_IQT_REG = 26b0
> [22:47:05 UTC] (XEN)
> [22:47:05 UTC] (XEN) ****************************************
> [22:47:05 UTC] (XEN) Panic on CPU 0:
> [22:47:05 UTC] (XEN) queue invalidate wait descriptor was not executed
> [22:47:05 UTC] (XEN) ****************************************
> [22:47:05 UTC] (XEN)
> [22:47:05 UTC] (XEN) Reboot in five seconds...
> [23:09:54 UTC]  The server is not powered on.  The Virtual Serial Port is not
> available.
> 
> The problem is quite difficult to reproduce (seen twice, unable to reproduce
> manually), but inspection of kexec_crash() path shows a certain deadlock
> because of one_cpu_only().
> 
> 
> This patch replaces the use of one_cpu_only() with a recursive spinlock which
> provides exclusion between different CPUs racing into kexec_crash(), but
> prevents deadlock if the same CPU reenters kexec_crash() again.

Does kexec_crash() not mind being reentered in this way?

 -- Keir

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> diff -r 996fbd7657bb -r a559ec5225fc xen/common/kexec.c
> --- a/xen/common/kexec.c
> +++ b/xen/common/kexec.c
> @@ -234,13 +234,6 @@ void __init set_kexec_crash_area_size(u6
>      }
>  }
>  
> -static void one_cpu_only(void)
> -{
> -    /* Only allow the first cpu to continue - force other cpus to spin */
> -    if ( test_and_set_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags) )
> -        for ( ; ; ) ;
> -}
> -
>  /* Save the registers in the per-cpu crash note buffer. */
>  void kexec_crash_save_cpu(void)
>  {
> @@ -294,19 +287,25 @@ static void kexec_common_shutdown(void)
>      watchdog_disable();
>      console_start_sync();
>      spin_debug_disable();
> -    one_cpu_only();
>      acpi_dmar_reinstate();
>  }
>  
>  void kexec_crash(void)
>  {
> +    static DEFINE_SPINLOCK(crash_lock);
>      int pos;
>  
>      pos = (test_bit(KEXEC_FLAG_CRASH_POS, &kexec_flags) != 0);
>      if ( !test_bit(KEXEC_IMAGE_CRASH_BASE + pos, &kexec_flags) )
>          return;
>  
> +    /* In a cascade failure, it is possible to re-enter kexec_crash on the
> same
> +     * CPU.  This spinlock is deliberately never unlocked, to allow
> reentrance
> +     * on the same CPU, but provides excusion from different CPUs racing. */
> +    spin_lock_recursive(&crash_lock);
> +
>      kexecing = TRUE;
> +    set_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags);
>  
>      kexec_common_shutdown();
>      kexec_crash_save_cpu();

Re: [PATCH] xen/kexec: Prevent deadlock because of one_cpu_only()

[Andrew Cooper]

On 05/04/2013 22:21, Keir Fraser wrote:
> On 05/04/2013 20:52, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:
>
>> During automated testing of the Xen crash path via fatal NMIs, we discovered
>> the following deadlock:
>>
>> [22:47:04 UTC] (XEN) ****************************************
>> [22:47:04 UTC] (XEN) Panic on CPU 0:
>> [22:47:04 UTC] (XEN) FATAL TRAP: vector = 2 (nmi)
>> [22:47:04 UTC] (XEN) [error_code=0000] , IN INTERRUPT CONTEXT
>> [22:47:04 UTC] (XEN) ****************************************
>> [22:47:04 UTC] (XEN)
>> [22:47:04 UTC] (XEN) Reboot in five seconds...
>> [22:47:05 UTC] (XEN) DMAR_IQA_REG = 839b34002
>> [22:47:05 UTC] (XEN) DMAR_IQH_REG = 0
>> [22:47:05 UTC] (XEN) DMAR_IQT_REG = 26b0
>> [22:47:05 UTC] (XEN)
>> [22:47:05 UTC] (XEN) ****************************************
>> [22:47:05 UTC] (XEN) Panic on CPU 0:
>> [22:47:05 UTC] (XEN) queue invalidate wait descriptor was not executed
>> [22:47:05 UTC] (XEN) ****************************************
>> [22:47:05 UTC] (XEN)
>> [22:47:05 UTC] (XEN) Reboot in five seconds...
>> [23:09:54 UTC]  The server is not powered on.  The Virtual Serial Port is not
>> available.
>>
>> The problem is quite difficult to reproduce (seen twice, unable to reproduce
>> manually), but inspection of kexec_crash() path shows a certain deadlock
>> because of one_cpu_only().
>>
>>
>> This patch replaces the use of one_cpu_only() with a recursive spinlock which
>> provides exclusion between different CPUs racing into kexec_crash(), but
>> prevents deadlock if the same CPU reenters kexec_crash() again.
> Does kexec_crash() not mind being reentered in this way?
>
>  -- Keir

Not really.

I first experimented with seeing whether it was possible to detect
reentrance and back out, but there certainly cases with the reentrant
NMI/MCE where you can loose the state required to get back to the outer
instance of kexec_crash.  Furthermore, in the case of a crash its
possible memory corruption has occurred, meaning faults are more likely,
leading to reentrance back onto the kexec_crash() path.  Also, there are
BUG()s and ASSERTS()s in the current kexec_crash() path.

Therefore, the sensible approach is to make kexec_crash() safe to be
reentered, even if we do make all attempts to minimise the chances of
reentrance.

Looking through the entire kexec_crash() path, console_start_sync()
could deadlock on the serial tx_lock if a fault occurs midway through
the function, and acpi_dmar_reinstate() will leave the DMAR table in a
state which fails the checksum.  kexec_crash_save_cpu() might not
completely write the crash notes, but there are no other obvious problems.

I can probably fix the acpi_dmar_reinstate() and kexec_crash_save_cpu()
issues without too much problem and will look into those in due course,
but my next task is to see whether there was a programatic reason as to
why the wait descriptor was not executed successfully.

Given that on the crash path something has already gone wrong, I would
argue this patch on the merit of removing a deadlock without making the
situation any worse.  I think it is impossible to have a kexec_crash()
path which is safely reentrant and guarantee to work without issue;
after all, if we reenter too many times because of faults we will likely
triple fault.

~Andrew

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> diff -r 996fbd7657bb -r a559ec5225fc xen/common/kexec.c
>> --- a/xen/common/kexec.c
>> +++ b/xen/common/kexec.c
>> @@ -234,13 +234,6 @@ void __init set_kexec_crash_area_size(u6
>>      }
>>  }
>>  
>> -static void one_cpu_only(void)
>> -{
>> -    /* Only allow the first cpu to continue - force other cpus to spin */
>> -    if ( test_and_set_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags) )
>> -        for ( ; ; ) ;
>> -}
>> -
>>  /* Save the registers in the per-cpu crash note buffer. */
>>  void kexec_crash_save_cpu(void)
>>  {
>> @@ -294,19 +287,25 @@ static void kexec_common_shutdown(void)
>>      watchdog_disable();
>>      console_start_sync();
>>      spin_debug_disable();
>> -    one_cpu_only();
>>      acpi_dmar_reinstate();
>>  }
>>  
>>  void kexec_crash(void)
>>  {
>> +    static DEFINE_SPINLOCK(crash_lock);
>>      int pos;
>>  
>>      pos = (test_bit(KEXEC_FLAG_CRASH_POS, &kexec_flags) != 0);
>>      if ( !test_bit(KEXEC_IMAGE_CRASH_BASE + pos, &kexec_flags) )
>>          return;
>>  
>> +    /* In a cascade failure, it is possible to re-enter kexec_crash on the
>> same
>> +     * CPU.  This spinlock is deliberately never unlocked, to allow
>> reentrance
>> +     * on the same CPU, but provides excusion from different CPUs racing. */
>> +    spin_lock_recursive(&crash_lock);
>> +
>>      kexecing = TRUE;
>> +    set_bit(KEXEC_FLAG_IN_PROGRESS, &kexec_flags);
>>  
>>      kexec_common_shutdown();
>>      kexec_crash_save_cpu();
>

Re: [PATCH] xen/kexec: Prevent deadlock because of one_cpu_only()

[Jan Beulich]

>>> On 06.04.13 at 17:03, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> I can probably fix the acpi_dmar_reinstate() and kexec_crash_save_cpu()
> issues without too much problem and will look into those in due course,
> but my next task is to see whether there was a programatic reason as to
> why the wait descriptor was not executed successfully.

Was that perhaps still without the Intel chipset interrupt remapping
errata workaround? These errata can - afaict - have exactly that
panic as a symptom.

Jan

Re: [PATCH] xen/kexec: Prevent deadlock because of one_cpu_only()

[Andrew Cooper]

On 08/04/13 08:48, Jan Beulich wrote:
>>>> On 06.04.13 at 17:03, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> I can probably fix the acpi_dmar_reinstate() and kexec_crash_save_cpu()
>> issues without too much problem and will look into those in due course,
>> but my next task is to see whether there was a programatic reason as to
>> why the wait descriptor was not executed successfully.
> Was that perhaps still without the Intel chipset interrupt remapping
> errata workaround? These errata can - afaict - have exactly that
> panic as a symptom.
>
> Jan
>

This is not from an affected system; It is Sandy-Bridge EP (And we have
the workaround in place anyway), although we too saw these symptoms for
the errata-affected systems.

I am double checking the errata docs, given the similarity.

~Andrew