[PATCH 0/4] x86/x86: KVM: Fixes for KVM's PI wakeup handler

[PATCH 0/4] x86/x86: KVM: Fixes for KVM's PI wakeup handler

[Sean Christopherson]

Two fixes and two cleanups related to KVM's posted interrupt wakeup
handler.  Fix #1 ensures any in-flight IRQs finish after changing the
handler.  Fix #2 actually uninstalls KVM's handler so that a spurious IRQ
won't jump into a freed module.

AFAIK, no one has actually hit these bugs as it would require a really
spurious IRQ, or a bug+race elsehwere that caused a device to post an
interrupt well after a KVM guest is torn down.

Sean Christopherson (4):
  x86/irq: Ensure PI wakeup handler is unregistered before module unload
  KVM: VMX: Unregister posted interrupt wakeup handler on hardware
    unsetup
  x86/irq: KVM: Harden posted interrupt (un)registration paths
  KVM: VMX: Register posted interrupt wakeup handler iff APICv is
    enabled

 arch/x86/include/asm/irq.h |  3 ++-
 arch/x86/kernel/irq.c      | 30 +++++++++++++++++++++---------
 arch/x86/kvm/vmx/vmx.c     |  9 +++++++--
 3 files changed, 30 insertions(+), 12 deletions(-)

-- 
2.33.0.882.g93a45727a2-goog

[PATCH 1/4] x86/irq: Ensure PI wakeup handler is unregistered before module unload

[Sean Christopherson]

Add a synchronize_rcu() after setting the posted interrupt wakeup handler
to ensure all readers, i.e. in-flight IRQ handlers, see the new handler
before returning to the caller.  If the caller is an exiting module and
is unregistering its handler, failure to wait could result in the IRQ
handler jumping into an unloaded module.

Fixes: f6b3c72c2366 ("x86/irq: Define a global vector for VT-d Posted-Interrupts")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kernel/irq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index e28f6a5d14f1..20773d315308 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -293,6 +293,7 @@ void kvm_set_posted_intr_wakeup_handler(void (*handler)(void))
 		kvm_posted_intr_wakeup_handler = handler;
 	else
 		kvm_posted_intr_wakeup_handler = dummy_handler;
+	synchronize_rcu();
 }
 EXPORT_SYMBOL_GPL(kvm_set_posted_intr_wakeup_handler);
 
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 2/4] KVM: VMX: Unregister posted interrupt wakeup handler on hardware unsetup

[Sean Christopherson]

Unregister KVM's posted interrupt wakeup handler during unsetup so that a
spurious interrupt that arrives after kvm_intel.ko is unloaded doesn't
call into freed memory.

Fixes: bf9f6ac8d749 ("KVM: Update Posted-Interrupts Descriptor when vCPU is blocked")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 1c8b2b6e7ed9..bfdcdb399212 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7553,6 +7553,8 @@ static void vmx_migrate_timers(struct kvm_vcpu *vcpu)
 
 static void hardware_unsetup(void)
 {
+	kvm_set_posted_intr_wakeup_handler(NULL);
+
 	if (nested)
 		nested_vmx_hardware_unsetup();
 
@@ -7881,8 +7883,6 @@ static __init int hardware_setup(void)
 		vmx_x86_ops.request_immediate_exit = __kvm_request_immediate_exit;
 	}
 
-	kvm_set_posted_intr_wakeup_handler(pi_wakeup_handler);
-
 	kvm_mce_cap_supported |= MCG_LMCE_P;
 
 	if (pt_mode != PT_MODE_SYSTEM && pt_mode != PT_MODE_HOST_GUEST)
@@ -7906,6 +7906,9 @@ static __init int hardware_setup(void)
 	r = alloc_kvm_area();
 	if (r)
 		nested_vmx_hardware_unsetup();
+
+	kvm_set_posted_intr_wakeup_handler(pi_wakeup_handler);
+
 	return r;
 }
 
-- 
2.33.0.882.g93a45727a2-goog

[PATCH 3/4] x86/irq: KVM: Harden posted interrupt (un)registration paths

[Sean Christopherson]

Split the register and unregister paths for the posted interrupt wakeup
handler, and WARN on conditions that are blatant bugs, e.g. attempting to
overwrite an existing handler, unregistering the wrong handler, etc...
This is very much a "low hanging fruit" hardening, e.g. a broken module
could foul things up by doing concurrent registration from multiple CPUs.

Drop the use of a dummy handler so that the rejection logic can use a
simple NULL check.  There is zero benefit to blindly calling into a dummy
handler.

Note, the registration path doesn't require synchronization, as it's the
caller's responsibility to not generate interrupts it cares about until
after its handler is registered, i.e. there can't be a relevant in-flight
interrupt.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/irq.h |  3 ++-
 arch/x86/kernel/irq.c      | 29 ++++++++++++++++++++---------
 arch/x86/kvm/vmx/vmx.c     |  4 ++--
 3 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/arch/x86/include/asm/irq.h b/arch/x86/include/asm/irq.h
index 768aa234cbb4..c79014c2443d 100644
--- a/arch/x86/include/asm/irq.h
+++ b/arch/x86/include/asm/irq.h
@@ -30,7 +30,8 @@ struct irq_desc;
 extern void fixup_irqs(void);
 
 #ifdef CONFIG_HAVE_KVM
-extern void kvm_set_posted_intr_wakeup_handler(void (*handler)(void));
+extern void kvm_register_posted_intr_wakeup_handler(void (*handler)(void));
+extern void kvm_unregister_posted_intr_wakeup_handler(void (*handler)(void));
 #endif
 
 extern void (*x86_platform_ipi_callback)(void);
diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 20773d315308..97f452cc84be 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -284,18 +284,26 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_x86_platform_ipi)
 #endif
 
 #ifdef CONFIG_HAVE_KVM
-static void dummy_handler(void) {}
-static void (*kvm_posted_intr_wakeup_handler)(void) = dummy_handler;
+static void (*kvm_posted_intr_wakeup_handler)(void);
 
-void kvm_set_posted_intr_wakeup_handler(void (*handler)(void))
+void kvm_register_posted_intr_wakeup_handler(void (*handler)(void))
 {
-	if (handler)
-		kvm_posted_intr_wakeup_handler = handler;
-	else
-		kvm_posted_intr_wakeup_handler = dummy_handler;
+	if (WARN_ON_ONCE(!handler || kvm_posted_intr_wakeup_handler))
+		return;
+
+	WRITE_ONCE(kvm_posted_intr_wakeup_handler, handler);
+}
+EXPORT_SYMBOL_GPL(kvm_register_posted_intr_wakeup_handler);
+
+void kvm_unregister_posted_intr_wakeup_handler(void (*handler)(void))
+{
+	if (WARN_ON_ONCE(!handler || handler != kvm_posted_intr_wakeup_handler))
+		return;
+
+	WRITE_ONCE(kvm_posted_intr_wakeup_handler, NULL);
 	synchronize_rcu();
 }
-EXPORT_SYMBOL_GPL(kvm_set_posted_intr_wakeup_handler);
+EXPORT_SYMBOL_GPL(kvm_unregister_posted_intr_wakeup_handler);
 
 /*
  * Handler for POSTED_INTERRUPT_VECTOR.
@@ -311,9 +319,12 @@ DEFINE_IDTENTRY_SYSVEC_SIMPLE(sysvec_kvm_posted_intr_ipi)
  */
 DEFINE_IDTENTRY_SYSVEC(sysvec_kvm_posted_intr_wakeup_ipi)
 {
+	void (*handler)(void) = READ_ONCE(kvm_posted_intr_wakeup_handler);
+
 	ack_APIC_irq();
 	inc_irq_stat(kvm_posted_intr_wakeup_ipis);
-	kvm_posted_intr_wakeup_handler();
+	if (handler)
+		handler();
 }
 
 /*
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index bfdcdb399212..9164f1870d49 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7553,7 +7553,7 @@ static void vmx_migrate_timers(struct kvm_vcpu *vcpu)
 
 static void hardware_unsetup(void)
 {
-	kvm_set_posted_intr_wakeup_handler(NULL);
+	kvm_unregister_posted_intr_wakeup_handler(pi_wakeup_handler);
 
 	if (nested)
 		nested_vmx_hardware_unsetup();
@@ -7907,7 +7907,7 @@ static __init int hardware_setup(void)
 	if (r)
 		nested_vmx_hardware_unsetup();
 
-	kvm_set_posted_intr_wakeup_handler(pi_wakeup_handler);
+	kvm_register_posted_intr_wakeup_handler(pi_wakeup_handler);
 
 	return r;
 }
-- 
2.33.0.882.g93a45727a2-goog

Re: [PATCH 3/4] x86/irq: KVM: Harden posted interrupt (un)registration paths

[Paolo Bonzini]

On 09/10/21 02:11, Sean Christopherson wrote:
> Split the register and unregister paths for the posted interrupt wakeup
> handler, and WARN on conditions that are blatant bugs, e.g. attempting to
> overwrite an existing handler, unregistering the wrong handler, etc...
> This is very much a "low hanging fruit" hardening, e.g. a broken module
> could foul things up by doing concurrent registration from multiple CPUs.
> 
> Drop the use of a dummy handler so that the rejection logic can use a
> simple NULL check.  There is zero benefit to blindly calling into a dummy
> handler.
> 
> Note, the registration path doesn't require synchronization, as it's the
> caller's responsibility to not generate interrupts it cares about until
> after its handler is registered, i.e. there can't be a relevant in-flight
> interrupt.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Seems a bit overengineered, considering there are just two callers.  Doing

diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
index 20773d315308..766ffe3ba313 100644
--- a/arch/x86/kernel/irq.c
+++ b/arch/x86/kernel/irq.c
@@ -291,9 +291,10 @@ void kvm_set_posted_intr_wakeup_handler(void (*handler)(void))
  {
  	if (handler)
  		kvm_posted_intr_wakeup_handler = handler;
-	else
+	else {
  		kvm_posted_intr_wakeup_handler = dummy_handler;
-	synchronize_rcu();
+		synchronize_rcu();
+	}
  }
  EXPORT_SYMBOL_GPL(kvm_set_posted_intr_wakeup_handler);
  

on top of path 1 ensures that the synchronization is only done on the
unregistration path.

Paolo

> -- 
>   arch/x86/include/asm/irq.h |  3 ++-
>   arch/x86/kernel/irq.c      | 29 ++++++++++++++++++++---------
>   arch/x86/kvm/vmx/vmx.c     |  4 ++--
>   3 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/include/asm/irq.h b/arch/x86/include/asm/irq.h
> index 768aa234cbb4..c79014c2443d 100644
> --- a/arch/x86/include/asm/irq.h
> +++ b/arch/x86/include/asm/irq.h
> @@ -30,7 +30,8 @@ struct irq_desc;
>   extern void fixup_irqs(void);
>   
>   #ifdef CONFIG_HAVE_KVM
> -extern void kvm_set_posted_intr_wakeup_handler(void (*handler)(void));
> +extern void kvm_register_posted_intr_wakeup_handler(void (*handler)(void));
> +extern void kvm_unregister_posted_intr_wakeup_handler(void (*handler)(void));
>   #endif
>   
>   extern void (*x86_platform_ipi_callback)(void);
> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> index 20773d315308..97f452cc84be 100644
> --- a/arch/x86/kernel/irq.c
> +++ b/arch/x86/kernel/irq.c
> @@ -284,18 +284,26 @@ DEFINE_IDTENTRY_SYSVEC(sysvec_x86_platform_ipi)
>   #endif
>   
>   #ifdef CONFIG_HAVE_KVM
> -static void dummy_handler(void) {}
> -static void (*kvm_posted_intr_wakeup_handler)(void) = dummy_handler;
> +static void (*kvm_posted_intr_wakeup_handler)(void);
>   
> -void kvm_set_posted_intr_wakeup_handler(void (*handler)(void))
> +void kvm_register_posted_intr_wakeup_handler(void (*handler)(void))
>   {
> -	if (handler)
> -		kvm_posted_intr_wakeup_handler = handler;
> -	else
> -		kvm_posted_intr_wakeup_handler = dummy_handler;
> +	if (WARN_ON_ONCE(!handler || kvm_posted_intr_wakeup_handler))
> +		return;
> +
> +	WRITE_ONCE(kvm_posted_intr_wakeup_handler, handler);
> +}
> +EXPORT_SYMBOL_GPL(kvm_register_posted_intr_wakeup_handler);
> +
> +void kvm_unregister_posted_intr_wakeup_handler(void (*handler)(void))
> +{
> +	if (WARN_ON_ONCE(!handler || handler != kvm_posted_intr_wakeup_handler))
> +		return;
> +
> +	WRITE_ONCE(kvm_posted_intr_wakeup_handler, NULL);
>   	synchronize_rcu();
>   }
> -EXPORT_SYMBOL_GPL(kvm_set_posted_intr_wakeup_handler);
> +EXPORT_SYMBOL_GPL(kvm_unregister_posted_intr_wakeup_handler);
>   
>   /*
>    * Handler for POSTED_INTERRUPT_VECTOR.
> @@ -311,9 +319,12 @@ DEFINE_IDTENTRY_SYSVEC_SIMPLE(sysvec_kvm_posted_intr_ipi)
>    */
>   DEFINE_IDTENTRY_SYSVEC(sysvec_kvm_posted_intr_wakeup_ipi)
>   {
> +	void (*handler)(void) = READ_ONCE(kvm_posted_intr_wakeup_handler);
> +
>   	ack_APIC_irq();
>   	inc_irq_stat(kvm_posted_intr_wakeup_ipis);
> -	kvm_posted_intr_wakeup_handler();
> +	if (handler)
> +		handler();
>   }
>   
>   /*
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index bfdcdb399212..9164f1870d49 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7553,7 +7553,7 @@ static void vmx_migrate_timers(struct kvm_vcpu *vcpu)
>   
>   static void hardware_unsetup(void)
>   {
> -	kvm_set_posted_intr_wakeup_handler(NULL);
> +	kvm_unregister_posted_intr_wakeup_handler(pi_wakeup_handler);
>   
>   	if (nested)
>   		nested_vmx_hardware_unsetup();
> @@ -7907,7 +7907,7 @@ static __init int hardware_setup(void)
>   	if (r)
>   		nested_vmx_hardware_unsetup();
>   
> -	kvm_set_posted_intr_wakeup_handler(pi_wakeup_handler);
> +	kvm_register_posted_intr_wakeup_handler(pi_wakeup_handler);
>   
>   	return r;
>   }
> 

[PATCH 4/4] KVM: VMX: Register posted interrupt wakeup handler iff APICv is enabled

[Sean Christopherson]

Don't bother registering a posted interrupt wakeup handler if APICv is
disabled, KVM utilizes the wakeup vector if and only if APICv is enabled.
Practically speaking, there's no meaningful functional change as KVM's
wakeup handler is a glorified nop if there are no vCPUs using posted
interrupts, not to mention that nothing in the system should be sending
wakeup interrupts when APICv is disabled.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9164f1870d49..df9ad4675215 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7553,7 +7553,8 @@ static void vmx_migrate_timers(struct kvm_vcpu *vcpu)
 
 static void hardware_unsetup(void)
 {
-	kvm_unregister_posted_intr_wakeup_handler(pi_wakeup_handler);
+	if (enable_apicv)
+		kvm_unregister_posted_intr_wakeup_handler(pi_wakeup_handler);
 
 	if (nested)
 		nested_vmx_hardware_unsetup();
@@ -7907,7 +7908,8 @@ static __init int hardware_setup(void)
 	if (r)
 		nested_vmx_hardware_unsetup();
 
-	kvm_register_posted_intr_wakeup_handler(pi_wakeup_handler);
+	if (enable_apicv)
+		kvm_register_posted_intr_wakeup_handler(pi_wakeup_handler);
 
 	return r;
 }
-- 
2.33.0.882.g93a45727a2-goog

Re: [PATCH 4/4] KVM: VMX: Register posted interrupt wakeup handler iff APICv is enabled

[Paolo Bonzini]

On 09/10/21 02:11, Sean Christopherson wrote:
> Don't bother registering a posted interrupt wakeup handler if APICv is
> disabled, KVM utilizes the wakeup vector if and only if APICv is enabled.
> Practically speaking, there's no meaningful functional change as KVM's
> wakeup handler is a glorified nop if there are no vCPUs using posted
> interrupts, not to mention that nothing in the system should be sending
> wakeup interrupts when APICv is disabled.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/vmx/vmx.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 9164f1870d49..df9ad4675215 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7553,7 +7553,8 @@ static void vmx_migrate_timers(struct kvm_vcpu *vcpu)
>   
>   static void hardware_unsetup(void)
>   {
> -	kvm_unregister_posted_intr_wakeup_handler(pi_wakeup_handler);
> +	if (enable_apicv)
> +		kvm_unregister_posted_intr_wakeup_handler(pi_wakeup_handler);
>   
>   	if (nested)
>   		nested_vmx_hardware_unsetup();
> @@ -7907,7 +7908,8 @@ static __init int hardware_setup(void)
>   	if (r)
>   		nested_vmx_hardware_unsetup();
>   
> -	kvm_register_posted_intr_wakeup_handler(pi_wakeup_handler);
> +	if (enable_apicv)
> +		kvm_register_posted_intr_wakeup_handler(pi_wakeup_handler);
>   
>   	return r;
>   }

Also adds unnecessary complexity (you have to ensure the condition 
cannot change from one call to the other), so I am only queuing patches 
1 and 2.

Paolo