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

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

[Paolo Bonzini]

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

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

[Luis Henriques]

On Tue, Apr 21, 2015 at 10:47:37AM +0200, Paolo Bonzini wrote:
> 
> 
> 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:
> 

Thanks Paolo.  I was going to apply this backport to the 3.16 kernel
but it looks like the original commit is a clean cherry-pick.  Shall I
still apply your backport, or do you think the original commit should
be applied instead?

Cheers,
--
Luís

> 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
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Paolo Bonzini]

On 22/04/2015 15:34, Luis Henriques wrote:
> Thanks Paolo.  I was going to apply this backport to the 3.16 kernel
> but it looks like the original commit is a clean cherry-pick.  Shall I
> still apply your backport, or do you think the original commit should
> be applied instead?

Indeed you're right.  I wrote the backport for 3.16(.0).  However,
commit 56cc2406d68c0f09505c389e276f27a99f495cbd was marked for stable,
so it's necessary to cherry-pick the entire patch on the stable kernel
where the buggy commit was backported.

That should be, according to the stable@vger.kernel.org archives,
3.10.54+, 3.13.11.7+, 3.14.18+, 3.16.2+.

Paolo

> Cheers,
> --
> Luís
> 
>> 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
>> --
>> To unsubscribe from this list: send the line "unsubscribe stable" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Luis Henriques]

On Wed, Apr 22, 2015 at 03:47:04PM +0200, Paolo Bonzini wrote:
> 
> 
> On 22/04/2015 15:34, Luis Henriques wrote:
> > Thanks Paolo.  I was going to apply this backport to the 3.16 kernel
> > but it looks like the original commit is a clean cherry-pick.  Shall I
> > still apply your backport, or do you think the original commit should
> > be applied instead?
> 
> Indeed you're right.  I wrote the backport for 3.16(.0).  However,
> commit 56cc2406d68c0f09505c389e276f27a99f495cbd was marked for stable,
> so it's necessary to cherry-pick the entire patch on the stable kernel
> where the buggy commit was backported.
> 
> That should be, according to the stable@vger.kernel.org archives,
> 3.10.54+, 3.13.11.7+, 3.14.18+, 3.16.2+.
> 

Great, thanks for the quick reply.  I'll queue the (entire) fix for
the 3.16 kernel.

Cheers,
--
Luís

> Paolo
> 
> > Cheers,
> > --
> > Luís
> > 
> >> 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
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe stable" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Saso Slavicic]

> From: Paolo Bonzini
> Sent: Tuesday, April 21, 2015 10:48 AM


Hi, big thanks to all involved in this and to Brad for endless reboots ;-)


>> 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:

This will probably end up in RHEL6 sooner or later (but probably not before
6.7)?
As I like to experiment a bit, would this patch do for -2.6.32-504.12.2.el6
kernel? The code in that function is somewhat different...

--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -230,8 +230,13 @@
 
 static inline int apic_test_and_set_irr(int vec, struct kvm_lapic *apic)
 {
+	int ret = apic_test_and_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;
-	return apic_test_and_set_vector(vec, apic->regs + APIC_IRR);
+	return ret;
 }
 
 static inline int apic_search_irr(struct kvm_lapic *apic)


Regards,
Saso Slavicic

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

[Paolo Bonzini]

On 28/04/2015 18:24, Saso Slavicic wrote:
> This will probably end up in RHEL6 sooner or later (but probably not before
> 6.7)?
> As I like to experiment a bit, would this patch do for -2.6.32-504.12.2.el6
> kernel? The code in that function is somewhat different...
> 
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -230,8 +230,13 @@
>  
>  static inline int apic_test_and_set_irr(int vec, struct kvm_lapic *apic)
>  {
> +	int ret = apic_test_and_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;
> -	return apic_test_and_set_vector(vec, apic->regs + APIC_IRR);
> +	return ret;
>  }
>  
>  static inline int apic_search_irr(struct kvm_lapic *apic)
> 
> 
> Regards,

Yes, this exact same patch is brewing in the RHEL6.7 internal trees...

See https://bugzilla.redhat.com/show_bug.cgi?id=1213741

Paolo