On Fri, Jan 05, 2024, Jacob Xu wrote:
> > + if (kick) {
> > + if (target_vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED)
> > + target_vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> > +
> > + kvm_make_request(KVM_REQ_UPDATE_PROTECTED_GUEST_STATE, target_vcpu);
>
> I think we should switch the order of these two statements for
> setting mp_state and for making the request for
> KVM_REQ_UPDATE_PROTECTED_GUEST_STATE.
> There is a race condition I observed when booting with SVSM where:
> 1. BSP sets target vcpu to KVM_MP_STATE_RUNNABLE
> 2. AP thread within the loop of arch/x86/kvm.c:vcpu_run() checks
> vm_vcpu_running()
> 3. AP enters the guest without having updated the VMSA state from
> KVM_REQ_UPDATE_PROTECTED_GUEST_STATE
>
> This results in the AP executing on a bad RIP and then crashing.
> If we set the request first, then we avoid the race condition.
That just introducs a different race, e.g. if this task gets delayed and the
target vCPU processes KVM_REQ_UPDATE_PROTECTED_GUEST_STATE before its marked
RUNNABLE, then the target vCPU could end up stuck in the UNINITIALIZED loop.
Reading and writing arch.mp_state across vCPUs is simply not safe. There's a
reason why KVM atomically manages INITs and SIPIs and only modifies mp_state when
processing events on the target vCPU.
> > + kvm_vcpu_kick(target_vcpu);
...
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 87b78d63e81d..df9ec357d538 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10858,6 +10858,14 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >
> > if (kvm_check_request(KVM_REQ_UPDATE_CPU_DIRTY_LOGGING, vcpu))
> > static_call(kvm_x86_update_cpu_dirty_logging)(vcpu);
> > +
> > + if (kvm_check_request(KVM_REQ_UPDATE_PROTECTED_GUEST_STATE, vcpu)) {
> > + kvm_vcpu_reset(vcpu, true);
> > + if (vcpu->arch.mp_state != KVM_MP_STATE_RUNNABLE) {
> > + r = 1;
> > + goto out;
> > + }
> > + }
> > }
> >
> > if (kvm_check_request(KVM_REQ_EVENT, vcpu) || req_int_win ||
> > @@ -13072,6 +13080,9 @@ static inline bool kvm_vcpu_has_events(struct kvm_vcpu *vcpu)
> > if (kvm_test_request(KVM_REQ_PMI, vcpu))
> > return true;
> >
> > + if (kvm_test_request(KVM_REQ_UPDATE_PROTECTED_GUEST_STATE, vcpu))
> > + return true;
> > +
> > if (kvm_arch_interrupt_allowed(vcpu) &&
> > (kvm_cpu_has_interrupt(vcpu) ||
> > kvm_guest_apic_has_interrupt(vcpu)))
> > --
> > 2.25.1
> >
> >
