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

[Gleb Natapov <gleb@redhat.com>]

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.