KVM: VMX: fix invalid cpu passed to smp_call_function_single
PATCH 1/2 KVM: VMX: fix invalid cpu passed to smp_call_function_single
In loaded_vmcs_clear, loaded_vmcs->cpu is the fist parameter passed to smp_call_function_single, if the target cpu is downing (doing cpu hot remove), loaded_vmcs->cpu can become -1 then -1 is passed to smp_call_function_single
It can be triggered when vcpu is being destroyed, loaded_vmcs_clear is called in the preemptionable context
--- arch/x86/kvm/vmx.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
arch/x86/kvm/vmx.c +5 -3
@@ -1007,9 +1007,11 @@ static void __loaded_vmcs_clear(void *arg)static void loaded_vmcs_clear (struct loaded_vmcs *loaded_vmcs) { -if (loaded_vmcs->cpu !=-1 ) -smp_call_function_single ( - loaded_vmcs->cpu, __loaded_vmcs_clear, loaded_vmcs,1 ); +int cpu = loaded_vmcs->cpu; + +if (cpu !=-1 ) +smp_call_function_single (cpu, + __loaded_vmcs_clear, loaded_vmcs,1 ); }static inline void vpid_sync_vcpu_single (struct vcpu_vmx *vmx)
Signature · 1 line
1.7.7.6
PATCH 2/2 KVM: VMX: fix memory order between loading vmcs and clearing vmcs
vmcs->cpu indicates whether it exists on the target cpu, -1 means the vmcs does not exist on any vcpu
If vcpu load vmcs with vmcs.cpu = -1, it can be directly added to cpu's percpu list. The list can be corrupted if the cpu prefetch the vmcs's list before reading vmcs->cpu. Meanwhile, we should remove vmcs from the list before making vmcs->vcpu == -1 be visible
--- arch/x86/kvm/vmx.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)
arch/x86/kvm/vmx.c +17 -0
@@ -1002,6 +1002,15 @@ static void __loaded_vmcs_clear(void *arg)if (per_cpu (current_vmcs, cpu) == loaded_vmcs->vmcs)per_cpu (current_vmcs, cpu) =NULL ;list_del (&loaded_vmcs->loaded_vmcss_on_cpu_link); + +/* +* we should ensure updating loaded_vmcs->loaded_vmcss_on_cpu_link +* is before setting loaded_vmcs->vcpu to -1 which is done in +* loaded_vmcs_init. Otherwise, other cpu can see vcpu = -1 fist +* then adds the vmcs into percpu list before it is deleted. +*/ +smp_wmb (); +loaded_vmcs_init (loaded_vmcs); }
@@ -1537,6 +1546,14 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)kvm_make_request (KVM_REQ_TLB_FLUSH , vcpu);local_irq_disable (); + +/* +* Read loaded_vmcs->cpu should be before fetching +* loaded_vmcs->loaded_vmcss_on_cpu_link. +* See the comments in __loaded_vmcs_clear(). +*/ +smp_rmb (); +list_add (&vmx->loaded_vmcs->loaded_vmcss_on_cpu_link, &per_cpu (loaded_vmcss_on_cpu, cpu));local_irq_enable ();
Signature · 1 line
1.7.7.6
Re: [PATCH 2/2] KVM: VMX: fix memory order between loading vmcs and clearing vmcs
Quoted 20 lines · Xiao Guangrong
vmcs->cpuindicates whether it exists on the target cpu, -1 means the vmcs does not exist on any vcpuIf vcpu load vmcs with vmcs.cpu = -1, it can be directly added to cpu's percpu list. The list can be corrupted if the cpu prefetch the vmcs's list before reading
vmcs->cpu. Meanwhile, we should remove vmcs from the list before makingvmcs->vcpu== -1 be visibleSigned-off-by Xiao Guangrong <[email protected]>--- arch/x86/kvm/vmx.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)arch/x86/kvm/vmx.c +0
-0@@ -1002,6 +1002,15 @@ static void __loaded_vmcs_clear(void *arg)if (per_cpu (current_vmcs, cpu) == loaded_vmcs->vmcs)per_cpu (current_vmcs, cpu) =NULL ;
arch/x86/kvm/vmx.c static void __loaded_vmcs_clear(void *arg) · line 1013
Quoted 10 lines
arch/x86/kvm/vmx.c +9
-0list_del (&loaded_vmcs->loaded_vmcss_on_cpu_link); + +/* +* we should ensure updating loaded_vmcs->loaded_vmcss_on_cpu_link +* is before setting loaded_vmcs->vcpu to -1 which is done in +* loaded_vmcs_init. Otherwise, other cpu can see vcpu = -1 fist +* then adds the vmcs into percpu list before it is deleted. +*/ +smp_wmb (); +
Neither loads nor stores are reordered with like operations (see section 8.2.3.2 of intel's volume 3). This behaviour makes the barrier not necessary.
However, i agree access to loaded_vmcs is not obviously safe. I can't tell its safe with vmm_exclusive = 0 (where vcpu->cpu can change at any time).
Re: [PATCH 2/2] KVM: VMX: fix memory order between loading vmcs and clearing vmcs
Quoted 27 lines · Marcelo Tosatti
Quoted 26 lines · Xiao Guangrong
vmcs->cpuindicates whether it exists on the target cpu, -1 means the vmcs does not exist on any vcpuIf vcpu load vmcs with vmcs.cpu = -1, it can be directly added to cpu's percpu list. The list can be corrupted if the cpu prefetch the vmcs's list before reading
vmcs->cpu. Meanwhile, we should remove vmcs from the list before makingvmcs->vcpu== -1 be visibleSigned-off-by Xiao Guangrong <[email protected]>--- arch/x86/kvm/vmx.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)arch/x86/kvm/vmx.c +5
-0@@ -1002,6 +1002,15 @@ static void __loaded_vmcs_clear(void *arg)if (per_cpu (current_vmcs, cpu) == loaded_vmcs->vmcs)per_cpu (current_vmcs, cpu) =NULL ;list_del (&loaded_vmcs->loaded_vmcss_on_cpu_link); + +/* +* we should ensure updating loaded_vmcs->loaded_vmcss_on_cpu_link +* is before setting loaded_vmcs->vcpu to -1 which is done in +* loaded_vmcs_init. Otherwise, other cpu can see vcpu = -1 fist
Quoted 8 lines
arch/x86/kvm/vmx.c static void __loaded_vmcs_clear(void *arg) · line 1013
Quoted 4 lines
arch/x86/kvm/vmx.c +4
-0+* then adds the vmcs into percpu list before it is deleted. +*/ +smp_wmb (); +Neither loads nor stores are reordered with like operations (see section 8.2.3.2 of intel's volume 3). This behaviour makes the barrier not necessary.
Ouch, yes, you are right. My memory is wrong. It seems only later-read can be reordered with early-write.
But if 'read vs read' and 'write vs write' can be guaranteed by CPU, smp_wmb() and smp_rmb() should only be a complier barrier, so i think we can add the barriers to improve the readable and the portable.
And anyway, the current code missed complier-barrier.
Quoted 4 lines
However, i agree access to
loaded_vmcsis not obviously safe. I can't tell its safe withvmm_exclusive= 0 (wherevcpu->cpucan change at any time).
If vmm_exclusive = 0, the vmcs can removed from percpu list when vcpu is scheduled out. The list is not broken.
Re: [PATCH 2/2] KVM: VMX: fix memory order between loading vmcs and clearing vmcs
In reply to Marcelo TosattiQuoted 27 lines · Marcelo Tosatti
Quoted 26 lines · Xiao Guangrong
vmcs->cpuindicates whether it exists on the target cpu, -1 means the vmcs does not exist on any vcpuIf vcpu load vmcs with vmcs.cpu = -1, it can be directly added to cpu's percpu list. The list can be corrupted if the cpu prefetch the vmcs's list before reading
vmcs->cpu. Meanwhile, we should remove vmcs from the list before makingvmcs->vcpu== -1 be visibleSigned-off-by Xiao Guangrong <[email protected]>--- arch/x86/kvm/vmx.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)arch/x86/kvm/vmx.c +5
-0@@ -1002,6 +1002,15 @@ static void __loaded_vmcs_clear(void *arg)if (per_cpu (current_vmcs, cpu) == loaded_vmcs->vmcs)per_cpu (current_vmcs, cpu) =NULL ;list_del (&loaded_vmcs->loaded_vmcss_on_cpu_link); + +/* +* we should ensure updating loaded_vmcs->loaded_vmcss_on_cpu_link +* is before setting loaded_vmcs->vcpu to -1 which is done in +* loaded_vmcs_init. Otherwise, other cpu can see vcpu = -1 fist
Quoted 8 lines
arch/x86/kvm/vmx.c static void __loaded_vmcs_clear(void *arg) · line 1013
Quoted 4 lines
arch/x86/kvm/vmx.c +4
-0+* then adds the vmcs into percpu list before it is deleted. +*/ +smp_wmb (); +Neither loads nor stores are reordered with like operations (see section 8.2.3.2 of intel's volume 3). This behaviour makes the barrier not necessary.
Ouch, yes, you are right. My memory is wrong. It seems only later-read can be reordered with early-write.
But if 'read vs read' and 'write vs write' can be guaranteed by CPU, smp_wmb() and smp_rmb() should act as a complier barrier, so i think we can add the barriers to improve the readable and the portable.
And anyway, the current code missed complier-barrier.
Quoted 4 lines
However, i agree access to
loaded_vmcsis not obviously safe. I can't tell its safe withvmm_exclusive= 0 (wherevcpu->cpucan change at any time).
If vmm_exclusive = 0, the vmcs can removed from percpu list when vcpu is scheduled out. The list is not broken.
Marcelo Tosatti
AppliedRe: [PATCH 2/2] KVM: VMX: fix memory order between loading vmcs and clearing vmcs
In reply to Xiao GuangrongQuoted 8 lines · Xiao Guangrong
vmcs->cpuindicates whether it exists on the target cpu, -1 means the vmcs does not exist on any vcpuIf vcpu load vmcs with vmcs.cpu = -1, it can be directly added to cpu's percpu list. The list can be corrupted if the cpu prefetch the vmcs's list before reading
vmcs->cpu. Meanwhile, we should remove vmcs from the list before makingvmcs->vcpu== -1 be visible
Quoted 4 lines
Signed-off-by Xiao Guangrong <[email protected]>--- arch/x86/kvm/vmx.c | 17 +++++++++++++++++ 1 files changed, 17 insertions(+), 0 deletions(-)
Applied, thanks.
Marcelo Tosatti
AppliedQuoted 11 lines · Xiao Guangrong
In
loaded_vmcs_clear,loaded_vmcs->cpuis the fist parameter passed tosmp_call_function_single, if the target cpu is downing (doing cpu hot remove),loaded_vmcs->cpucan become -1 then -1 is passed tosmp_call_function_singleIt can be triggered when vcpu is being destroyed,
loaded_vmcs_clearis called in the preemptionable contextSigned-off-by Xiao Guangrong <[email protected]>--- arch/x86/kvm/vmx.c | 8 +++++--- 1 files changed, 5 insertions(+), 3 deletions(-)
Applied, thanks.