Re: [PATCH] KVM: nVMX: Disable preemption while reading from shadow VMCS

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2014-10-08 21:07, Bandan Das wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> In order to access the shadow VMCS, we need to load it. At this point,
>> vmx->loaded_vmcs->vmcs and the actually loaded one start to differ. If
>> we now get preempted by Linux, vmx_vcpu_put and, on return, the
>> vmx_vcpu_load will work against the wrong vmcs. That can cause
>> copy_shadow_to_vmcs12 to corrupt the vmcs12 state.
> 
> Ouch! I apologize if I missed this in the previous discussion but why do
> we never get into this condition while running a Linux guest ?

Well, you need high load on the host, preemption at the "wrong" time,
and some relevant difference between the still to-be-copied shadow vmcs
fields between the proper vmcs and the real one that was used by KVM at
that point. I don't think it is a Jailhouse-only issue, but other
hypervisors may be less sensitive.

> 
> Will there be a performance impact of this change ? I hope it's 
> negligible though..

The preemption counter is cheap, the only impact should be on scheduling
latency. But that is irrelevant as long as we have the counterpart under
the same preemption lock.

Jan

> 
>> Fix the issue by disabling preemption during the copy operation.
>>
>> copy_vmcs12_to_shadow is safe from this issue as it is executed by
>> vmx_vcpu_run when preemption is already disabled before vmentry.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> This fixes specifically Jailhouse in KVM on CPUs with shadow VMCS
>> support.
>>
>>  arch/x86/kvm/vmx.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 04fa1b8..f3de106 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -6417,6 +6417,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
>>  	const unsigned long *fields = shadow_read_write_fields;
>>  	const int num_fields = max_shadow_read_write_fields;
>>  
>> +	preempt_disable();
>> +
>>  	vmcs_load(shadow_vmcs);
>>  
>>  	for (i = 0; i < num_fields; i++) {
>> @@ -6440,6 +6442,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
>>  
>>  	vmcs_clear(shadow_vmcs);
>>  	vmcs_load(vmx->loaded_vmcs->vmcs);
>> +
>> +	preempt_enable();
>>  }
>>  
>>  static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

-- 
You received this message because you are subscribed to the Google Groups "Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jailhouse-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.