| Message ID | 55360EA9.2030806@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)