From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH RFC] KVM: Fix race in apic->pending_events processing Date: Tue, 28 May 2013 12:56:19 +0200 Message-ID: <51A48D53.7070204@redhat.com> References: <20130526130031.GS4725@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Jan Kiszka To: Gleb Natapov Return-path: Received: from mail-qe0-f49.google.com ([209.85.128.49]:49757 "EHLO mail-qe0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757623Ab3E1K4a (ORCPT ); Tue, 28 May 2013 06:56:30 -0400 Received: by mail-qe0-f49.google.com with SMTP id a11so4188739qen.36 for ; Tue, 28 May 2013 03:56:29 -0700 (PDT) In-Reply-To: <20130526130031.GS4725@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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) { /* evaluate pending_events before reading the vector */ smp_rmb(); (Even if we go with your patch, it deserves a comment in the code). Paolo > Signed-off-by: Gleb Natapov > 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 >