Re: KVM: x86: Racy mp_state manipulations

[Paolo Bonzini <pbonzini@redhat.com>]

Il 03/03/2013 17:48, Jan Kiszka ha scritto:
> Hi all,
> 
> KVM's mp_state on x86 is usually manipulated over the context of the
> VCPU. Therefore, no locking is required. There are unfortunately two
> exceptions, and one of them is definitely broken: INIT and SIPI delivery.
> 
> The lapic may set mp_state over the context of the sending VCPU. For
> SIPI, it first checks if the mp_state is INIT_RECEIVED before updating
> it to SIPI_RECEIVED. We can only race here with user space setting the
> state in parallel, I suppose. Probably harmless in practice.

Still it would be better to add an smp_wmb/smp_rmb pair between accesses
of mp_state and sipi_vector.

Also, Io
> What is critical is the update on INIT. That signal is asynchronous to
> the target VCPU state. And we can loose it:
> 
> vcpu 1				vcpu 2
> ------				------
> hlt;
> vmexit
> 				__apic_accept_irq(APIC_DM_INIT)
> 				mp_state = KVM_MP_STATE_INIT_RECEIVED
> mp_state = KVM_MP_STATE_HALTED
> 
> And there it goes, our INIT state. I've triggered this under heavy INIT
> load and my nVMX patch for processing it while in VMXON.
> 
> I'm currently considering options to fix this:
> 
> - through a lock at mp_state manipulations, check under the lock that
>   we don't perform invalid state transitions (e.g. INIT->HLT)
> - signal the INIT via some KVM_REQ_INIT to the target VCPU, fully
>   localizing mp_state updates, the same could be done with SIPI, just
>   to play safe
> 
> I'm leaning toward the latter ATM, Any thoughts or other idea?

The latter makes sense since it's not a fast path, but the only
transition that is acceptable to KVM_MP_STATE_HALTED is from
KVM_MP_STATE_RUNNABLE:

from \ to    RUNNABLE     UNINIT      INIT     HALTED       SIPI
RUNNABLE       n/a          yes        yes       yes         NO
UNINIT         NO           n/a        yes       NO          NO
INIT           NO           yes        n/a       NO          yes
HALTED         yes          yes        yes       n/a         NO
SIPI           yes          yes        yes       NO          n/a

so for this particular bug it should also work to use a cmpxchg when
setting KVM_MP_STATE_HALTED.  Same for INIT->SIPI, since writes to
sipi_vector are harmless.

BTW, what happens when you send an INIT IPI to the bootstrap processor?
 This may be interesting if we want to emulate soft resets correctly in
QEMU; KVM makes it go to wait-for-SIPI state if I read the code
correctly, but that is wrong.

Paolo