Re: [PATCH stable] KVM: x86: Fix lost interrupt on irr_pending race

[Paolo Bonzini <pbonzini@redhat.com>]

On 21/04/2015 09:52, Paolo Bonzini wrote:
> From: Nadav Amit <namit@cs.technion.ac.il>
> 
> [ upstream commit f210f7572bedf3320599e8b2d8e8ec2d96270d0b ]
> 
> apic_find_highest_irr assumes irr_pending is set if any vector in APIC_IRR is
> set.  If this assumption is broken and apicv is disabled, the injection of
> interrupts may be deferred until another interrupt is delivered to the guest.
> Ultimately, if no other interrupt should be injected to that vCPU, the pending
> interrupt may be lost.
> 
> commit 56cc2406d68c ("KVM: nVMX: fix "acknowledge interrupt on exit" when APICv
> is in use") changed the behavior of apic_clear_irr so irr_pending is cleared
> after setting APIC_IRR vector. After this commit, if apic_set_irr and
> apic_clear_irr run simultaneously, a race may occur, resulting in APIC_IRR
> vector set, and irr_pending cleared. In the following example, assume a single
> vector is set in IRR prior to calling apic_clear_irr:
> 
> apic_set_irr				apic_clear_irr
> ------------				--------------
> apic->irr_pending = true;
> 					apic_clear_vector(...);
> 					vec = apic_search_irr(apic);
> 					// => vec == -1
> apic_set_vector(...);
> 					apic->irr_pending = (vec != -1);
> 					// => apic->irr_pending == false
> 
> Nonetheless, it appears the race might even occur prior to this commit:
> 
> apic_set_irr				apic_clear_irr
> ------------				--------------
> apic->irr_pending = true;
> 					apic->irr_pending = false;
> 					apic_clear_vector(...);
> 					if (apic_search_irr(apic) != -1)
> 						apic->irr_pending = true;
> 					// => apic->irr_pending == false
> apic_set_vector(...);
> 
> Fixing this issue by:
> 1. Restoring the previous behavior of apic_clear_irr: clear irr_pending, call
>    apic_clear_vector, and then if APIC_IRR is non-zero, set irr_pending.
> 2. On apic_set_irr: first call apic_set_vector, then set irr_pending.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> Fixes: 33e4c68656a2e461b296ce714ec322978de85412
> Cc: stable@vger.kernel.org # 2.6.32+
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	The race was reported in 3.17+ by Brad Campbell and in
> 	2.6.32 by Saso Slavicic, so it qualifies for stable.

Patch for kernels before 3.17:

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 6e8ce5a1a05d..e0e5642dae41 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -341,8 +341,12 @@ EXPORT_SYMBOL_GPL(kvm_apic_update_irr);
 
 static inline void apic_set_irr(int vec, struct kvm_lapic *apic)
 {
-	apic->irr_pending = true;
 	apic_set_vector(vec, apic->regs + APIC_IRR);
+	/*
+	 * irr_pending must be true if any interrupt is pending; set it after
+	 * APIC_IRR to avoid race with apic_clear_irr
+	 */
+	apic->irr_pending = true;
 }
 
 static inline int apic_search_irr(struct kvm_lapic *apic)


Thanks,

Paolo