KVM: Protect device ops->create and list_add with kvm->lock

commit a28ebea2adc4a2bef5989a5a181ec238f59fbcad upstream.

KVM devices were manipulating list data structures without any form of
synchronization, and some implementations of the create operations also
suffered from a lack of synchronization.

Now when we've split the xics create operation into create and init, we
can hold the kvm->lock mutex while calling the create operation and when
manipulating the devices list.

The error path in the generic code gets slightly ugly because we have to
take the mutex again and delete the device from the list, but holding
the mutex during anon_inode_getfd or releasing/locking the mutex in the
common non-error path seemed wrong.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
[bwh: Backported to 3.16:
 - Drop change to a failure path that doesn't exist in kvm_vgic_create() 
 - Adjust filename, context]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

2 files changed, 14 insertions, 10 deletions

diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
index 24ac123f4ad8..7b490485712c 100644
--- a/virt/kvm/arm/vgic.c
+++ b/virt/kvm/arm/vgic.c
@@ -1624,12 +1624,8 @@ int kvm_vgic_create(struct kvm *kvm)
 	int i, vcpu_lock_idx = -1, ret;
 	struct kvm_vcpu *vcpu;
 
-	mutex_lock(&kvm->lock);
-
-	if (kvm->arch.vgic.vctrl_base) {
-		ret = -EEXIST;
-		goto out;
-	}
+	if (kvm->arch.vgic.vctrl_base)
+		return -EEXIST;
 
 	/*
 	 * Any time a vcpu is run, vcpu_load is called which tries to grab the
@@ -1659,9 +1655,6 @@ out_unlock:
 		vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx);
 		mutex_unlock(&vcpu->mutex);
 	}
-
-out:
-	mutex_unlock(&kvm->lock);
 	return ret;
 }
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d98e6cf1e781..f8cbf5ba6136 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -587,6 +587,11 @@ static void kvm_destroy_devices(struct kvm *kvm)
 {
 	struct list_head *node, *tmp;
 
+	/*
+	 * We do not need to take the kvm->lock here, because nobody else
+	 * has a reference to the struct kvm at this point and therefore
+	 * cannot access the devices list anyhow.
+	 */
 	list_for_each_safe(node, tmp, &kvm->devices) {
 		struct kvm_device *dev =
 			list_entry(node, struct kvm_device, vm_node);
@@ -2322,11 +2327,15 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 	dev->ops = ops;
 	dev->kvm = kvm;
 
+	mutex_lock(&kvm->lock);
 	ret = ops->create(dev, cd->type);
 	if (ret < 0) {
+		mutex_unlock(&kvm->lock);
 		kfree(dev);
 		return ret;
 	}
+	list_add(&dev->vm_node, &kvm->devices);
+	mutex_unlock(&kvm->lock);
 
 	if (ops->init)
 		ops->init(dev);
@@ -2334,10 +2343,12 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
 	ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR | O_CLOEXEC);
 	if (ret < 0) {
 		ops->destroy(dev);
+		mutex_lock(&kvm->lock);
+		list_del(&dev->vm_node);
+		mutex_unlock(&kvm->lock);
 		return ret;
 	}
 
-	list_add(&dev->vm_node, &kvm->devices);
 	kvm_get_kvm(kvm);
 	cd->fd = ret;
 	return 0;