[PATCH v4 0/4] x86/crash: Fix double NMI shootdown bug

[PATCH v4 0/4] x86/crash: Fix double NMI shootdown bug

[Sean Christopherson]

Fix a double NMI shootdown bug found and debugged by Guilherme, who did all
the hard work.  NMI shootdown is a one-time thing; the handler leaves NMIs
blocked and enters halt.  At best, a second (or third...) shootdown is an
expensive nop, at worst it can hang the kernel and prevent kexec'ing into
a new kernel, e.g. prior to the hardening of register_nmi_handler(), a
double shootdown resulted in a double list_add(), which is fatal when running
with CONFIG_BUG_ON_DATA_CORRUPTION=y.

With the "right" kexec/kdump configuration, emergency_vmx_disable_all() can
be reached after kdump_nmi_shootdown_cpus() (currently the only two users
of nmi_shootdown_cpus()).

To fix, move the disabling of virtualization into crash_nmi_callback(),
remove emergency_vmx_disable_all()'s callback, and do a shootdown for
emergency_vmx_disable_all() if and only if a shootdown hasn't yet occurred.
The only thing emergency_vmx_disable_all() cares about is disabling VMX/SVM
(obviously), and since I can't envision a use case for an NMI shootdown that
doesn't want to disable virtualization, doing that in the core handler means
emergency_vmx_disable_all() only needs to ensure _a_ shootdown occurs, it
doesn't care when that shootdown happened or what callback may have run.

Patches 2-4 bring SVM on par with VMX.

v4:
  - Set GIF=1 prior to disabling SVM. [Andrew, Tom]
  - Name new helper cpu_emergency_disable_virtualization() instead of
    cpu_crash_disable_virtualization().
  - Add patch to handle SVM in the stop/reboot case.
  - Drop patch to fold __cpu_emergency_vmxoff() into cpu_emergency_vmxoff(),
    will be sent as part of a larger cleanup.

v3:
  - https://lore.kernel.org/all/20221114233441.3895891-1-seanjc@google.com
  - Re-collect Guilherme's Tested-by.
  - Tweak comment in patch 1 to reference STGI instead of CLGI.
  - Celebrate this series' half-birthday.

v2:
  - Use a NULL handler and crash_ipi_issued instead of a magic nop
    handler. [tglx]
  - Add comments to call out that modifying the existing handler
    once the NMI is sent may cause explosions.
  - Add a patch to cleanup cpu_emergency_vmxoff().
  - https://lore.kernel.org/all/20220518001647.1291448-1-seanjc@google.com

v1: https://lore.kernel.org/all/20220511234332.3654455-1-seanjc@google.com

Sean Christopherson (4):
  x86/crash: Disable virt in core NMI crash handler to avoid double
    shootdown
  x86/virt: Force GIF=1 prior to disabling SVM (for reboot flows)
  x86/reboot: Disable virtualization in an emergency if SVM is supported
  x86/reboot: Disable SVM, not just VMX, when stopping CPUs

 arch/x86/include/asm/reboot.h  |  2 +
 arch/x86/include/asm/virtext.h | 13 ++++-
 arch/x86/kernel/crash.c        | 17 +------
 arch/x86/kernel/reboot.c       | 88 ++++++++++++++++++++++++----------
 arch/x86/kernel/smp.c          |  6 +--
 5 files changed, 82 insertions(+), 44 deletions(-)


base-commit: d800169041c0e035160c8b81f30d4b7e8f8ef777
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH v4 1/4] x86/crash: Disable virt in core NMI crash handler to avoid double shootdown

[Sean Christopherson]

Disable virtualization in crash_nmi_callback() and rework the
emergency_vmx_disable_all() path to do an NMI shootdown if and only if a
shootdown has not already occurred.   NMI crash shootdown fundamentally
can't support multiple invocations as responding CPUs are deliberately
put into halt state without unblocking NMIs.  But, the emergency reboot
path doesn't have any work of its own, it simply cares about disabling
virtualization, i.e. so long as a shootdown occurred, emergency reboot
doesn't care who initiated the shootdown, or when.

If "crash_kexec_post_notifiers" is specified on the kernel command line,
panic() will invoke crash_smp_send_stop() and result in a second call to
nmi_shootdown_cpus() during native_machine_emergency_restart().

Invoke the callback _before_ disabling virtualization, as the current
VMCS needs to be cleared before doing VMXOFF.  Note, this results in a
subtle change in ordering between disabling virtualization and stopping
Intel PT on the responding CPUs.  While VMX and Intel PT do interact,
VMXOFF and writes to MSR_IA32_RTIT_CTL do not induce faults between one
another, which is all that matters when panicking.

Harden nmi_shootdown_cpus() against multiple invocations to try and
capture any such kernel bugs via a WARN instead of hanging the system
during a crash/dump, e.g. prior to the recent hardening of
register_nmi_handler(), re-registering the NMI handler would trigger a
double list_add() and hang the system if CONFIG_BUG_ON_DATA_CORRUPTION=y.

 list_add double add: new=ffffffff82220800, prev=ffffffff8221cfe8, next=ffffffff82220800.
 WARNING: CPU: 2 PID: 1319 at lib/list_debug.c:29 __list_add_valid+0x67/0x70
 Call Trace:
  __register_nmi_handler+0xcf/0x130
  nmi_shootdown_cpus+0x39/0x90
  native_machine_emergency_restart+0x1c9/0x1d0
  panic+0x237/0x29b

Extract the disabling logic to a common helper to deduplicate code, and
to prepare for doing the shootdown in the emergency reboot path if SVM
is supported.

Note, prior to commit ed72736183c4 ("x86/reboot: Force all cpus to exit
VMX root if VMX is supported"), nmi_shootdown_cpus() was subtly protected
against a second invocation by a cpu_vmx_enabled() check as the kdump
handler would disable VMX if it ran first.

Fixes: ed72736183c4 ("x86/reboot: Force all cpus to exit VMX root if VMX is supported)
Cc: stable@vger.kernel.org
Reported-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Link: https://lore.kernel.org/all/20220427224924.592546-2-gpiccoli@igalia.com
Signed-off-by: Sean Christopherson <seanjc@google.com>
Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 arch/x86/include/asm/reboot.h |  2 ++
 arch/x86/kernel/crash.c       | 17 +--------
 arch/x86/kernel/reboot.c      | 65 ++++++++++++++++++++++++++++-------
 3 files changed, 56 insertions(+), 28 deletions(-)

diff --git a/arch/x86/include/asm/reboot.h b/arch/x86/include/asm/reboot.h
index 04c17be9b5fd..bc5b4d788c08 100644
--- a/arch/x86/include/asm/reboot.h
+++ b/arch/x86/include/asm/reboot.h
@@ -25,6 +25,8 @@ void __noreturn machine_real_restart(unsigned int type);
 #define MRR_BIOS	0
 #define MRR_APM		1
 
+void cpu_emergency_disable_virtualization(void);
+
 typedef void (*nmi_shootdown_cb)(int, struct pt_regs*);
 void nmi_panic_self_stop(struct pt_regs *regs);
 void nmi_shootdown_cpus(nmi_shootdown_cb callback);
diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 9730c88530fc..6b8a6aae02e3 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -37,7 +37,6 @@
 #include <linux/kdebug.h>
 #include <asm/cpu.h>
 #include <asm/reboot.h>
-#include <asm/virtext.h>
 #include <asm/intel_pt.h>
 #include <asm/crash.h>
 #include <asm/cmdline.h>
@@ -81,15 +80,6 @@ static void kdump_nmi_callback(int cpu, struct pt_regs *regs)
 	 */
 	cpu_crash_vmclear_loaded_vmcss();
 
-	/* Disable VMX or SVM if needed.
-	 *
-	 * We need to disable virtualization on all CPUs.
-	 * Having VMX or SVM enabled on any CPU may break rebooting
-	 * after the kdump kernel has finished its task.
-	 */
-	cpu_emergency_vmxoff();
-	cpu_emergency_svm_disable();
-
 	/*
 	 * Disable Intel PT to stop its logging
 	 */
@@ -148,12 +138,7 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
 	 */
 	cpu_crash_vmclear_loaded_vmcss();
 
-	/* Booting kdump kernel with VMX or SVM enabled won't work,
-	 * because (among other limitations) we can't disable paging
-	 * with the virt flags.
-	 */
-	cpu_emergency_vmxoff();
-	cpu_emergency_svm_disable();
+	cpu_emergency_disable_virtualization();
 
 	/*
 	 * Disable Intel PT to stop its logging
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index c3636ea4aa71..374c14b3a9bf 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -528,10 +528,7 @@ static inline void kb_wait(void)
 	}
 }
 
-static void vmxoff_nmi(int cpu, struct pt_regs *regs)
-{
-	cpu_emergency_vmxoff();
-}
+static inline void nmi_shootdown_cpus_on_restart(void);
 
 /* Use NMIs as IPIs to tell all CPUs to disable virtualization */
 static void emergency_vmx_disable_all(void)
@@ -554,7 +551,7 @@ static void emergency_vmx_disable_all(void)
 		__cpu_emergency_vmxoff();
 
 		/* Halt and exit VMX root operation on the other CPUs. */
-		nmi_shootdown_cpus(vmxoff_nmi);
+		nmi_shootdown_cpus_on_restart();
 	}
 }
 
@@ -795,6 +792,17 @@ void machine_crash_shutdown(struct pt_regs *regs)
 /* This is the CPU performing the emergency shutdown work. */
 int crashing_cpu = -1;
 
+/*
+ * Disable virtualization, i.e. VMX or SVM, to ensure INIT is recognized during
+ * reboot.  VMX blocks INIT if the CPU is post-VMXON, and SVM blocks INIT if
+ * GIF=0, i.e. if the crash occurred between CLGI and STGI.
+ */
+void cpu_emergency_disable_virtualization(void)
+{
+	cpu_emergency_vmxoff();
+	cpu_emergency_svm_disable();
+}
+
 #if defined(CONFIG_SMP)
 
 static nmi_shootdown_cb shootdown_callback;
@@ -817,7 +825,14 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
 		return NMI_HANDLED;
 	local_irq_disable();
 
-	shootdown_callback(cpu, regs);
+	if (shootdown_callback)
+		shootdown_callback(cpu, regs);
+
+	/*
+	 * Prepare the CPU for reboot _after_ invoking the callback so that the
+	 * callback can safely use virtualization instructions, e.g. VMCLEAR.
+	 */
+	cpu_emergency_disable_virtualization();
 
 	atomic_dec(&waiting_for_crash_ipi);
 	/* Assume hlt works */
@@ -828,18 +843,32 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
 	return NMI_HANDLED;
 }
 
-/*
- * Halt all other CPUs, calling the specified function on each of them
+/**
+ * nmi_shootdown_cpus - Stop other CPUs via NMI
+ * @callback:	Optional callback to be invoked from the NMI handler
  *
- * This function can be used to halt all other CPUs on crash
- * or emergency reboot time. The function passed as parameter
- * will be called inside a NMI handler on all CPUs.
+ * The NMI handler on the remote CPUs invokes @callback, if not
+ * NULL, first and then disables virtualization to ensure that
+ * INIT is recognized during reboot.
+ *
+ * nmi_shootdown_cpus() can only be invoked once. After the first
+ * invocation all other CPUs are stuck in crash_nmi_callback() and
+ * cannot respond to a second NMI.
  */
 void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 {
 	unsigned long msecs;
+
 	local_irq_disable();
 
+	/*
+	 * Avoid certain doom if a shootdown already occurred; re-registering
+	 * the NMI handler will cause list corruption, modifying the callback
+	 * will do who knows what, etc...
+	 */
+	if (WARN_ON_ONCE(crash_ipi_issued))
+		return;
+
 	/* Make a note of crashing cpu. Will be used in NMI callback. */
 	crashing_cpu = safe_smp_processor_id();
 
@@ -867,7 +896,17 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 		msecs--;
 	}
 
-	/* Leave the nmi callback set */
+	/*
+	 * Leave the nmi callback set, shootdown is a one-time thing.  Clearing
+	 * the callback could result in a NULL pointer dereference if a CPU
+	 * (finally) responds after the timeout expires.
+	 */
+}
+
+static inline void nmi_shootdown_cpus_on_restart(void)
+{
+	if (!crash_ipi_issued)
+		nmi_shootdown_cpus(NULL);
 }
 
 /*
@@ -897,6 +936,8 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
 	/* No other CPUs to shoot down */
 }
 
+static inline void nmi_shootdown_cpus_on_restart(void) { }
+
 void run_crash_ipi_callback(struct pt_regs *regs)
 {
 }
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH v4 2/4] x86/virt: Force GIF=1 prior to disabling SVM (for reboot flows)

[Sean Christopherson]

Set GIF=1 prior to disabling SVM to ensure that INIT is recognized if the
kernel is disabling SVM in an emergency, e.g. if the kernel is about to
jump into a crash kernel or may reboot without doing a full CPU RESET.
If GIF is left cleared, the new kernel (or firmware) will be unabled to
awaken APs.  Eat faults on STGI (due to EFER.SVME=0) as it's possible
that SVM could be disabled via NMI shootdown between reading EFER.SVME
and executing STGI.

Link: https://lore.kernel.org/all/cbcb6f35-e5d7-c1c9-4db9-fe5cc4de579a@amd.com
Cc: stable@vger.kernel.org
Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/virtext.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
index 8757078d4442..0acb14806a74 100644
--- a/arch/x86/include/asm/virtext.h
+++ b/arch/x86/include/asm/virtext.h
@@ -126,7 +126,18 @@ static inline void cpu_svm_disable(void)
 
 	wrmsrl(MSR_VM_HSAVE_PA, 0);
 	rdmsrl(MSR_EFER, efer);
-	wrmsrl(MSR_EFER, efer & ~EFER_SVME);
+	if (efer & EFER_SVME) {
+		/*
+		 * Force GIF=1 prior to disabling SVM, e.g. to ensure INIT and
+		 * NMI aren't blocked.  Eat faults on STGI, as it #UDs if SVM
+		 * isn't enabled and SVM can be disabled by an NMI callback.
+		 */
+		asm_volatile_goto("1: stgi\n\t"
+				  _ASM_EXTABLE(1b, %l[fault])
+				  ::: "memory" : fault);
+fault:
+		wrmsrl(MSR_EFER, efer & ~EFER_SVME);
+	}
 }
 
 /** Makes sure SVM is disabled, if it is supported on the CPU
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH v4 3/4] x86/reboot: Disable virtualization in an emergency if SVM is supported

[Sean Christopherson]

Disable SVM on all CPUs via NMI shootdown during an emergency reboot.
Like VMX, SVM can block INIT, e.g. if the emergency reboot is triggered
between CLGI and STGI, and thus can prevent bringing up other CPUs via
INIT-SIPI-SIPI.

Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kernel/reboot.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 374c14b3a9bf..d03c551defcc 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -530,27 +530,26 @@ static inline void kb_wait(void)
 
 static inline void nmi_shootdown_cpus_on_restart(void);
 
-/* Use NMIs as IPIs to tell all CPUs to disable virtualization */
-static void emergency_vmx_disable_all(void)
+static void emergency_reboot_disable_virtualization(void)
 {
 	/* Just make sure we won't change CPUs while doing this */
 	local_irq_disable();
 
 	/*
-	 * Disable VMX on all CPUs before rebooting, otherwise we risk hanging
-	 * the machine, because the CPU blocks INIT when it's in VMX root.
+	 * Disable virtualization on all CPUs before rebooting to avoid hanging
+	 * the system, as VMX and SVM block INIT when running in the host.
 	 *
 	 * We can't take any locks and we may be on an inconsistent state, so
-	 * use NMIs as IPIs to tell the other CPUs to exit VMX root and halt.
+	 * use NMIs as IPIs to tell the other CPUs to disable VMX/SVM and halt.
 	 *
-	 * Do the NMI shootdown even if VMX if off on _this_ CPU, as that
-	 * doesn't prevent a different CPU from being in VMX root operation.
+	 * Do the NMI shootdown even if virtualization is off on _this_ CPU, as
+	 * other CPUs may have virtualization enabled.
 	 */
-	if (cpu_has_vmx()) {
-		/* Safely force _this_ CPU out of VMX root operation. */
-		__cpu_emergency_vmxoff();
+	if (cpu_has_vmx() || cpu_has_svm(NULL)) {
+		/* Safely force _this_ CPU out of VMX/SVM operation. */
+		cpu_emergency_disable_virtualization();
 
-		/* Halt and exit VMX root operation on the other CPUs. */
+		/* Disable VMX/SVM and halt on other CPUs. */
 		nmi_shootdown_cpus_on_restart();
 	}
 }
@@ -587,7 +586,7 @@ static void native_machine_emergency_restart(void)
 	unsigned short mode;
 
 	if (reboot_emergency)
-		emergency_vmx_disable_all();
+		emergency_reboot_disable_virtualization();
 
 	tboot_shutdown(TB_SHUTDOWN_REBOOT);
 
-- 
2.38.1.584.g0f3c55d4c2-goog

[PATCH v4 4/4] x86/reboot: Disable SVM, not just VMX, when stopping CPUs

[Sean Christopherson]

Disable SVM and more importantly force GIF=1 when halting a CPU or
rebooting the machine.  Similar to VMX, SVM allows software to block
INITs via CLGI, and thus can be problematic for a crash/reboot.  The
window for failure is smaller with SVM as INIT is only blocked while
GIF=0, i.e. between CLGI and STGI, but the window does exist.

Fixes: fba4f472b33a ("x86/reboot: Turn off KVM when halting a CPU")
Cc: stable@vger.kernel
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kernel/smp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 06db901fabe8..375b33ecafa2 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -32,7 +32,7 @@
 #include <asm/mce.h>
 #include <asm/trace/irq_vectors.h>
 #include <asm/kexec.h>
-#include <asm/virtext.h>
+#include <asm/reboot.h>
 
 /*
  *	Some notes on x86 processor bugs affecting SMP operation:
@@ -122,7 +122,7 @@ static int smp_stop_nmi_callback(unsigned int val, struct pt_regs *regs)
 	if (raw_smp_processor_id() == atomic_read(&stopping_cpu))
 		return NMI_HANDLED;
 
-	cpu_emergency_vmxoff();
+	cpu_emergency_disable_virtualization();
 	stop_this_cpu(NULL);
 
 	return NMI_HANDLED;
@@ -134,7 +134,7 @@ static int smp_stop_nmi_callback(unsigned int val, struct pt_regs *regs)
 DEFINE_IDTENTRY_SYSVEC(sysvec_reboot)
 {
 	ack_APIC_irq();
-	cpu_emergency_vmxoff();
+	cpu_emergency_disable_virtualization();
 	stop_this_cpu(NULL);
 }
 
-- 
2.38.1.584.g0f3c55d4c2-goog

Re: [PATCH v4 4/4] x86/reboot: Disable SVM, not just VMX, when stopping CPUs

[Sean Christopherson]

On Wed, Nov 30, 2022, Sean Christopherson wrote:
> Disable SVM and more importantly force GIF=1 when halting a CPU or
> rebooting the machine.  Similar to VMX, SVM allows software to block
> INITs via CLGI, and thus can be problematic for a crash/reboot.  The
> window for failure is smaller with SVM as INIT is only blocked while
> GIF=0, i.e. between CLGI and STGI, but the window does exist.
> 
> Fixes: fba4f472b33a ("x86/reboot: Turn off KVM when halting a CPU")
> Cc: stable@vger.kernel

Argh, forgot the .org, and of course my scripts then failed to filter out the
address.

Re: [PATCH v4 2/4] x86/virt: Force GIF=1 prior to disabling SVM (for reboot flows)

[Andrew Cooper]

On 30/11/2022 23:36, Sean Christopherson wrote:
> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> index 8757078d4442..0acb14806a74 100644
> --- a/arch/x86/include/asm/virtext.h
> +++ b/arch/x86/include/asm/virtext.h
> @@ -126,7 +126,18 @@ static inline void cpu_svm_disable(void)
>  
>  	wrmsrl(MSR_VM_HSAVE_PA, 0);
>  	rdmsrl(MSR_EFER, efer);
> -	wrmsrl(MSR_EFER, efer & ~EFER_SVME);
> +	if (efer & EFER_SVME) {
> +		/*
> +		 * Force GIF=1 prior to disabling SVM, e.g. to ensure INIT and
> +		 * NMI aren't blocked.  Eat faults on STGI, as it #UDs if SVM
> +		 * isn't enabled and SVM can be disabled by an NMI callback.

I'd be tempted to tweak this for clarity.

How about "We don't know the state of GIF, and if NMIs are enabled,
there is a race condition where EFER.SVME can be cleared behind our
back.  Ignore #UD, and force GIF=1 in case INIT/NMI are currently
blocked."  ?

The STGI can't actually #UD on real hardware, because SKINIT and SVM
exist in identical sets of parts, but it can #UD in principle in a VM
which doesn't offer emulate SKINIT.

Given that we are in cpu_svm_disable(), there's also
MSR_VM_CR.INIT_REDIRECTION to consider, but perhaps that's better left
to the series which adds SKINIT support.

~Andrew

Re: [PATCH v4 2/4] x86/virt: Force GIF=1 prior to disabling SVM (for reboot flows)

[Sean Christopherson]

On Thu, Dec 01, 2022, Andrew Cooper wrote:
> On 30/11/2022 23:36, Sean Christopherson wrote:
> > diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
> > index 8757078d4442..0acb14806a74 100644
> > --- a/arch/x86/include/asm/virtext.h
> > +++ b/arch/x86/include/asm/virtext.h
> > @@ -126,7 +126,18 @@ static inline void cpu_svm_disable(void)
> >  
> >  	wrmsrl(MSR_VM_HSAVE_PA, 0);
> >  	rdmsrl(MSR_EFER, efer);
> > -	wrmsrl(MSR_EFER, efer & ~EFER_SVME);
> > +	if (efer & EFER_SVME) {
> > +		/*
> > +		 * Force GIF=1 prior to disabling SVM, e.g. to ensure INIT and
> > +		 * NMI aren't blocked.  Eat faults on STGI, as it #UDs if SVM
> > +		 * isn't enabled and SVM can be disabled by an NMI callback.
> 
> I'd be tempted to tweak this for clarity.
> 
> How about "We don't know the state of GIF, and if NMIs are enabled,
> there is a race condition where EFER.SVME can be cleared behind our
> back.  Ignore #UD, and force GIF=1 in case INIT/NMI are currently
> blocked."  ?
> 
> The STGI can't actually #UD on real hardware, because SKINIT and SVM
> exist in identical sets of parts, but it can #UD in principle in a VM
> which doesn't offer emulate SKINIT.

Ah, right, "may #UD", not "will #UD".  And despite writing this, I also keep
forgetting why forcing GIF is even necessary.  How about?

		/*
		 * Force GIF=1 prior to disabling SVM to ensure INIT and NMI
		 * aren't blocked, e.g. if a fatal error occurred between CLGI
		 * and STGI.  Note, STGI may #UD if SVM is disabled from NMI
		 * context between reading EFER and executing STGI.  In that
		 * case, GIF must already be set, otherwise the NMI would have
		 * been blocked, so just eat the fault.
		 */

> Given that we are in cpu_svm_disable(), there's also
> MSR_VM_CR.INIT_REDIRECTION to consider, but perhaps that's better left
> to the series which adds SKINIT support.
> 
> ~Andrew

Re: [PATCH v4 2/4] x86/virt: Force GIF=1 prior to disabling SVM (for reboot flows)

[Andrew Cooper]

On 01/12/2022 23:04, Sean Christopherson wrote:
> On Thu, Dec 01, 2022, Andrew Cooper wrote:
>> On 30/11/2022 23:36, Sean Christopherson wrote:
>>> diff --git a/arch/x86/include/asm/virtext.h b/arch/x86/include/asm/virtext.h
>>> index 8757078d4442..0acb14806a74 100644
>>> --- a/arch/x86/include/asm/virtext.h
>>> +++ b/arch/x86/include/asm/virtext.h
>>> @@ -126,7 +126,18 @@ static inline void cpu_svm_disable(void)
>>>  
>>>  	wrmsrl(MSR_VM_HSAVE_PA, 0);
>>>  	rdmsrl(MSR_EFER, efer);
>>> -	wrmsrl(MSR_EFER, efer & ~EFER_SVME);
>>> +	if (efer & EFER_SVME) {
>>> +		/*
>>> +		 * Force GIF=1 prior to disabling SVM, e.g. to ensure INIT and
>>> +		 * NMI aren't blocked.  Eat faults on STGI, as it #UDs if SVM
>>> +		 * isn't enabled and SVM can be disabled by an NMI callback.
>> I'd be tempted to tweak this for clarity.
>>
>> How about "We don't know the state of GIF, and if NMIs are enabled,
>> there is a race condition where EFER.SVME can be cleared behind our
>> back.  Ignore #UD, and force GIF=1 in case INIT/NMI are currently
>> blocked."  ?
>>
>> The STGI can't actually #UD on real hardware, because SKINIT and SVM
>> exist in identical sets of parts, but it can #UD in principle in a VM
>> which doesn't offer emulate SKINIT.
> Ah, right, "may #UD", not "will #UD".  And despite writing this, I also keep
> forgetting why forcing GIF is even necessary.  How about?
>
> 		/*
> 		 * Force GIF=1 prior to disabling SVM to ensure INIT and NMI
> 		 * aren't blocked, e.g. if a fatal error occurred between CLGI
> 		 * and STGI.  Note, STGI may #UD if SVM is disabled from NMI
> 		 * context between reading EFER and executing STGI.  In that
> 		 * case, GIF must already be set, otherwise the NMI would have
> 		 * been blocked, so just eat the fault.
> 		 */

LGTM.

~Andrew

Re: [PATCH v4 0/4] x86/crash: Fix double NMI shootdown bug

[Thomas Gleixner]

On Wed, Nov 30 2022 at 23:36, Sean Christopherson wrote:

> Fix a double NMI shootdown bug found and debugged by Guilherme, who did all
> the hard work.  NMI shootdown is a one-time thing; the handler leaves NMIs
> blocked and enters halt.  At best, a second (or third...) shootdown is an
> expensive nop, at worst it can hang the kernel and prevent kexec'ing into
> a new kernel, e.g. prior to the hardening of register_nmi_handler(), a
> double shootdown resulted in a double list_add(), which is fatal when running
> with CONFIG_BUG_ON_DATA_CORRUPTION=y.
>
> With the "right" kexec/kdump configuration, emergency_vmx_disable_all() can
> be reached after kdump_nmi_shootdown_cpus() (currently the only two users
> of nmi_shootdown_cpus()).
>
> To fix, move the disabling of virtualization into crash_nmi_callback(),
> remove emergency_vmx_disable_all()'s callback, and do a shootdown for
> emergency_vmx_disable_all() if and only if a shootdown hasn't yet occurred.
> The only thing emergency_vmx_disable_all() cares about is disabling VMX/SVM
> (obviously), and since I can't envision a use case for an NMI shootdown that
> doesn't want to disable virtualization, doing that in the core handler means
> emergency_vmx_disable_all() only needs to ensure _a_ shootdown occurs, it
> doesn't care when that shootdown happened or what callback may have run.

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH v4 0/4] x86/crash: Fix double NMI shootdown bug

[Sean Christopherson]

On Wed, 30 Nov 2022 23:36:46 +0000, Sean Christopherson wrote:
> Fix a double NMI shootdown bug found and debugged by Guilherme, who did all
> the hard work.  NMI shootdown is a one-time thing; the handler leaves NMIs
> blocked and enters halt.  At best, a second (or third...) shootdown is an
> expensive nop, at worst it can hang the kernel and prevent kexec'ing into
> a new kernel, e.g. prior to the hardening of register_nmi_handler(), a
> double shootdown resulted in a double list_add(), which is fatal when running
> with CONFIG_BUG_ON_DATA_CORRUPTION=y.
> 
> [...]

Applied to kvm-x86 misc, thanks!

[1/4] x86/crash: Disable virt in core NMI crash handler to avoid double shootdown
      https://github.com/kvm-x86/linux/commit/26044aff37a5
[2/4] x86/virt: Force GIF=1 prior to disabling SVM (for reboot flows)
      https://github.com/kvm-x86/linux/commit/6a3236580b0b
[3/4] x86/reboot: Disable virtualization in an emergency if SVM is supported
      https://github.com/kvm-x86/linux/commit/d81f952aa657
[4/4] x86/reboot: Disable SVM, not just VMX, when stopping CPUs
      https://github.com/kvm-x86/linux/commit/a2b07fa7b933

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes

Re: [PATCH v4 0/4] x86/crash: Fix double NMI shootdown bug

[Guilherme G. Piccoli]

On 27/01/2023 21:05, Sean Christopherson wrote:
> On Wed, 30 Nov 2022 23:36:46 +0000, Sean Christopherson wrote:
>> Fix a double NMI shootdown bug found and debugged by Guilherme, who did all
>> the hard work.  NMI shootdown is a one-time thing; the handler leaves NMIs
>> blocked and enters halt.  At best, a second (or third...) shootdown is an
>> expensive nop, at worst it can hang the kernel and prevent kexec'ing into
>> a new kernel, e.g. prior to the hardening of register_nmi_handler(), a
>> double shootdown resulted in a double list_add(), which is fatal when running
>> with CONFIG_BUG_ON_DATA_CORRUPTION=y.
>>
>> [...]
> 
> Applied to kvm-x86 misc, thanks!
> 
> [1/4] x86/crash: Disable virt in core NMI crash handler to avoid double shootdown
>       https://github.com/kvm-x86/linux/commit/26044aff37a5
> [2/4] x86/virt: Force GIF=1 prior to disabling SVM (for reboot flows)
>       https://github.com/kvm-x86/linux/commit/6a3236580b0b
> [3/4] x86/reboot: Disable virtualization in an emergency if SVM is supported
>       https://github.com/kvm-x86/linux/commit/d81f952aa657
> [4/4] x86/reboot: Disable SVM, not just VMX, when stopping CPUs
>       https://github.com/kvm-x86/linux/commit/a2b07fa7b933
> 
> --
> https://github.com/kvm-x86/linux/tree/next
> https://github.com/kvm-x86/linux/tree/fixes


Thanks a bunch Sean!