Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1438143 > unrolled thread

[PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest

Started byWanpeng Li <kernellwp@gmail.com>
First post2016-07-07 05:50 +0200
Last post2016-07-07 09:30 +0200
Articles 11 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest Wanpeng Li <kernellwp@gmail.com> - 2016-07-07 05:50 +0200
    [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it Wanpeng Li <kernellwp@gmail.com> - 2016-07-07 05:50 +0200
      Re: [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02  even if L1 doesn't enable it Paolo Bonzini <pbonzini@redhat.com> - 2016-07-07 10:20 +0200
        Re: [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02  even if L1 doesn't enable it Wanpeng Li <kernellwp@gmail.com> - 2016-07-07 10:40 +0200
          Re: [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02  even if L1 doesn't enable it Paolo Bonzini <pbonzini@redhat.com> - 2016-07-07 12:40 +0200
            Re: [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02  even if L1 doesn't enable it Wanpeng Li <kernellwp@gmail.com> - 2016-07-07 13:00 +0200
    Re: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit  in nested guest Haozhong Zhang <haozhong.zhang@intel.com> - 2016-07-07 08:50 +0200
      Re: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit  in nested guest Wanpeng Li <kernellwp@gmail.com> - 2016-07-07 09:00 +0200
        Re: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit  in nested guest Haozhong Zhang <haozhong.zhang@intel.com> - 2016-07-07 09:10 +0200
          Re: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit  in nested guest Wanpeng Li <kernellwp@gmail.com> - 2016-07-07 09:10 +0200
            Re: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit  in nested guest Haozhong Zhang <haozhong.zhang@intel.com> - 2016-07-07 09:30 +0200

#1438143 — [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest

FromWanpeng Li <kernellwp@gmail.com>
Date2016-07-07 05:50 +0200
Subject[PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest
Message-ID<rS8u5-1rw-1@gated-at.bofh.it>
From: Wanpeng Li <wanpeng.li@hotmail.com>

BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<          (null)>]           (null)
PGD 0
Oops: 0010 [#1] SMP
Call Trace:
 ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
 handle_preemption_timer+0xe/0x20 [kvm_intel]
 vmx_handle_exit+0x169/0x15a0 [kvm_intel]
 ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
 kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
 ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
 ? vcpu_load+0x1c/0x60 [kvm]
 ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
 kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
 do_vfs_ioctl+0x96/0x6a0
 ? __fget_light+0x2a/0x90
 SyS_ioctl+0x79/0x90
 do_syscall_64+0x68/0x180
 entry_SYSCALL64_slow_path+0x25/0x25
Code:  Bad RIP value.
RIP  [<          (null)>]           (null)
 RSP <ffff8800b5263c48>
CR2: 0000000000000000
---[ end trace 9c70c48b1a2bc66e ]---

This can be reproduced readily by preemption timer enabled on L0 and disabled
on L1.

Preemption timer for nested VMX is emulated by hrtimer which is started on L2
entry, stopped on L2 exit and evaluated via the check_nested_events hook. However,
nested_vmx_exit_handled is always return true for preemption timer vmexit, then
the L1 preemption timer vmexit is captured and be treated as a L2 preemption
timer vmexit, incurr a nested vmexit dereference NULL pointer.

This patch fix it by depending on check_nested_events to capture L2 preemption
timer(emulated hrtimer) expire and nested vmexit.

Tested-by: Haozhong Zhang <haozhong.zhang@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Yunhong Jiang <yunhong.jiang@intel.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Haozhong Zhang <haozhong.zhang@intel.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v2 -> v3:
 * update patch subject
v1 -> v2:
 * fix typo in patch description

 arch/x86/kvm/vmx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 85e2f0a..29c16a8 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8041,6 +8041,8 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
 		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
 	case EXIT_REASON_PCOMMIT:
 		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_PCOMMIT);
+	case EXIT_REASON_PREEMPTION_TIMER:
+		return false;
 	default:
 		return true;
 	}
-- 
1.9.1

[toc] | [next] | [standalone]

#1438146 — [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

FromWanpeng Li <kernellwp@gmail.com>
Date2016-07-07 05:50 +0200
Subject[PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it
Message-ID<rS8u5-1rw-7@gated-at.bofh.it>
In reply to#1438143
From: Wanpeng Li <wanpeng.li@hotmail.com>

We will go to vcpu_run() loop after L0 emulates VMRESUME which incurs 
kvm_sched_out and kvm_sched_in operations since cond_resched() will be 
called once need resched. Preemption timer will be reprogrammed if vCPU 
is scheduled to a different pCPU. Then the preemption timer bit of vmcs02 
will be set if L0 enable preemption timer to run L1 even if L1 doesn't 
enable preemption timer to run L2.

This patch fix it by don't reprogram preemption timer of vmcs02 if L1's 
vCPU is scheduled on diffent pCPU when we are in the way to vmresume 
nested guest.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Yunhong Jiang <yunhong.jiang@intel.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Haozhong Zhang <haozhong.zhang@intel.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
 arch/x86/kvm/x86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0cc6cf8..e8fe16a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2742,7 +2742,8 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		if (tsc_delta < 0)
 			mark_tsc_unstable("KVM discovered backwards TSC");
 
-		if (kvm_lapic_hv_timer_in_use(vcpu) &&
+		if (!is_guest_mode(vcpu) &&
+			kvm_lapic_hv_timer_in_use(vcpu) &&
 				kvm_x86_ops->set_hv_timer(vcpu,
 					kvm_get_lapic_tscdeadline_msr(vcpu)))
 			kvm_lapic_switch_to_sw_timer(vcpu);
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1438317 — Re: [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-07-07 10:20 +0200
SubjectRe: [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it
Message-ID<rScHp-4lY-65@gated-at.bofh.it>
In reply to#1438146

On 07/07/2016 05:46, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> We will go to vcpu_run() loop after L0 emulates VMRESUME which incurs 
> kvm_sched_out and kvm_sched_in operations since cond_resched() will be 
> called once need resched. Preemption timer will be reprogrammed if vCPU 
> is scheduled to a different pCPU. Then the preemption timer bit of vmcs02 
> will be set if L0 enable preemption timer to run L1 even if L1 doesn't 
> enable preemption timer to run L2.
> 
> This patch fix it by don't reprogram preemption timer of vmcs02 if L1's 
> vCPU is scheduled on diffent pCPU when we are in the way to vmresume 
> nested guest.

Again, this is wrong.  There is no reason why L1's APIC timer cannot be
emulated through the vmcs12's preemption timer setting.  The only issue
is getting the pin-based execution controls right.

Paolo

[toc] | [prev] | [next] | [standalone]

#1438360 — Re: [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

FromWanpeng Li <kernellwp@gmail.com>
Date2016-07-07 10:40 +0200
SubjectRe: [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it
Message-ID<rSd0K-4uy-35@gated-at.bofh.it>
In reply to#1438317
2016-07-07 16:10 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 07/07/2016 05:46, Wanpeng Li wrote:
>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>> We will go to vcpu_run() loop after L0 emulates VMRESUME which incurs
>> kvm_sched_out and kvm_sched_in operations since cond_resched() will be
>> called once need resched. Preemption timer will be reprogrammed if vCPU
>> is scheduled to a different pCPU. Then the preemption timer bit of vmcs02
>> will be set if L0 enable preemption timer to run L1 even if L1 doesn't
>> enable preemption timer to run L2.
>>
>> This patch fix it by don't reprogram preemption timer of vmcs02 if L1's
>> vCPU is scheduled on diffent pCPU when we are in the way to vmresume
>> nested guest.
>
> Again, this is wrong.  There is no reason why L1's APIC timer cannot be
> emulated through the vmcs12's preemption timer setting.  The only issue
> is getting the pin-based execution controls right.

This patch doesn't intend to implement "L1 TSC deadline timer to
trigger while L2 is running", it just solves why vmcs02 is set even if

exec_control = vmcs12->pin_based_vm_exec_control;
exec_control |= vmcs_config.pin_based_exec_ctrl;
exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;

We should set pin-based execution controls right to implement "L1 TSC
deadline timer to trigger while L2 is running".

Regards,
Wanpeng Li

[toc] | [prev] | [next] | [standalone]

#1438514 — Re: [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-07-07 12:40 +0200
SubjectRe: [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it
Message-ID<rSeSR-5H4-15@gated-at.bofh.it>
In reply to#1438360

On 07/07/2016 10:31, Wanpeng Li wrote:
> 2016-07-07 16:10 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>
>>
>> On 07/07/2016 05:46, Wanpeng Li wrote:
>>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>>
>>> We will go to vcpu_run() loop after L0 emulates VMRESUME which incurs
>>> kvm_sched_out and kvm_sched_in operations since cond_resched() will be
>>> called once need resched. Preemption timer will be reprogrammed if vCPU
>>> is scheduled to a different pCPU. Then the preemption timer bit of vmcs02
>>> will be set if L0 enable preemption timer to run L1 even if L1 doesn't
>>> enable preemption timer to run L2.
>>>
>>> This patch fix it by don't reprogram preemption timer of vmcs02 if L1's
>>> vCPU is scheduled on diffent pCPU when we are in the way to vmresume
>>> nested guest.
>>
>> Again, this is wrong.  There is no reason why L1's APIC timer cannot be
>> emulated through the vmcs12's preemption timer setting.  The only issue
>> is getting the pin-based execution controls right.
> 
> This patch doesn't intend to implement "L1 TSC deadline timer to
> trigger while L2 is running", it just solves why vmcs02 is set even if
> 
> exec_control = vmcs12->pin_based_vm_exec_control;
> exec_control |= vmcs_config.pin_based_exec_ctrl;
> exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
> 
> We should set pin-based execution controls right to implement "L1 TSC
> deadline timer to trigger while L2 is running".

Ok, now I get it, but I still cannot understand the logic in your patch.

You write:

		if (!is_guest_mode(vcpu) &&
			kvm_lapic_hv_timer_in_use(vcpu) &&
 				kvm_x86_ops->set_hv_timer(vcpu,
 					kvm_get_lapic_tscdeadline_msr(vcpu)))
 			kvm_lapic_switch_to_sw_timer(vcpu);

but this means that while L2 runs you miss L1's APIC timer interrupt.  Do you
want this instead:

		if (kvm_lapic_hv_timer_in_use(vcpu) &&
		    (is_guest_mode(vcpu) ||
 		     kvm_x86_ops->set_hv_timer(vcpu,
 					       kvm_get_lapic_tscdeadline_msr(vcpu))))
 			kvm_lapic_switch_to_sw_timer(vcpu);

?

Thanks,

Paolo

[toc] | [prev] | [next] | [standalone]

#1438522 — Re: [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it

FromWanpeng Li <kernellwp@gmail.com>
Date2016-07-07 13:00 +0200
SubjectRe: [PATCH v3 2/2] KVM: nVMX: Fix preemption timer bit set in vmcs02 even if L1 doesn't enable it
Message-ID<rSfcf-5Oc-45@gated-at.bofh.it>
In reply to#1438514
2016-07-07 18:33 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 07/07/2016 10:31, Wanpeng Li wrote:
>> 2016-07-07 16:10 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>>
>>>
>>> On 07/07/2016 05:46, Wanpeng Li wrote:
>>>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>>>
>>>> We will go to vcpu_run() loop after L0 emulates VMRESUME which incurs
>>>> kvm_sched_out and kvm_sched_in operations since cond_resched() will be
>>>> called once need resched. Preemption timer will be reprogrammed if vCPU
>>>> is scheduled to a different pCPU. Then the preemption timer bit of vmcs02
>>>> will be set if L0 enable preemption timer to run L1 even if L1 doesn't
>>>> enable preemption timer to run L2.
>>>>
>>>> This patch fix it by don't reprogram preemption timer of vmcs02 if L1's
>>>> vCPU is scheduled on diffent pCPU when we are in the way to vmresume
>>>> nested guest.
>>>
>>> Again, this is wrong.  There is no reason why L1's APIC timer cannot be
>>> emulated through the vmcs12's preemption timer setting.  The only issue
>>> is getting the pin-based execution controls right.
>>
>> This patch doesn't intend to implement "L1 TSC deadline timer to
>> trigger while L2 is running", it just solves why vmcs02 is set even if
>>
>> exec_control = vmcs12->pin_based_vm_exec_control;
>> exec_control |= vmcs_config.pin_based_exec_ctrl;
>> exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>
>> We should set pin-based execution controls right to implement "L1 TSC
>> deadline timer to trigger while L2 is running".
>
> Ok, now I get it, but I still cannot understand the logic in your patch.
>
> You write:
>
>                 if (!is_guest_mode(vcpu) &&
>                         kvm_lapic_hv_timer_in_use(vcpu) &&
>                                 kvm_x86_ops->set_hv_timer(vcpu,
>                                         kvm_get_lapic_tscdeadline_msr(vcpu)))
>                         kvm_lapic_switch_to_sw_timer(vcpu);
>
> but this means that while L2 runs you miss L1's APIC timer interrupt.  Do you
> want this instead:
>
>                 if (kvm_lapic_hv_timer_in_use(vcpu) &&
>                     (is_guest_mode(vcpu) ||
>                      kvm_x86_ops->set_hv_timer(vcpu,
>                                                kvm_get_lapic_tscdeadline_msr(vcpu))))
>                         kvm_lapic_switch_to_sw_timer(vcpu);

Nice point out. :) I will send out another version.

Regards,
Wanpeng Li

[toc] | [prev] | [next] | [standalone]

#1438201 — Re: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest

FromHaozhong Zhang <haozhong.zhang@intel.com>
Date2016-07-07 08:50 +0200
SubjectRe: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest
Message-ID<rSbii-3kT-15@gated-at.bofh.it>
In reply to#1438143
On 07/07/16 11:46, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
> 
> BUG: unable to handle kernel NULL pointer dereference at           (null)
> IP: [<          (null)>]           (null)
> PGD 0
> Oops: 0010 [#1] SMP
> Call Trace:
>  ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
>  handle_preemption_timer+0xe/0x20 [kvm_intel]
>  vmx_handle_exit+0x169/0x15a0 [kvm_intel]
>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>  kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>  ? vcpu_load+0x1c/0x60 [kvm]
>  ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
>  kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
>  do_vfs_ioctl+0x96/0x6a0
>  ? __fget_light+0x2a/0x90
>  SyS_ioctl+0x79/0x90
>  do_syscall_64+0x68/0x180
>  entry_SYSCALL64_slow_path+0x25/0x25
> Code:  Bad RIP value.
> RIP  [<          (null)>]           (null)
>  RSP <ffff8800b5263c48>
> CR2: 0000000000000000
> ---[ end trace 9c70c48b1a2bc66e ]---
> 
> This can be reproduced readily by preemption timer enabled on L0 and disabled
> on L1.
> 
> Preemption timer for nested VMX is emulated by hrtimer which is started on L2
> entry, stopped on L2 exit and evaluated via the check_nested_events hook. However,
> nested_vmx_exit_handled is always return true for preemption timer vmexit, then
> the L1 preemption timer vmexit is captured and be treated as a L2 preemption
> timer vmexit, incurr a nested vmexit dereference NULL pointer.
> 
> This patch fix it by depending on check_nested_events to capture L2 preemption
> timer(emulated hrtimer) expire and nested vmexit.
> 
> Tested-by: Haozhong Zhang <haozhong.zhang@intel.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Yunhong Jiang <yunhong.jiang@intel.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: Haozhong Zhang <haozhong.zhang@intel.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v2 -> v3:
>  * update patch subject
> v1 -> v2:
>  * fix typo in patch description
> 
>  arch/x86/kvm/vmx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 85e2f0a..29c16a8 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8041,6 +8041,8 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
>  		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
>  	case EXIT_REASON_PCOMMIT:
>  		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_PCOMMIT);
> +	case EXIT_REASON_PREEMPTION_TIMER:
> +		return false;

If patch 2 can avoid accidentally enabling preemption timer in vmcs02,
will this one still be needed?

Haozhong


>  	default:
>  		return true;
>  	}
> -- 
> 1.9.1
>

[toc] | [prev] | [next] | [standalone]

#1438207 — Re: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest

FromWanpeng Li <kernellwp@gmail.com>
Date2016-07-07 09:00 +0200
SubjectRe: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest
Message-ID<rSbrX-3oi-9@gated-at.bofh.it>
In reply to#1438201
2016-07-07 14:48 GMT+08:00 Haozhong Zhang <haozhong.zhang@intel.com>:
> On 07/07/16 11:46, Wanpeng Li wrote:
>> From: Wanpeng Li <wanpeng.li@hotmail.com>
>>
>> BUG: unable to handle kernel NULL pointer dereference at           (null)
>> IP: [<          (null)>]           (null)
>> PGD 0
>> Oops: 0010 [#1] SMP
>> Call Trace:
>>  ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
>>  handle_preemption_timer+0xe/0x20 [kvm_intel]
>>  vmx_handle_exit+0x169/0x15a0 [kvm_intel]
>>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>>  kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
>>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>>  ? vcpu_load+0x1c/0x60 [kvm]
>>  ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
>>  kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
>>  do_vfs_ioctl+0x96/0x6a0
>>  ? __fget_light+0x2a/0x90
>>  SyS_ioctl+0x79/0x90
>>  do_syscall_64+0x68/0x180
>>  entry_SYSCALL64_slow_path+0x25/0x25
>> Code:  Bad RIP value.
>> RIP  [<          (null)>]           (null)
>>  RSP <ffff8800b5263c48>
>> CR2: 0000000000000000
>> ---[ end trace 9c70c48b1a2bc66e ]---
>>
>> This can be reproduced readily by preemption timer enabled on L0 and disabled
>> on L1.
>>
>> Preemption timer for nested VMX is emulated by hrtimer which is started on L2
>> entry, stopped on L2 exit and evaluated via the check_nested_events hook. However,
>> nested_vmx_exit_handled is always return true for preemption timer vmexit, then
>> the L1 preemption timer vmexit is captured and be treated as a L2 preemption
>> timer vmexit, incurr a nested vmexit dereference NULL pointer.
>>
>> This patch fix it by depending on check_nested_events to capture L2 preemption
>> timer(emulated hrtimer) expire and nested vmexit.
>>
>> Tested-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> Cc: Yunhong Jiang <yunhong.jiang@intel.com>
>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>> Cc: Haozhong Zhang <haozhong.zhang@intel.com>
>> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> ---
>> v2 -> v3:
>>  * update patch subject
>> v1 -> v2:
>>  * fix typo in patch description
>>
>>  arch/x86/kvm/vmx.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 85e2f0a..29c16a8 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -8041,6 +8041,8 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
>>               return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
>>       case EXIT_REASON_PCOMMIT:
>>               return nested_cpu_has2(vmcs12, SECONDARY_EXEC_PCOMMIT);
>> +     case EXIT_REASON_PREEMPTION_TIMER:
>> +             return false;
>
> If patch 2 can avoid accidentally enabling preemption timer in vmcs02,
> will this one still be needed?

After complete "L1 TSC deadline timer to trigger while L2 is running",
L0's preemption timer fire when L2 is running can result in
(is_guest_mode(vcpu) && nested_vmx_exit_handled(vcpu)) be true, right?

Regards,
Wanpeng Li

[toc] | [prev] | [next] | [standalone]

#1438208 — Re: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest

FromHaozhong Zhang <haozhong.zhang@intel.com>
Date2016-07-07 09:10 +0200
SubjectRe: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest
Message-ID<rSbBD-3GT-7@gated-at.bofh.it>
In reply to#1438207
On 07/07/16 14:56, Wanpeng Li wrote:
> 2016-07-07 14:48 GMT+08:00 Haozhong Zhang <haozhong.zhang@intel.com>:
> > On 07/07/16 11:46, Wanpeng Li wrote:
> >> From: Wanpeng Li <wanpeng.li@hotmail.com>
> >>
> >> BUG: unable to handle kernel NULL pointer dereference at           (null)
> >> IP: [<          (null)>]           (null)
> >> PGD 0
> >> Oops: 0010 [#1] SMP
> >> Call Trace:
> >>  ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
> >>  handle_preemption_timer+0xe/0x20 [kvm_intel]
> >>  vmx_handle_exit+0x169/0x15a0 [kvm_intel]
> >>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
> >>  kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
> >>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
> >>  ? vcpu_load+0x1c/0x60 [kvm]
> >>  ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
> >>  kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
> >>  do_vfs_ioctl+0x96/0x6a0
> >>  ? __fget_light+0x2a/0x90
> >>  SyS_ioctl+0x79/0x90
> >>  do_syscall_64+0x68/0x180
> >>  entry_SYSCALL64_slow_path+0x25/0x25
> >> Code:  Bad RIP value.
> >> RIP  [<          (null)>]           (null)
> >>  RSP <ffff8800b5263c48>
> >> CR2: 0000000000000000
> >> ---[ end trace 9c70c48b1a2bc66e ]---
> >>
> >> This can be reproduced readily by preemption timer enabled on L0 and disabled
> >> on L1.
> >>
> >> Preemption timer for nested VMX is emulated by hrtimer which is started on L2
> >> entry, stopped on L2 exit and evaluated via the check_nested_events hook. However,
> >> nested_vmx_exit_handled is always return true for preemption timer vmexit, then
> >> the L1 preemption timer vmexit is captured and be treated as a L2 preemption
> >> timer vmexit, incurr a nested vmexit dereference NULL pointer.
> >>
> >> This patch fix it by depending on check_nested_events to capture L2 preemption
> >> timer(emulated hrtimer) expire and nested vmexit.
> >>
> >> Tested-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> Cc: Radim Krčmář <rkrcmar@redhat.com>
> >> Cc: Yunhong Jiang <yunhong.jiang@intel.com>
> >> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> >> Cc: Haozhong Zhang <haozhong.zhang@intel.com>
> >> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> >> ---
> >> v2 -> v3:
> >>  * update patch subject
> >> v1 -> v2:
> >>  * fix typo in patch description
> >>
> >>  arch/x86/kvm/vmx.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> index 85e2f0a..29c16a8 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -8041,6 +8041,8 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
> >>               return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
> >>       case EXIT_REASON_PCOMMIT:
> >>               return nested_cpu_has2(vmcs12, SECONDARY_EXEC_PCOMMIT);
> >> +     case EXIT_REASON_PREEMPTION_TIMER:
> >> +             return false;
> >
> > If patch 2 can avoid accidentally enabling preemption timer in vmcs02,
> > will this one still be needed?
> 
> After complete "L1 TSC deadline timer to trigger while L2 is running",
> L0's preemption timer fire when L2 is running can result in
> (is_guest_mode(vcpu) && nested_vmx_exit_handled(vcpu)) be true, right?
>

In prepare_vmcs02():

    exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
    ...
    vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, exec_control);

so preemption timer will never be enabled while L2 guest is running.

Haozhong

[toc] | [prev] | [next] | [standalone]

#1438209 — Re: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest

FromWanpeng Li <kernellwp@gmail.com>
Date2016-07-07 09:10 +0200
SubjectRe: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest
Message-ID<rSbBD-3GT-5@gated-at.bofh.it>
In reply to#1438208
2016-07-07 15:02 GMT+08:00 Haozhong Zhang <haozhong.zhang@intel.com>:
> On 07/07/16 14:56, Wanpeng Li wrote:
>> 2016-07-07 14:48 GMT+08:00 Haozhong Zhang <haozhong.zhang@intel.com>:
>> > On 07/07/16 11:46, Wanpeng Li wrote:
>> >> From: Wanpeng Li <wanpeng.li@hotmail.com>
>> >>
>> >> BUG: unable to handle kernel NULL pointer dereference at           (null)
>> >> IP: [<          (null)>]           (null)
>> >> PGD 0
>> >> Oops: 0010 [#1] SMP
>> >> Call Trace:
>> >>  ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
>> >>  handle_preemption_timer+0xe/0x20 [kvm_intel]
>> >>  vmx_handle_exit+0x169/0x15a0 [kvm_intel]
>> >>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>> >>  kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
>> >>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>> >>  ? vcpu_load+0x1c/0x60 [kvm]
>> >>  ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
>> >>  kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
>> >>  do_vfs_ioctl+0x96/0x6a0
>> >>  ? __fget_light+0x2a/0x90
>> >>  SyS_ioctl+0x79/0x90
>> >>  do_syscall_64+0x68/0x180
>> >>  entry_SYSCALL64_slow_path+0x25/0x25
>> >> Code:  Bad RIP value.
>> >> RIP  [<          (null)>]           (null)
>> >>  RSP <ffff8800b5263c48>
>> >> CR2: 0000000000000000
>> >> ---[ end trace 9c70c48b1a2bc66e ]---
>> >>
>> >> This can be reproduced readily by preemption timer enabled on L0 and disabled
>> >> on L1.
>> >>
>> >> Preemption timer for nested VMX is emulated by hrtimer which is started on L2
>> >> entry, stopped on L2 exit and evaluated via the check_nested_events hook. However,
>> >> nested_vmx_exit_handled is always return true for preemption timer vmexit, then
>> >> the L1 preemption timer vmexit is captured and be treated as a L2 preemption
>> >> timer vmexit, incurr a nested vmexit dereference NULL pointer.
>> >>
>> >> This patch fix it by depending on check_nested_events to capture L2 preemption
>> >> timer(emulated hrtimer) expire and nested vmexit.
>> >>
>> >> Tested-by: Haozhong Zhang <haozhong.zhang@intel.com>
>> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> >> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> >> Cc: Yunhong Jiang <yunhong.jiang@intel.com>
>> >> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>> >> Cc: Haozhong Zhang <haozhong.zhang@intel.com>
>> >> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
>> >> ---
>> >> v2 -> v3:
>> >>  * update patch subject
>> >> v1 -> v2:
>> >>  * fix typo in patch description
>> >>
>> >>  arch/x86/kvm/vmx.c | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> >> index 85e2f0a..29c16a8 100644
>> >> --- a/arch/x86/kvm/vmx.c
>> >> +++ b/arch/x86/kvm/vmx.c
>> >> @@ -8041,6 +8041,8 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
>> >>               return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
>> >>       case EXIT_REASON_PCOMMIT:
>> >>               return nested_cpu_has2(vmcs12, SECONDARY_EXEC_PCOMMIT);
>> >> +     case EXIT_REASON_PREEMPTION_TIMER:
>> >> +             return false;
>> >
>> > If patch 2 can avoid accidentally enabling preemption timer in vmcs02,
>> > will this one still be needed?
>>
>> After complete "L1 TSC deadline timer to trigger while L2 is running",
>> L0's preemption timer fire when L2 is running can result in
>> (is_guest_mode(vcpu) && nested_vmx_exit_handled(vcpu)) be true, right?
>>
>
> In prepare_vmcs02():
>
>     exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>     ...
>     vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, exec_control);
>
> so preemption timer will never be enabled while L2 guest is running.

It is not true after Paolo's patch.

Regards,
Wanpeng Li

[toc] | [prev] | [next] | [standalone]

#1438226 — Re: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest

FromHaozhong Zhang <haozhong.zhang@intel.com>
Date2016-07-07 09:30 +0200
SubjectRe: [PATCH v3 1/2] KVM: nVMX: Fix incorrect preemption timer vmexit in nested guest
Message-ID<rSbUZ-3OW-29@gated-at.bofh.it>
In reply to#1438209
On 07/07/16 15:07, Wanpeng Li wrote:
> 2016-07-07 15:02 GMT+08:00 Haozhong Zhang <haozhong.zhang@intel.com>:
> > On 07/07/16 14:56, Wanpeng Li wrote:
> >> 2016-07-07 14:48 GMT+08:00 Haozhong Zhang <haozhong.zhang@intel.com>:
> >> > On 07/07/16 11:46, Wanpeng Li wrote:
> >> >> From: Wanpeng Li <wanpeng.li@hotmail.com>
> >> >>
> >> >> BUG: unable to handle kernel NULL pointer dereference at           (null)
> >> >> IP: [<          (null)>]           (null)
> >> >> PGD 0
> >> >> Oops: 0010 [#1] SMP
> >> >> Call Trace:
> >> >>  ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
> >> >>  handle_preemption_timer+0xe/0x20 [kvm_intel]
> >> >>  vmx_handle_exit+0x169/0x15a0 [kvm_intel]
> >> >>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
> >> >>  kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
> >> >>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
> >> >>  ? vcpu_load+0x1c/0x60 [kvm]
> >> >>  ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
> >> >>  kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
> >> >>  do_vfs_ioctl+0x96/0x6a0
> >> >>  ? __fget_light+0x2a/0x90
> >> >>  SyS_ioctl+0x79/0x90
> >> >>  do_syscall_64+0x68/0x180
> >> >>  entry_SYSCALL64_slow_path+0x25/0x25
> >> >> Code:  Bad RIP value.
> >> >> RIP  [<          (null)>]           (null)
> >> >>  RSP <ffff8800b5263c48>
> >> >> CR2: 0000000000000000
> >> >> ---[ end trace 9c70c48b1a2bc66e ]---
> >> >>
> >> >> This can be reproduced readily by preemption timer enabled on L0 and disabled
> >> >> on L1.
> >> >>
> >> >> Preemption timer for nested VMX is emulated by hrtimer which is started on L2
> >> >> entry, stopped on L2 exit and evaluated via the check_nested_events hook. However,
> >> >> nested_vmx_exit_handled is always return true for preemption timer vmexit, then
> >> >> the L1 preemption timer vmexit is captured and be treated as a L2 preemption
> >> >> timer vmexit, incurr a nested vmexit dereference NULL pointer.
> >> >>
> >> >> This patch fix it by depending on check_nested_events to capture L2 preemption
> >> >> timer(emulated hrtimer) expire and nested vmexit.
> >> >>
> >> >> Tested-by: Haozhong Zhang <haozhong.zhang@intel.com>
> >> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> >> Cc: Radim Krčmář <rkrcmar@redhat.com>
> >> >> Cc: Yunhong Jiang <yunhong.jiang@intel.com>
> >> >> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> >> >> Cc: Haozhong Zhang <haozhong.zhang@intel.com>
> >> >> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> >> >> ---
> >> >> v2 -> v3:
> >> >>  * update patch subject
> >> >> v1 -> v2:
> >> >>  * fix typo in patch description
> >> >>
> >> >>  arch/x86/kvm/vmx.c | 2 ++
> >> >>  1 file changed, 2 insertions(+)
> >> >>
> >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> >> index 85e2f0a..29c16a8 100644
> >> >> --- a/arch/x86/kvm/vmx.c
> >> >> +++ b/arch/x86/kvm/vmx.c
> >> >> @@ -8041,6 +8041,8 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
> >> >>               return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
> >> >>       case EXIT_REASON_PCOMMIT:
> >> >>               return nested_cpu_has2(vmcs12, SECONDARY_EXEC_PCOMMIT);
> >> >> +     case EXIT_REASON_PREEMPTION_TIMER:
> >> >> +             return false;
> >> >
> >> > If patch 2 can avoid accidentally enabling preemption timer in vmcs02,
> >> > will this one still be needed?
> >>
> >> After complete "L1 TSC deadline timer to trigger while L2 is running",
> >> L0's preemption timer fire when L2 is running can result in
> >> (is_guest_mode(vcpu) && nested_vmx_exit_handled(vcpu)) be true, right?
> >>
> >
> > In prepare_vmcs02():
> >
> >     exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
> >     ...
> >     vmcs_write32(PIN_BASED_VM_EXEC_CONTROL, exec_control);
> >
> > so preemption timer will never be enabled while L2 guest is running.
> 
> It is not true after Paolo's patch.
>

OK, I didn't quite follow discussions before v3 and just noticed
Paolo's patch in v2 comments is to enable L1 preemption timer while L2
is running. Then this one looks reasonable.

Thanks,
Haozhong

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web