[0/4] x86: Don't invoke asm_exc_nmi() on the kernel stack

Message

Lai Jiangshan April 26, 2021, 11:09 p.m. UTC

From: Lai Jiangshan <[e-mail]>

In VMX, the NMI handler needs to be invoked after NMI VM-Exit.

Before the commit 1a5488ef0dcf6 ("KVM: VMX: Invoke NMI handler via
indirect call instead of INTn"), the work is done by INTn ("int $2").

But INTn microcode is relatively expensive, so the commit reworked
NMI VM-Exit handling to invoke the kernel handler by function call.
And INTn doesn't set the NMI blocked flag required by the linux kernel
NMI entry.  So moving away from INTn are very reasonable.

Yet some details were missed.  After the said commit applied, the NMI
entry pointer is fetched from the IDT table and called from the kernel
stack.  But the NMI entry pointer installed on the IDT table is
asm_exc_nmi() which expects to be invoked on the IST stack by the ISA.
And it relies on the "NMI executing" variable on the IST stack to work
correctly.  When it is unexpectedly called from the kernel stack, the
RSP-located "NMI executing" variable is also on the kernel stack and
is "uninitialized" and can cause the NMI entry to run in the wrong way.

During fixing the problem for KVM, I found that there might be the same
problem for early booting stage where the IST is not set up. asm_exc_nmi()
is not allowed to be used in this stage for the same reason about
the RSP-located "NMI executing" variable.

For both cases, we should use asm_noist_exc_nmi() which is introduced
in the patch 1 via renaming from an existing asm_xenpv_exc_nmi() and
which is safe on the kernel stack.

https://lore.kernel.org/lkml/[e-mail]/

Cc: Thomas Gleixner <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Cc: Sean Christopherson <[e-mail]>
Cc: Steven Rostedt <[e-mail]>
Cc: Andi Kleen <[e-mail]>
Cc: Andy Lutomirski <[e-mail]>
Cc: Vitaly Kuznetsov <[e-mail]>
Cc: Wanpeng Li <[e-mail]>
Cc: Jim Mattson <[e-mail]>
Cc: Joerg Roedel <[e-mail]>
Cc: [e-mail]
Cc: Josh Poimboeuf <[e-mail]>
Cc: Uros Bizjak <[e-mail]>
Cc: Maxim Levitsky <[e-mail]>
Signed-off-by: Lai Jiangshan <[e-mail]>

Lai Jiangshan (4):
  x86/xen/entry: Rename xenpv_exc_nmi to noist_exc_nmi
  x86/entry: Use asm_noist_exc_nmi() for NMI in early booting stage
  KVM/VMX: Invoke NMI non-IST entry instead of IST entry
  KVM/VMX: fold handle_interrupt_nmi_irqoff() into its solo caller

 arch/x86/include/asm/idtentry.h |  4 +---
 arch/x86/kernel/idt.c           |  8 +++++++-
 arch/x86/kernel/nmi.c           | 12 ++++++++++++
 arch/x86/kvm/vmx/vmx.c          | 27 ++++++++++++++-------------
 arch/x86/xen/enlighten_pv.c     |  9 +++------
 arch/x86/xen/xen-asm.S          |  2 +-
 6 files changed, 38 insertions(+), 24 deletions(-)

Comments

Paolo Bonzini April 30, 2021, 7:14 a.m. UTC | #1

On 27/04/21 01:09, Lai Jiangshan wrote:
> From: Lai Jiangshan <[e-mail]>
> 
> In VMX, the NMI handler needs to be invoked after NMI VM-Exit.
> 
> Before the commit 1a5488ef0dcf6 ("KVM: VMX: Invoke NMI handler via
> indirect call instead of INTn"), the work is done by INTn ("int $2").
> 
> But INTn microcode is relatively expensive, so the commit reworked
> NMI VM-Exit handling to invoke the kernel handler by function call.
> And INTn doesn't set the NMI blocked flag required by the linux kernel
> NMI entry.  So moving away from INTn are very reasonable.
> 
> Yet some details were missed.  After the said commit applied, the NMI
> entry pointer is fetched from the IDT table and called from the kernel
> stack.  But the NMI entry pointer installed on the IDT table is
> asm_exc_nmi() which expects to be invoked on the IST stack by the ISA.
> And it relies on the "NMI executing" variable on the IST stack to work
> correctly.  When it is unexpectedly called from the kernel stack, the
> RSP-located "NMI executing" variable is also on the kernel stack and
> is "uninitialized" and can cause the NMI entry to run in the wrong way.
> 
> During fixing the problem for KVM, I found that there might be the same
> problem for early booting stage where the IST is not set up. asm_exc_nmi()
> is not allowed to be used in this stage for the same reason about
> the RSP-located "NMI executing" variable.
> 
> For both cases, we should use asm_noist_exc_nmi() which is introduced
> in the patch 1 via renaming from an existing asm_xenpv_exc_nmi() and
> which is safe on the kernel stack.
> 
> https://lore.kernel.org/lkml/[e-mail]/

For the KVM part,

Acked-by: Paolo Bonzini <[e-mail]>

Thanks,

Paolo

> Cc: Thomas Gleixner <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: Sean Christopherson <[e-mail]>
> Cc: Steven Rostedt <[e-mail]>
> Cc: Andi Kleen <[e-mail]>
> Cc: Andy Lutomirski <[e-mail]>
> Cc: Vitaly Kuznetsov <[e-mail]>
> Cc: Wanpeng Li <[e-mail]>
> Cc: Jim Mattson <[e-mail]>
> Cc: Joerg Roedel <[e-mail]>
> Cc: [e-mail]
> Cc: Josh Poimboeuf <[e-mail]>
> Cc: Uros Bizjak <[e-mail]>
> Cc: Maxim Levitsky <[e-mail]>
> Signed-off-by: Lai Jiangshan <[e-mail]>
> 
> Lai Jiangshan (4):
>    x86/xen/entry: Rename xenpv_exc_nmi to noist_exc_nmi
>    x86/entry: Use asm_noist_exc_nmi() for NMI in early booting stage
>    KVM/VMX: Invoke NMI non-IST entry instead of IST entry
>    KVM/VMX: fold handle_interrupt_nmi_irqoff() into its solo caller
> 
>   arch/x86/include/asm/idtentry.h |  4 +---
>   arch/x86/kernel/idt.c           |  8 +++++++-
>   arch/x86/kernel/nmi.c           | 12 ++++++++++++
>   arch/x86/kvm/vmx/vmx.c          | 27 ++++++++++++++-------------
>   arch/x86/xen/enlighten_pv.c     |  9 +++------
>   arch/x86/xen/xen-asm.S          |  2 +-
>   6 files changed, 38 insertions(+), 24 deletions(-)
>

Thomas Gleixner May 3, 2021, 2:36 p.m. UTC | #2

On Fri, Apr 30 2021 at 09:14, Paolo Bonzini wrote:
> On 27/04/21 01:09, Lai Jiangshan wrote:
>
> Acked-by: Paolo Bonzini <[e-mail]>

Thanks Paolo. I'm working through it now...