[PATCH] x86/boot: Make alternative patching NMI-safe

[PATCH] x86/boot: Make alternative patching NMI-safe

[Andrew Cooper]

During patching, there is a very slim risk that an NMI or MCE interrupt in the
middle of altering the code in the NMI/MCE paths, in which case bad things
will happen.

The NMI risk can be eliminated by running the patching loop in NMI context, at
which point the CPU will defer further NMIs until patching is complete.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/alternative.c | 61 +++++++++++++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index ee18e6c..521f896 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -16,6 +16,7 @@
  */
 
 #include <xen/types.h>
+#include <asm/apic.h>
 #include <asm/processor.h>
 #include <asm/alternative.h>
 #include <xen/init.h>
@@ -82,11 +83,6 @@ static const unsigned char * const p6_nops[ASM_NOP_MAX+1] init_or_livepatch_cons
 
 static const unsigned char * const *ideal_nops init_or_livepatch_data = p6_nops;
 
-static int __init mask_nmi_callback(const struct cpu_user_regs *regs, int cpu)
-{
-    return 1;
-}
-
 static void __init arch_init_ideal_nops(void)
 {
     switch ( boot_cpu_data.x86_vendor )
@@ -203,24 +199,50 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
 }
 
 /*
+ * At boot time, we patch alternatives in NMI context.  This means that the
+ * active NMI-shadow will defer any further NMIs, removing the slim race
+ * condition where an NMI hits while we are midway though patching some
+ * instructions in the NMI path.
+ */
+static int __init nmi_apply_alternatives(const struct cpu_user_regs *regs,
+                                         int cpu)
+{
+    static bool __initdata once;
+
+    /*
+     * More than one NMI may occur between the two set_nmi_callback() below.
+     * We only need to apply alternatives once.
+     */
+    if ( !once )
+    {
+        unsigned long cr0;
+
+        once = true;
+
+        cr0 = read_cr0();
+
+        /* Disable WP to allow patching read-only pages. */
+        write_cr0(cr0 & ~X86_CR0_WP);
+
+        apply_alternatives(__alt_instructions, __alt_instructions_end);
+
+        write_cr0(cr0);
+    }
+
+    return 1;
+}
+
+/*
  * This routine is called with local interrupt disabled and used during
  * bootup.
  */
 void __init alternative_instructions(void)
 {
     nmi_callback_t *saved_nmi_callback;
-    unsigned long cr0 = read_cr0();
 
     arch_init_ideal_nops();
 
     /*
-     * The patching is not fully atomic, so try to avoid local interruptions
-     * that might execute the to be patched code.
-     * Other CPUs are not running.
-     */
-    saved_nmi_callback = set_nmi_callback(mask_nmi_callback);
-
-    /*
      * Don't stop machine check exceptions while patching.
      * MCEs only happen when something got corrupted and in this
      * case we must do something about the corruption.
@@ -232,13 +254,14 @@ void __init alternative_instructions(void)
      */
     ASSERT(!local_irq_is_enabled());
 
-    /* Disable WP to allow application of alternatives to read-only pages. */
-    write_cr0(cr0 & ~X86_CR0_WP);
-
-    apply_alternatives(__alt_instructions, __alt_instructions_end);
+    /*
+     * As soon as the callback is set up, the next NMI will trigger patching,
+     * even an NMI ahead of our explicit self-NMI.
+     */
+    saved_nmi_callback = set_nmi_callback(nmi_apply_alternatives);
 
-    /* Reinstate WP. */
-    write_cr0(cr0);
+    /* Send ourselves an NMI to trigger the callback. */
+    self_nmi();
 
     set_nmi_callback(saved_nmi_callback);
 }
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/boot: Make alternative patching NMI-safe

[Jan Beulich]

>>> On 05.02.18 at 11:24, <andrew.cooper3@citrix.com> wrote:
> During patching, there is a very slim risk that an NMI or MCE interrupt in the
> middle of altering the code in the NMI/MCE paths, in which case bad things
> will happen.
> 
> The NMI risk can be eliminated by running the patching loop in NMI context, at
> which point the CPU will defer further NMIs until patching is complete.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

So you continue to think that the risk of hitting an #MC here is
acceptable, despite there being a solution to the problem. To be
honest, I find this a little strange. (I do agree that there's no
good solution to the similar live patching problem.)

> @@ -232,13 +254,14 @@ void __init alternative_instructions(void)
>       */
>      ASSERT(!local_irq_is_enabled());
>  
> -    /* Disable WP to allow application of alternatives to read-only pages. */
> -    write_cr0(cr0 & ~X86_CR0_WP);
> -
> -    apply_alternatives(__alt_instructions, __alt_instructions_end);
> +    /*
> +     * As soon as the callback is set up, the next NMI will trigger patching,
> +     * even an NMI ahead of our explicit self-NMI.
> +     */
> +    saved_nmi_callback = set_nmi_callback(nmi_apply_alternatives);
>  
> -    /* Reinstate WP. */
> -    write_cr0(cr0);
> +    /* Send ourselves an NMI to trigger the callback. */
> +    self_nmi();
>  
>      set_nmi_callback(saved_nmi_callback);
>  }

Hmm, you restore the old callback and return without having waited
for the patching to actually occur? Or are you implying that the
delivery of the NMI is guaranteed to be fully synchronous on all
hardware? I'm not aware of the LAPIC spec saying anything like this.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/boot: Make alternative patching NMI-safe

[Andrew Cooper]

On 05/02/18 14:09, Jan Beulich wrote:
>>>> On 05.02.18 at 11:24, <andrew.cooper3@citrix.com> wrote:
>> During patching, there is a very slim risk that an NMI or MCE interrupt in the
>> middle of altering the code in the NMI/MCE paths, in which case bad things
>> will happen.
>>
>> The NMI risk can be eliminated by running the patching loop in NMI context, at
>> which point the CPU will defer further NMIs until patching is complete.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> So you continue to think that the risk of hitting an #MC here is
> acceptable, despite there being a solution to the problem. To be
> honest, I find this a little strange. (I do agree that there's no
> good solution to the similar live patching problem.)

The risk is already sufficiently tiny that in 3 years, it hasn't been
observed, nor do I think it is likely to be observed in the future.  At
this point on boot, there is nothing interesting set up, which further
reduces the risk of an MCE.

Furthermore, whether or not Xen survives the MCE (and don't believe I've
ever seen Xen successfully recover from an MCE), the hardware is faulty
and needs replacing (modulo cosmic rays, but the chances of those really
are astronomical).

Irrespective of that, there is no way I'm aware of for generating MCEs
on demand, and therefore, no way of testing the logic.  For that reason
alone, I don't think it is wise to be taking complicated invasive logic.

>
>> @@ -232,13 +254,14 @@ void __init alternative_instructions(void)
>>       */
>>      ASSERT(!local_irq_is_enabled());
>>  
>> -    /* Disable WP to allow application of alternatives to read-only pages. */
>> -    write_cr0(cr0 & ~X86_CR0_WP);
>> -
>> -    apply_alternatives(__alt_instructions, __alt_instructions_end);
>> +    /*
>> +     * As soon as the callback is set up, the next NMI will trigger patching,
>> +     * even an NMI ahead of our explicit self-NMI.
>> +     */
>> +    saved_nmi_callback = set_nmi_callback(nmi_apply_alternatives);
>>  
>> -    /* Reinstate WP. */
>> -    write_cr0(cr0);
>> +    /* Send ourselves an NMI to trigger the callback. */
>> +    self_nmi();
>>  
>>      set_nmi_callback(saved_nmi_callback);
>>  }
> Hmm, you restore the old callback and return without having waited
> for the patching to actually occur? Or are you implying that the
> delivery of the NMI is guaranteed to be fully synchronous on all
> hardware? I'm not aware of the LAPIC spec saying anything like this.

I hadn't even considered that.  In practice, NMIs always appear
synchronously following the ICR write.

I expect this probably wasn't the case for off-chip APICs.

In this case, I can move the once boolean to being at file scope and
check for that.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/boot: Make alternative patching NMI-safe

[Jan Beulich]

>>> On 05.02.18 at 16:16, <andrew.cooper3@citrix.com> wrote:
> On 05/02/18 14:09, Jan Beulich wrote:
>>>>> On 05.02.18 at 11:24, <andrew.cooper3@citrix.com> wrote:
>>> During patching, there is a very slim risk that an NMI or MCE interrupt in the
>>> middle of altering the code in the NMI/MCE paths, in which case bad things
>>> will happen.
>>>
>>> The NMI risk can be eliminated by running the patching loop in NMI context, at
>>> which point the CPU will defer further NMIs until patching is complete.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> So you continue to think that the risk of hitting an #MC here is
>> acceptable, despite there being a solution to the problem. To be
>> honest, I find this a little strange. (I do agree that there's no
>> good solution to the similar live patching problem.)
> 
> The risk is already sufficiently tiny that in 3 years, it hasn't been
> observed, nor do I think it is likely to be observed in the future.  At
> this point on boot, there is nothing interesting set up, which further
> reduces the risk of an MCE.
> 
> Furthermore, whether or not Xen survives the MCE (and don't believe I've
> ever seen Xen successfully recover from an MCE), the hardware is faulty
> and needs replacing (modulo cosmic rays, but the chances of those really
> are astronomical).
> 
> Irrespective of that, there is no way I'm aware of for generating MCEs
> on demand, and therefore, no way of testing the logic.  For that reason
> alone, I don't think it is wise to be taking complicated invasive logic.

Your first attempt, minus the live patching parts, wasn't all that
invasive, and the code would have been the same for NMI and
#MC (so the testing argument also doesn't apply, as NMIs are
not uncommon).

>>> @@ -232,13 +254,14 @@ void __init alternative_instructions(void)
>>>       */
>>>      ASSERT(!local_irq_is_enabled());
>>>  
>>> -    /* Disable WP to allow application of alternatives to read-only pages. */
>>> -    write_cr0(cr0 & ~X86_CR0_WP);
>>> -
>>> -    apply_alternatives(__alt_instructions, __alt_instructions_end);
>>> +    /*
>>> +     * As soon as the callback is set up, the next NMI will trigger patching,
>>> +     * even an NMI ahead of our explicit self-NMI.
>>> +     */
>>> +    saved_nmi_callback = set_nmi_callback(nmi_apply_alternatives);
>>>  
>>> -    /* Reinstate WP. */
>>> -    write_cr0(cr0);
>>> +    /* Send ourselves an NMI to trigger the callback. */
>>> +    self_nmi();
>>>  
>>>      set_nmi_callback(saved_nmi_callback);
>>>  }
>> Hmm, you restore the old callback and return without having waited
>> for the patching to actually occur? Or are you implying that the
>> delivery of the NMI is guaranteed to be fully synchronous on all
>> hardware? I'm not aware of the LAPIC spec saying anything like this.
> 
> I hadn't even considered that.  In practice, NMIs always appear
> synchronously following the ICR write.
> 
> I expect this probably wasn't the case for off-chip APICs.
> 
> In this case, I can move the once boolean to being at file scope and
> check for that.

Sounds reasonable, if that's the route to go in the first place.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/boot: Make alternative patching NMI-safe

[Andrew Cooper]

On 05/02/18 16:20, Jan Beulich wrote:
>>>> On 05.02.18 at 16:16, <andrew.cooper3@citrix.com> wrote:
>> On 05/02/18 14:09, Jan Beulich wrote:
>>>>>> On 05.02.18 at 11:24, <andrew.cooper3@citrix.com> wrote:
>>>> During patching, there is a very slim risk that an NMI or MCE interrupt in the
>>>> middle of altering the code in the NMI/MCE paths, in which case bad things
>>>> will happen.
>>>>
>>>> The NMI risk can be eliminated by running the patching loop in NMI context, at
>>>> which point the CPU will defer further NMIs until patching is complete.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> So you continue to think that the risk of hitting an #MC here is
>>> acceptable, despite there being a solution to the problem. To be
>>> honest, I find this a little strange. (I do agree that there's no
>>> good solution to the similar live patching problem.)
>> The risk is already sufficiently tiny that in 3 years, it hasn't been
>> observed, nor do I think it is likely to be observed in the future.  At
>> this point on boot, there is nothing interesting set up, which further
>> reduces the risk of an MCE.
>>
>> Furthermore, whether or not Xen survives the MCE (and don't believe I've
>> ever seen Xen successfully recover from an MCE), the hardware is faulty
>> and needs replacing (modulo cosmic rays, but the chances of those really
>> are astronomical).
>>
>> Irrespective of that, there is no way I'm aware of for generating MCEs
>> on demand, and therefore, no way of testing the logic.  For that reason
>> alone, I don't think it is wise to be taking complicated invasive logic.
> Your first attempt, minus the live patching parts, wasn't all that
> invasive, and the code would have been the same for NMI and
> #MC (so the testing argument also doesn't apply, as NMIs are
> not uncommon).

But as you correctly pointed out, it was a very long way from being
complete.  We currently have no idea whether we are in NMI context, so
arranging not to not execute an iret is hard.

In some copious free time, I should try to port Linux's re-entrant NMI
logic again.  I tried once before and that all became unstuck because of
intermixed NMI and MCEs.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/boot: Make alternative patching NMI-safe

[Jan Beulich]

>>> On 05.02.18 at 19:04, <andrew.cooper3@citrix.com> wrote:
> But as you correctly pointed out, it was a very long way from being
> complete.  We currently have no idea whether we are in NMI context, so
> arranging not to not execute an iret is hard.

As long as we don't mean to patch extremely early or extremely
late parts of the handler, why couldn't the NMI handler set a flag
(or increment some sort of counter; perhaps in struct cpu_info),
which the exit path inspects (and clears under the appropriate
conditions)?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/boot: Make alternative patching NMI-safe

[Konrad Rzeszutek Wilk]

On Mon, Feb 05, 2018 at 10:24:58AM +0000, Andrew Cooper wrote:
> During patching, there is a very slim risk that an NMI or MCE interrupt in the
> middle of altering the code in the NMI/MCE paths, in which case bad things
> will happen.
> 
> The NMI risk can be eliminated by running the patching loop in NMI context, at
> which point the CPU will defer further NMIs until patching is complete.

Oh, that is a very simple fix.

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> ---
>  xen/arch/x86/alternative.c | 61 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 42 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index ee18e6c..521f896 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -16,6 +16,7 @@
>   */
>  
>  #include <xen/types.h>
> +#include <asm/apic.h>
>  #include <asm/processor.h>
>  #include <asm/alternative.h>
>  #include <xen/init.h>
> @@ -82,11 +83,6 @@ static const unsigned char * const p6_nops[ASM_NOP_MAX+1] init_or_livepatch_cons
>  
>  static const unsigned char * const *ideal_nops init_or_livepatch_data = p6_nops;
>  
> -static int __init mask_nmi_callback(const struct cpu_user_regs *regs, int cpu)
> -{
> -    return 1;
> -}
> -
>  static void __init arch_init_ideal_nops(void)
>  {
>      switch ( boot_cpu_data.x86_vendor )
> @@ -203,24 +199,50 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
>  }
>  
>  /*
> + * At boot time, we patch alternatives in NMI context.  This means that the
> + * active NMI-shadow will defer any further NMIs, removing the slim race
> + * condition where an NMI hits while we are midway though patching some
> + * instructions in the NMI path.
> + */
> +static int __init nmi_apply_alternatives(const struct cpu_user_regs *regs,
> +                                         int cpu)
> +{
> +    static bool __initdata once;
> +
> +    /*
> +     * More than one NMI may occur between the two set_nmi_callback() below.
> +     * We only need to apply alternatives once.
> +     */
> +    if ( !once )
> +    {
> +        unsigned long cr0;
> +
> +        once = true;
> +
> +        cr0 = read_cr0();
> +
> +        /* Disable WP to allow patching read-only pages. */
> +        write_cr0(cr0 & ~X86_CR0_WP);
> +
> +        apply_alternatives(__alt_instructions, __alt_instructions_end);
> +
> +        write_cr0(cr0);
> +    }
> +
> +    return 1;
> +}
> +
> +/*
>   * This routine is called with local interrupt disabled and used during
>   * bootup.
>   */
>  void __init alternative_instructions(void)
>  {
>      nmi_callback_t *saved_nmi_callback;
> -    unsigned long cr0 = read_cr0();
>  
>      arch_init_ideal_nops();
>  
>      /*
> -     * The patching is not fully atomic, so try to avoid local interruptions
> -     * that might execute the to be patched code.
> -     * Other CPUs are not running.
> -     */
> -    saved_nmi_callback = set_nmi_callback(mask_nmi_callback);
> -
> -    /*
>       * Don't stop machine check exceptions while patching.
>       * MCEs only happen when something got corrupted and in this
>       * case we must do something about the corruption.
> @@ -232,13 +254,14 @@ void __init alternative_instructions(void)
>       */
>      ASSERT(!local_irq_is_enabled());
>  
> -    /* Disable WP to allow application of alternatives to read-only pages. */
> -    write_cr0(cr0 & ~X86_CR0_WP);
> -
> -    apply_alternatives(__alt_instructions, __alt_instructions_end);
> +    /*
> +     * As soon as the callback is set up, the next NMI will trigger patching,
> +     * even an NMI ahead of our explicit self-NMI.
> +     */
> +    saved_nmi_callback = set_nmi_callback(nmi_apply_alternatives);
>  
> -    /* Reinstate WP. */
> -    write_cr0(cr0);
> +    /* Send ourselves an NMI to trigger the callback. */
> +    self_nmi();
>  
>      set_nmi_callback(saved_nmi_callback);
>  }
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel