* Re: [PATCH stable] KVM: x86: Fix lost interrupt on irr_pending race
[not found] <1429602745-29882-1-git-send-email-pbonzini@redhat.com>
@ 2015-04-21 8:47 ` Paolo Bonzini
2015-04-22 13:34 ` Luis Henriques
2015-04-28 16:24 ` Saso Slavicic
0 siblings, 2 replies; 6+ messages in thread
From: Paolo Bonzini @ 2015-04-21 8:47 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: saso.linux, lists2009, Nadav Amit, stable
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH stable] KVM: x86: Fix lost interrupt on irr_pending race
2015-04-21 8:47 ` [PATCH stable] KVM: x86: Fix lost interrupt on irr_pending race Paolo Bonzini
@ 2015-04-22 13:34 ` Luis Henriques
2015-04-22 13:47 ` Paolo Bonzini
2015-04-28 16:24 ` Saso Slavicic
1 sibling, 1 reply; 6+ messages in thread
From: Luis Henriques @ 2015-04-22 13:34 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, kvm, saso.linux, lists2009, Nadav Amit, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH stable] KVM: x86: Fix lost interrupt on irr_pending race
2015-04-22 13:34 ` Luis Henriques
@ 2015-04-22 13:47 ` Paolo Bonzini
2015-04-22 13:52 ` Luis Henriques
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2015-04-22 13:47 UTC (permalink / raw)
To: Luis Henriques
Cc: linux-kernel, kvm, saso.linux, lists2009, Nadav Amit, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH stable] KVM: x86: Fix lost interrupt on irr_pending race
2015-04-22 13:47 ` Paolo Bonzini
@ 2015-04-22 13:52 ` Luis Henriques
0 siblings, 0 replies; 6+ messages in thread
From: Luis Henriques @ 2015-04-22 13:52 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, kvm, saso.linux, lists2009, Nadav Amit, stable
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH stable] KVM: x86: Fix lost interrupt on irr_pending race
2015-04-21 8:47 ` [PATCH stable] KVM: x86: Fix lost interrupt on irr_pending race Paolo Bonzini
2015-04-22 13:34 ` Luis Henriques
@ 2015-04-28 16:24 ` Saso Slavicic
2015-04-29 16:45 ` Paolo Bonzini
1 sibling, 1 reply; 6+ messages in thread
From: Saso Slavicic @ 2015-04-28 16:24 UTC (permalink / raw)
To: 'Paolo Bonzini', kvm; +Cc: lists2009, 'Nadav Amit'
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH stable] KVM: x86: Fix lost interrupt on irr_pending race
2015-04-28 16:24 ` Saso Slavicic
@ 2015-04-29 16:45 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2015-04-29 16:45 UTC (permalink / raw)
To: Saso Slavicic, kvm; +Cc: lists2009, 'Nadav Amit'
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-04-29 16:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1429602745-29882-1-git-send-email-pbonzini@redhat.com>
2015-04-21 8:47 ` [PATCH stable] KVM: x86: Fix lost interrupt on irr_pending race Paolo Bonzini
2015-04-22 13:34 ` Luis Henriques
2015-04-22 13:47 ` Paolo Bonzini
2015-04-22 13:52 ` Luis Henriques
2015-04-28 16:24 ` Saso Slavicic
2015-04-29 16:45 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).