[PATCH] KVM: Fix race between APIC TMR and IRR

[PATCH] KVM: Fix race between APIC TMR and IRR

[Avi Kivity]

When we queue an interrupt to the local apic, we set the IRR before the TMR.
The vcpu can pick up the IRR and inject the interrupt before setting the TMR,
and perhaps even EOI it, causing incorrect behaviour.

The race is really insignificant since it can only occur on the first
interrupt (usually following interrupts will not change TMR), but it's better
closed than open.

Fixed by reordering setting the TMR vs IRR.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/lapic.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 3063a0c..ba8c045 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -373,6 +373,12 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 		if (unlikely(!apic_enabled(apic)))
 			break;
 
+		if (trig_mode) {
+			apic_debug("level trig mode for vector %d", vector);
+			apic_set_vector(vector, apic->regs + APIC_TMR);
+		} else
+			apic_clear_vector(vector, apic->regs + APIC_TMR);
+
 		result = !apic_test_and_set_irr(vector, apic);
 		trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
 					  trig_mode, vector, !result);
@@ -383,11 +389,6 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
 			break;
 		}
 
-		if (trig_mode) {
-			apic_debug("level trig mode for vector %d", vector);
-			apic_set_vector(vector, apic->regs + APIC_TMR);
-		} else
-			apic_clear_vector(vector, apic->regs + APIC_TMR);
 		kvm_vcpu_kick(vcpu);
 		break;
 
-- 
1.6.5.3

Re: [PATCH] KVM: Fix race between APIC TMR and IRR

[Marcelo Tosatti]

On Tue, Dec 29, 2009 at 12:42:16PM +0200, Avi Kivity wrote:
> When we queue an interrupt to the local apic, we set the IRR before the TMR.
> The vcpu can pick up the IRR and inject the interrupt before setting the TMR,
> and perhaps even EOI it, causing incorrect behaviour.
> 
> The race is really insignificant since it can only occur on the first
> interrupt (usually following interrupts will not change TMR), but it's better
> closed than open.

The EOI handler clears TMR, so AFAICS it can occur not only on the first
interrupt.

Applied, thanks.

Re: [PATCH] KVM: Fix race between APIC TMR and IRR

[Avi Kivity]

On 12/30/2009 12:09 AM, Marcelo Tosatti wrote:
> On Tue, Dec 29, 2009 at 12:42:16PM +0200, Avi Kivity wrote:
>    
>> When we queue an interrupt to the local apic, we set the IRR before the TMR.
>> The vcpu can pick up the IRR and inject the interrupt before setting the TMR,
>> and perhaps even EOI it, causing incorrect behaviour.
>>
>> The race is really insignificant since it can only occur on the first
>> interrupt (usually following interrupts will not change TMR), but it's better
>> closed than open.
>>      
> The EOI handler clears TMR, so AFAICS it can occur not only on the first
> interrupt.
>    

In that case, we ought to queue it for .33 and -stable.

-- 
error compiling committee.c: too many arguments to function