[PATCH] x86/nmi: Fix shootdown of pcpus running in VMX non-root mode

[PATCH] x86/nmi: Fix shootdown of pcpus running in VMX non-root mode

[Andrew Cooper]

In the case of a crash, nmi_shootdown_cpus() patches nmi_crash() into the IDT
of each processor, such that the next NMI it receives will force it into the
crash path.

c/s 7dd3b06ff "vmx: fix handling of NMI VMEXIT" fixed one issue but
inadvertently introduced another.  The original use of self_nmi() would follow
vector #2, but a direct call to do_nmi() does not.

Introduce a function pointer which should be used in preference to direct
do_nmi() calls, which is updated on the crash path to point at do_nmi_crash()

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>

---

This patch very certainly functions correctly (it is in active use now in a
customer escalation), but I was wondering how paranoid we should be about
interleaved reads/writes and whether an atomic write would be better?
Performance is not a issue at all but in a crash senario we don't want to be
taking any chances with correctness.
---
 xen/arch/x86/crash.c            |    4 +++-
 xen/arch/x86/hvm/vmx/vmx.c      |    2 +-
 xen/arch/x86/traps.c            |    2 ++
 xen/include/asm-x86/processor.h |    3 ++-
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index c0b83df..0822a68 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -36,7 +36,7 @@ static unsigned int crashing_cpu;
 static DEFINE_PER_CPU_READ_MOSTLY(bool_t, crash_save_done);
 
 /* This becomes the NMI handler for non-crashing CPUs, when Xen is crashing. */
-void do_nmi_crash(struct cpu_user_regs *regs)
+void do_nmi_crash(const struct cpu_user_regs *regs)
 {
     int cpu = smp_processor_id();
 
@@ -160,6 +160,8 @@ static void nmi_shootdown_cpus(void)
         }
     }
 
+    nmi_handler = do_nmi_crash;
+
     /* Ensure the new callback function is set before sending out the NMI. */
     wmb();
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 88b7821..67bdcb4 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2701,7 +2701,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
              && ((intr_info & INTR_INFO_INTR_TYPE_MASK) ==
                  (X86_EVENTTYPE_NMI << 8)) )
         {
-            do_nmi(regs);
+            nmi_handler(regs);
             enable_nmis();
         }
         break;
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index f5516dc..890e22a 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -105,6 +105,8 @@ idt_entry_t *idt_tables[NR_CPUS] __read_mostly;
 void (*ioemul_handle_quirk)(
     u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs);
 
+void (*nmi_handler)(const struct cpu_user_regs *regs) __read_mostly = do_nmi;
+
 static int debug_stack_lines = 20;
 integer_param("debug_stack_lines", debug_stack_lines);
 
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 20eade6..70c9abc 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -522,9 +522,10 @@ DECLARE_TRAP_HANDLER(alignment_check);
 #undef DECLARE_TRAP_HANDLER_CONST
 #undef DECLARE_TRAP_HANDLER
 
+extern void (*nmi_handler)(const struct cpu_user_regs *regs);
 void trap_nop(void);
 void enable_nmis(void);
-void noreturn do_nmi_crash(struct cpu_user_regs *regs);
+void noreturn do_nmi_crash(const struct cpu_user_regs *regs);
 void do_reserved_trap(struct cpu_user_regs *regs);
 
 void syscall_enter(void);
-- 
1.7.10.4

Re: [PATCH] x86/nmi: Fix shootdown of pcpus running in VMX non-root mode

[Tim Deegan]

Hi,

At 11:25 +0000 on 09 Feb (1423477508), Andrew Cooper wrote:
> In the case of a crash, nmi_shootdown_cpus() patches nmi_crash() into the IDT
> of each processor, such that the next NMI it receives will force it into the
> crash path.
> 
> c/s 7dd3b06ff "vmx: fix handling of NMI VMEXIT" fixed one issue but
> inadvertently introduced another.  The original use of self_nmi() would follow
> vector #2, but a direct call to do_nmi() does not.
> 
> Introduce a function pointer which should be used in preference to direct
> do_nmi() calls, which is updated on the crash path to point at do_nmi_crash()
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> 
> ---
> 
> This patch very certainly functions correctly (it is in active use now in a
> customer escalation), but I was wondering how paranoid we should be about
> interleaved reads/writes and whether an atomic write would be better?
> Performance is not a issue at all but in a crash senario we don't want to be
> taking any chances with correctness.

Yes, atomic updates sound like a good idea.  Would it make sense to
add a _get_gate() or similar so the vmx path can read the actual IDT
rather than adding a _third_ place where we set what to do on NMI?

If not...

> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -522,9 +522,10 @@ DECLARE_TRAP_HANDLER(alignment_check);
>  #undef DECLARE_TRAP_HANDLER_CONST
>  #undef DECLARE_TRAP_HANDLER
>  
> +extern void (*nmi_handler)(const struct cpu_user_regs *regs);

...please add a comment here describing what's going on - in
particular that changing this variable doesn't change the handler for
all NMI paths (and that for most purposes set_nmi_callback() is
more suitable).

Cheers,

Tim.

Re: [PATCH] x86/nmi: Fix shootdown of pcpus running in VMX non-root mode

[Andrew Cooper]

On 09/02/15 11:43, Tim Deegan wrote:
> Hi,
>
> At 11:25 +0000 on 09 Feb (1423477508), Andrew Cooper wrote:
>> In the case of a crash, nmi_shootdown_cpus() patches nmi_crash() into the IDT
>> of each processor, such that the next NMI it receives will force it into the
>> crash path.
>>
>> c/s 7dd3b06ff "vmx: fix handling of NMI VMEXIT" fixed one issue but
>> inadvertently introduced another.  The original use of self_nmi() would follow
>> vector #2, but a direct call to do_nmi() does not.
>>
>> Introduce a function pointer which should be used in preference to direct
>> do_nmi() calls, which is updated on the crash path to point at do_nmi_crash()
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Tim Deegan <tim@xen.org>
>>
>> ---
>>
>> This patch very certainly functions correctly (it is in active use now in a
>> customer escalation), but I was wondering how paranoid we should be about
>> interleaved reads/writes and whether an atomic write would be better?
>> Performance is not a issue at all but in a crash senario we don't want to be
>> taking any chances with correctness.
> Yes, atomic updates sound like a good idea.  Would it make sense to
> add a _get_gate() or similar so the vmx path can read the actual IDT
> rather than adding a _third_ place where we set what to do on NMI?

A _get_gate() would return nmi() or nmi_crash() rather than do_nmi() or
do_nmi_crash().  The latter pair is needed as we are already executing
in C context rather than coming straight in from an interrupt.

>
> If not...
>
>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -522,9 +522,10 @@ DECLARE_TRAP_HANDLER(alignment_check);
>>  #undef DECLARE_TRAP_HANDLER_CONST
>>  #undef DECLARE_TRAP_HANDLER
>>  
>> +extern void (*nmi_handler)(const struct cpu_user_regs *regs);
> ...please add a comment here describing what's going on - in
> particular that changing this variable doesn't change the handler for
> all NMI paths (and that for most purposes set_nmi_callback() is
> more suitable).

Good point.  I will respin with some more atomicity and comments.

~Andrew

Re: [PATCH] x86/nmi: Fix shootdown of pcpus running in VMX non-root mode

[Jan Beulich]

>>> On 09.02.15 at 12:52, <andrew.cooper3@citrix.com> wrote:
> On 09/02/15 11:43, Tim Deegan wrote:
>> Hi,
>>
>> At 11:25 +0000 on 09 Feb (1423477508), Andrew Cooper wrote:
>>> In the case of a crash, nmi_shootdown_cpus() patches nmi_crash() into the 
> IDT
>>> of each processor, such that the next NMI it receives will force it into the
>>> crash path.
>>>
>>> c/s 7dd3b06ff "vmx: fix handling of NMI VMEXIT" fixed one issue but
>>> inadvertently introduced another.  The original use of self_nmi() would 
> follow
>>> vector #2, but a direct call to do_nmi() does not.
>>>
>>> Introduce a function pointer which should be used in preference to direct
>>> do_nmi() calls, which is updated on the crash path to point at 
> do_nmi_crash()
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Tim Deegan <tim@xen.org>
>>>
>>> ---
>>>
>>> This patch very certainly functions correctly (it is in active use now in a
>>> customer escalation), but I was wondering how paranoid we should be about
>>> interleaved reads/writes and whether an atomic write would be better?
>>> Performance is not a issue at all but in a crash senario we don't want to be
>>> taking any chances with correctness.
>> Yes, atomic updates sound like a good idea.  Would it make sense to
>> add a _get_gate() or similar so the vmx path can read the actual IDT
>> rather than adding a _third_ place where we set what to do on NMI?
> 
> A _get_gate() would return nmi() or nmi_crash() rather than do_nmi() or
> do_nmi_crash().  The latter pair is needed as we are already executing
> in C context rather than coming straight in from an interrupt.

So wouldn't it be possible to get rid of nmi_crash() and have
nmi() call *nmi_handler instead of don_nmi (and nmi_handler
would really just become an alias of exception_table[2]?

Jan

Re: [PATCH] x86/nmi: Fix shootdown of pcpus running in VMX non-root mode

[Andrew Cooper]

On 09/02/15 12:56, Jan Beulich wrote:
>>>> On 09.02.15 at 12:52, <andrew.cooper3@citrix.com> wrote:
>> On 09/02/15 11:43, Tim Deegan wrote:
>>> Hi,
>>>
>>> At 11:25 +0000 on 09 Feb (1423477508), Andrew Cooper wrote:
>>>> In the case of a crash, nmi_shootdown_cpus() patches nmi_crash() into the 
>> IDT
>>>> of each processor, such that the next NMI it receives will force it into the
>>>> crash path.
>>>>
>>>> c/s 7dd3b06ff "vmx: fix handling of NMI VMEXIT" fixed one issue but
>>>> inadvertently introduced another.  The original use of self_nmi() would 
>> follow
>>>> vector #2, but a direct call to do_nmi() does not.
>>>>
>>>> Introduce a function pointer which should be used in preference to direct
>>>> do_nmi() calls, which is updated on the crash path to point at 
>> do_nmi_crash()
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Tim Deegan <tim@xen.org>
>>>>
>>>> ---
>>>>
>>>> This patch very certainly functions correctly (it is in active use now in a
>>>> customer escalation), but I was wondering how paranoid we should be about
>>>> interleaved reads/writes and whether an atomic write would be better?
>>>> Performance is not a issue at all but in a crash senario we don't want to be
>>>> taking any chances with correctness.
>>> Yes, atomic updates sound like a good idea.  Would it make sense to
>>> add a _get_gate() or similar so the vmx path can read the actual IDT
>>> rather than adding a _third_ place where we set what to do on NMI?
>> A _get_gate() would return nmi() or nmi_crash() rather than do_nmi() or
>> do_nmi_crash().  The latter pair is needed as we are already executing
>> in C context rather than coming straight in from an interrupt.
> So wouldn't it be possible to get rid of nmi_crash() and have
> nmi() call *nmi_handler instead of don_nmi (and nmi_handler
> would really just become an alias of exception_table[2]?

nmi_crash() deliberately doesn't follow the handle_ist_exception path to
avoid possibly switching stack and possibly returning back to a guest. 
It has an emergency ud2 on the end to cover errors in do_nmi_crash().

Reusing exception_table[2] might be preferable to adding a new variable,
but it would involve moving exception_table[] from rodata to data, which
is rather less preferable overall.

~Andrew

Re: [PATCH] x86/nmi: Fix shootdown of pcpus running in VMX non-root mode

[Jan Beulich]

>>> On 09.02.15 at 14:13, <andrew.cooper3@citrix.com> wrote:
> On 09/02/15 12:56, Jan Beulich wrote:
>> So wouldn't it be possible to get rid of nmi_crash() and have
>> nmi() call *nmi_handler instead of don_nmi (and nmi_handler
>> would really just become an alias of exception_table[2]?
> 
> nmi_crash() deliberately doesn't follow the handle_ist_exception path to
> avoid possibly switching stack and possibly returning back to a guest. 

Is the stack switching really dangerous here? And if it is, wouldn't
checking a suitable flag to skip it still be better than the mess of
NMI entry points?

> It has an emergency ud2 on the end to cover errors in do_nmi_crash().

That could as well be a BUG() at the end of do_nmi_crash() itself.

> Reusing exception_table[2] might be preferable to adding a new variable,
> but it would involve moving exception_table[] from rodata to data, which
> is rather less preferable overall.

Considering the purpose you need the modification for, stealing an
unused (at that time) fixmap entry and creating a writeable alias
mapping would seem a reasonable alternative to me.

Jan

Re: [PATCH] x86/nmi: Fix shootdown of pcpus running in VMX non-root mode

[Andrew Cooper]

On 09/02/15 13:26, Jan Beulich wrote:
>>>> On 09.02.15 at 14:13, <andrew.cooper3@citrix.com> wrote:
>> On 09/02/15 12:56, Jan Beulich wrote:
>>> So wouldn't it be possible to get rid of nmi_crash() and have
>>> nmi() call *nmi_handler instead of don_nmi (and nmi_handler
>>> would really just become an alias of exception_table[2]?
>> nmi_crash() deliberately doesn't follow the handle_ist_exception path to
>> avoid possibly switching stack and possibly returning back to a guest. 
> Is the stack switching really dangerous here? And if it is, wouldn't
> checking a suitable flag to skip it still be better than the mess of
> NMI entry points?

I believe the argument was more along the lines of "less to go wrong"
than anything specifically unsafe.  One thing is certainly does do is
make the stack traces crystal clear between the crashing cpu and the
shot-down cpus. (It has been a while since I did this work, and the
commit message for 77ad1faa6b doesn't jog my memory.)

>
>> It has an emergency ud2 on the end to cover errors in do_nmi_crash().
> That could as well be a BUG() at the end of do_nmi_crash() itself.

That, along with the noreturn, would catch any coding errors in this
area.  The set of non-coding errors which could fail to be caught by a
BUG() but are caught by the ud2 is very small.

>
>> Reusing exception_table[2] might be preferable to adding a new variable,
>> but it would involve moving exception_table[] from rodata to data, which
>> is rather less preferable overall.
> Considering the purpose you need the modification for, stealing an
> unused (at that time) fixmap entry and creating a writeable alias
> mapping would seem a reasonable alternative to me.

It looks safe to borrow FIX_TBOOT_MAP_ADDRESS which is otherwise only
used in tboot_copy_memory() which is __init.

~Andrew