Re: [PATCH] KVM: VMX: Fix repeatly calling alloc_apic_access_page()

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Sheng Yang wrote:
>
>>>  	return kvm;
>>>  }
>>>
>>> diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
>>> index 42e7fad..89007b2 100644
>>> --- a/drivers/kvm/vmx.c
>>> +++ b/drivers/kvm/vmx.c
>>> @@ -1466,6 +1466,8 @@ static int alloc_apic_access_page(struct kvm *kvm)
>>>  	int r;
>>>
>>>  	r = -EFAULT;
>>> +	if (kvm->apic_access_page)
>>> +		return 0;
>>>       
>> Racy, what if two vcpus are created simultaneously?
>>     
>
> I think it is not racy, for BSP have been created before APs in sequence, and 
> I am ensure only BSP(vcpu id=0) would call this.
>
>   

That's true for qemu.  But we have to be use malicious userspace can't 
crash the kernel or leak memory using the kvm APIs.

>>>  	kvm_userspace_mem.slot = APIC_ACCESS_PAGE_PRIVATE_MEMSLOT;
>>>  	kvm_userspace_mem.flags = 0;
>>>  	kvm_userspace_mem.guest_phys_addr = 0xfee00000ULL;
>>> @@ -1584,7 +1586,8 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
>>>  	vmcs_writel(CR0_GUEST_HOST_MASK, ~0UL);
>>>  	vmcs_writel(CR4_GUEST_HOST_MASK, KVM_GUEST_CR4_MASK);
>>>
>>> -	if (vm_need_virtualize_apic_accesses(vmx->vcpu.kvm))
>>> +	if ((vmx->vcpu.vcpu_id == 0) &&
>>> +	    (vm_need_virtualize_apic_accesses(vmx->vcpu.kvm)))
>>>  		if (alloc_apic_access_page(vmx->vcpu.kvm) != 0)
>>>  			return -ENOMEM;
>>>       
>> We may not have vcpu id 0 (though it's very unlikely).
>>     
>
> Um... I am not quite understand when we will miss vcpu id 0. I think vcpu id 0 
> is used to indicate BSP.
>   

That's what qemu does.  But another userspace perhaps will do this 
differently.

This is a minor problem compared to the one above.

>   
>> I think the problems arise because we are doing a VM-wide operation
>> (memory slot) from a vcpu context.  I think adding a ->vm_create() arch
>> op and allocating the memory there will be better (under kvm->lock).
>>     
>
> Agree, but a little problem remains. 
>
> I have to do feature detection before call allocate function, but 
> kvm_create_vm() is called before kvm_create_irqchip(). So I can't find a 
> proper position for create_vm(). Any suggestion?
>   

You're right, during vm creation we have no idea if we want the 
FlexPriority page.

So you can do this in the same place, just take kvm->mutex and check if 
it exists first (you need to do that anyway), no dependency on vcpu 0.


-- 
Any sufficiently difficult bug is indistinguishable from a feature.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/