Skip to contentSkip to conversation

kvm: make processes waiting on vcpu mutex killable

PATCH Applied Michael S. Tsirkin 2 participants 2 messages Sep 16, 2012 – Sep 17, 2012

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Michael S. Tsirkin

author
#0 lore raw

PATCH 1/1 kvm: make processes waiting on vcpu mutex killable

vcpu mutex can be held for unlimited time so taking it with mutex_lock on an ioctl is wrong:
one process could be passed a vcpu fd and call this ioctl on the vcpu used by another process, it will then be unkillable until the owner exits.

Call mutex_lock_killable instead and return status.
Note: mutex_lock_interruptible would be even nicer, but I am not sure all users are prepared to handle EINTR from these ioctls. They might misinterpret it as an error.

Cleanup paths expect a vcpu that can't be used by any userspace so this will always succeed - catch bugs by calling BUG_ON.

Catch callers that don't check return state by adding __must_check.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

It's a minor bugfix - should we put it in 3.6?

 arch/x86/kvm/x86.c       | 12 +++++++++---
 include/linux/kvm_host.h |  2 +-
 virt/kvm/kvm_main.c      | 10 +++++++---
 3 files changed, 17 insertions(+), 7 deletions(-)
arch/x86/kvm/x86.c +9 -3
index 06fd97f..c33b046 100644
@@ -6015,7 +6015,9 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 	int r;
 
 	vcpu->arch.mtrr_state.have_fixed = 1;
-	vcpu_load(vcpu);
+	r = vcpu_load(vcpu);
+	if (r)
+		return r;
 	r = kvm_arch_vcpu_reset(vcpu);
 	if (r == 0)
 		r = kvm_mmu_setup(vcpu);

@@ -6026,9 +6028,11 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
 
 void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 {
+	int r;
 	vcpu->arch.apf.msr_val = 0;
 
-	vcpu_load(vcpu);
+	r = vcpu_load(vcpu);
+	BUG_ON(r);
 	kvm_mmu_unload(vcpu);
 	vcpu_put(vcpu);
 

@@ -6269,7 +6273,9 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 
 static void kvm_unload_vcpu_mmu(struct kvm_vcpu *vcpu)
 {
-	vcpu_load(vcpu);
+	int r;
+	r = vcpu_load(vcpu);
+	BUG_ON(r);
 	kvm_mmu_unload(vcpu);
 	vcpu_put(vcpu);
 }
include/linux/kvm_host.h +1 -1
index b70b48b..2a23103 100644
@@ -348,7 +348,7 @@ static inline struct kvm_vcpu *kvm_get_vcpu(struct kvm *kvm, int i)
 int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id);
 void kvm_vcpu_uninit(struct kvm_vcpu *vcpu);
 
-void vcpu_load(struct kvm_vcpu *vcpu);
+int __must_check vcpu_load(struct kvm_vcpu *vcpu);
 void vcpu_put(struct kvm_vcpu *vcpu);
 
 int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
virt/kvm/kvm_main.c +7 -3
index d617f69..a442c17 100644
@@ -137,11 +137,12 @@ inline int kvm_is_mmio_pfn(pfn_t pfn)
 /*
  * Switches to specified vcpu, until a matching vcpu_put()
  */
-void vcpu_load(struct kvm_vcpu *vcpu)
+int vcpu_load(struct kvm_vcpu *vcpu)
 {
 	int cpu;
 
-	mutex_lock(&vcpu->mutex);
+	if (mutex_lock_killable(&vcpu->mutex))
+		return -EINTR;
 	if (unlikely(vcpu->pid != current->pids[PIDTYPE_PID].pid)) {
 		/* The thread running this VCPU changed. */
 		struct pid *oldpid = vcpu->pid;

@@ -154,6 +155,7 @@ void vcpu_load(struct kvm_vcpu *vcpu)
 	preempt_notifier_register(&vcpu->preempt_notifier);
 	kvm_arch_vcpu_load(vcpu, cpu);
 	put_cpu();
+	return 0;
 }
 
 void vcpu_put(struct kvm_vcpu *vcpu)

@@ -1766,7 +1768,9 @@ static long kvm_vcpu_ioctl(struct file *filp,
 #endif
 
 
-	vcpu_load(vcpu);
+	r = vcpu_load(vcpu);
+	if (r)
+		return r;
 	switch (ioctl) {
 	case KVM_RUN:
 		r = -EINVAL;
Signature · 1 line
MST
Applied
#1 lore raw
Quoted 15 lines · Michael S. Tsirkin

vcpu mutex can be held for unlimited time so taking it with mutex_lock on an ioctl is wrong:
one process could be passed a vcpu fd and call this ioctl on the vcpu used by another process, it will then be unkillable until the owner exits.

Call mutex_lock_killable instead and return status.
Note: mutex_lock_interruptible would be even nicer, but I am not sure all users are prepared to handle EINTR from these ioctls. They might misinterpret it as an error.

Cleanup paths expect a vcpu that can't be used by any userspace so this will always succeed - catch bugs by calling BUG_ON.

Quoted 7 lines

Catch callers that don't check return state by adding __must_check.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

It's a minor bugfix - should we put it in 3.6?

Applied to branch 'queue' on the basis its not a regression/critical problem, thanks.