diff mbox

[RFC] KVM: Fix race in apic->pending_events processing

Message ID 51A48D53.7070204@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paolo Bonzini May 28, 2013, 10:56 a.m. UTC
Il 26/05/2013 15:00, Gleb Natapov ha scritto:
> apic->pending_events processing has a race that may cause INIT and SIPI
> processing to be reordered:
> 
> vpu0:                            vcpu1:
> set INIT
>                                test_and_clear_bit(KVM_APIC_INIT)
>                                   process INIT
> set INIT
> set SIPI
>                                test_and_clear_bit(KVM_APIC_SIPI)
>                                   process SIPI
> 
> At the and INIT is left pending in pending_events. The following patch
> tries to fix this using the fact that if INIT comes after SIPI it drops
> SIPI from the pending_events, so if pending_events is different after
> SIPI is processed it means that INIT was issued after SIPI otherwise
> all pending event are processed and pending_events can be reset to zero.

The patch looks correct, but is there any reason to do the cmpxchg at
the end?

That is, would this have any problem?  It seems a bit simpler:


(Even if we go with your patch, it deserves a comment in the code).

Paolo

> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 9d75193..67686b8 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1850,6 +1850,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_lapic *apic = vcpu->arch.apic;
>  	unsigned int sipi_vector;
> +	unsigned long pe;
>  
>  	if (!kvm_vcpu_has_lapic(vcpu))
>  		return;
> @@ -1862,7 +1863,8 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
>  		else
>  			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>  	}
> -	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
> +	pe = apic->pending_events;
> +	if (test_bit(KVM_APIC_SIPI, &pe) &&
>  	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
>  		/* evaluate pending_events before reading the vector */
>  		smp_rmb();
> @@ -1871,6 +1873,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
>  			 vcpu->vcpu_id, sipi_vector);
>  		kvm_vcpu_deliver_sipi_vector(vcpu, sipi_vector);
>  		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> +		cmpxchg(&apic->pending_events, pe, 0);
>  	}
>  }
>  
> --
> 			Gleb.
> --
> 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
> 

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

Comments

Gleb Natapov May 28, 2013, 12:56 p.m. UTC | #1
On Tue, May 28, 2013 at 12:56:19PM +0200, Paolo Bonzini wrote:
> Il 26/05/2013 15:00, Gleb Natapov ha scritto:
> > apic->pending_events processing has a race that may cause INIT and SIPI
> > processing to be reordered:
> > 
> > vpu0:                            vcpu1:
> > set INIT
> >                                test_and_clear_bit(KVM_APIC_INIT)
> >                                   process INIT
> > set INIT
> > set SIPI
> >                                test_and_clear_bit(KVM_APIC_SIPI)
> >                                   process SIPI
> > 
> > At the and INIT is left pending in pending_events. The following patch
> > tries to fix this using the fact that if INIT comes after SIPI it drops
> > SIPI from the pending_events, so if pending_events is different after
> > SIPI is processed it means that INIT was issued after SIPI otherwise
> > all pending event are processed and pending_events can be reset to zero.
> 
> The patch looks correct, but is there any reason to do the cmpxchg at
> the end?
> 
> That is, would this have any problem?  It seems a bit simpler:
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index e1adbb4..3fe00fc 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1873,7 +1873,11 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
>  		else
>  			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>  	}
> -	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
> +	/*
> +	 * Note that we may get another INIT+SIPI sequence right here; process
> +	 * the INIT first.  Assumes that there are only KVM_APIC_INIT/SIPI.
> +	 */
> +	if (cmpxchg(&apic->pending_events, KVM_APIC_SIPI, 0) == KVM_APIC_SIPI &&
>  	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
Because pending_events can be INIT/SIPI at this point and it should be
interpreted as: do SIPI and ignore INIT (atomically).

>  		/* evaluate pending_events before reading the vector */
>  		smp_rmb();
> 
> (Even if we go with your patch, it deserves a comment in the code).
> 
We can move explanation from the commit message to a coment.

> Paolo
> 
> > Signed-off-by: Gleb Natapov <gleb@redhat.com>
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 9d75193..67686b8 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -1850,6 +1850,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
> >  {
> >  	struct kvm_lapic *apic = vcpu->arch.apic;
> >  	unsigned int sipi_vector;
> > +	unsigned long pe;
> >  
> >  	if (!kvm_vcpu_has_lapic(vcpu))
> >  		return;
> > @@ -1862,7 +1863,8 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
> >  		else
> >  			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
> >  	}
> > -	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
> > +	pe = apic->pending_events;
> > +	if (test_bit(KVM_APIC_SIPI, &pe) &&
> >  	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
> >  		/* evaluate pending_events before reading the vector */
> >  		smp_rmb();
> > @@ -1871,6 +1873,7 @@ void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
> >  			 vcpu->vcpu_id, sipi_vector);
> >  		kvm_vcpu_deliver_sipi_vector(vcpu, sipi_vector);
> >  		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> > +		cmpxchg(&apic->pending_events, pe, 0);
> >  	}
> >  }
> >  
> > --
> > 			Gleb.
> > --
> > 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
> > 

--
			Gleb.
--
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 mbox

Patch

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index e1adbb4..3fe00fc 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1873,7 +1873,11 @@  void kvm_apic_accept_events(struct kvm_vcpu *vcpu)
 		else
 			vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
 	}
-	if (test_and_clear_bit(KVM_APIC_SIPI, &apic->pending_events) &&
+	/*
+	 * Note that we may get another INIT+SIPI sequence right here; process
+	 * the INIT first.  Assumes that there are only KVM_APIC_INIT/SIPI.
+	 */
+	if (cmpxchg(&apic->pending_events, KVM_APIC_SIPI, 0) == KVM_APIC_SIPI &&
 	    vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED) {
 		/* evaluate pending_events before reading the vector */
 		smp_rmb();