Re: [PATCH] KVM: VMX: fix vmwrite to invalid VMCS

[Paolo Bonzini <pbonzini@redhat.com>]

On 07/07/2015 15:50, Radim Krčmář wrote:

>> Andrey reported offlist that the bug went away by reverting 1cde293.  So
>> the patch would at least need a new commit message. :)
> 
> I think it's a different bug than the one Andrey reproduced
> (https://bugzilla.kernel.org/show_bug.cgi?id=100671).
> I'll send a v2 that cleans up the code and makes the commit message
> clearer, unless you find the reasoning below unsound.

Yes, the patch is okay.  The problem is that kvm-arch_vcpu_create is
called from a VM ioctl and thus is not between vcpu_load and vcpu_put.

Thanks, I applied it.

Paolo

> This bug is specific to 'kvm_arch_vcpu_create()' and Vlastimil Holer hit
> it on RHEL 7.2 (278.el7) kernel that didn't have 1cde2930e154
> ("sched/preempt: Add static_key() to preempt_notifiers").
> 
> The commit message does not base on tracing (I haven't reproduced it),
> but I couldn't make sense out of this bug otherwise.
> I think it happens just because other VCPU preempted the new one between
> vmx_vcpu_put()+put_cpu() and the end of kvm_x86_ops->fpu_activate(), so
> vmwrite accessed different VMCS.  The code in kvm_vm_ioctl_create_vcpu()
> that made me think so:
> 
>   vcpu = kvm_arch_vcpu_create(id) {
>     vcpu = kvm_x86_ops->vcpu_create(kvm, id) {
>       vmx = kmem_cache_zalloc(kvm_vcpu_cache, GFP_KERNEL);
>       kvm_vcpu_init(&vmx->vcpu, kvm, id);
>       vmx->loaded_vmcs = &vmx->vmcs01;
>       vmx->loaded_vmcs->vmcs = alloc_vmcs();
>       loaded_vmcs_init(vmx->loaded_vmcs);
> 
>       // disabling preemption and activating VMCS
>       cpu = get_cpu();
>       vmx_vcpu_load(&vmx->vcpu, cpu);
> 
>       vmx_vcpu_setup(vmx);
> 
>       // abandoning VMCS and enabling preemption
>       vmx_vcpu_put(&vmx->vcpu);
>       put_cpu();
> 
>       return &vmx->vcpu;
>     }
> 
>     // enabled preemption and undefined current VMCS
>     kvm_x86_ops->fpu_activate(vcpu);
>     return vcpu;
>   }
> 
>   preempt_notifier_init(&vcpu->preempt_notifier, &kvm_preempt_ops);
>   kvm_arch_vcpu_setup(vcpu) {
>     vcpu_load(vcpu);
>     ...
>   }
>