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@xxxxxxxxxxxxxxxxx>
> >
> > [ 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@xxxxxxxxxxxxxxxxx>
> > Fixes: 33e4c68656a2e461b296ce714ec322978de85412
> > Cc: stable@xxxxxxxxxxxxxxx # 2.6.32+
> > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html