The list of KVM devices is not currently protected with any lock. As
far as I can see nothing prevents multiple threads from creating devices
on the KVM fd simultaenously, potentially leading to corruption.
Further, we already have VFIO looping over this list without any form of
synchronization, and we are about to add similar functionality for the
ITS on arm64.
Note that the traversal in kvm_destroy_devices does not appear to be a
problem because only the last reference to the KVM struct can actually
perform this action.
I use a mutex to protect the list, since the arm64 ITS code is going to
need to iterate over the devices (to find all ITSes) and register KVM IO
devices for each one, which in turn grabs another mutex and kmallocs
stuff.
Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
---
Changes since v1:
- My RCU knowledge was too rusty and was both incorrectly implemented
and would not address the arm64 need. Just use a mutex instead.
include/linux/kvm_host.h | 1 +
virt/kvm/kvm_main.c | 8 ++++++++
virt/kvm/vfio.c | 11 +++++++++--
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index aafd702..de32196 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -421,6 +421,7 @@ struct kvm {
long mmu_notifier_count;
#endif
long tlbs_dirty;
+ struct mutex devices_lock; /* protects the devices list */
struct list_head devices;
struct dentry *debugfs_dentry;
struct kvm_stat_data **debugfs_stat_data;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 61b31a5..33e0579 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -616,6 +616,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
mutex_init(&kvm->irq_lock);
mutex_init(&kvm->slots_lock);
atomic_set(&kvm->users_count, 1);
+ mutex_init(&kvm->devices_lock);
INIT_LIST_HEAD(&kvm->devices);
r = kvm_arch_init_vm(kvm, type);
@@ -694,6 +695,11 @@ static void kvm_destroy_devices(struct kvm *kvm)
{
struct kvm_device *dev, *tmp;
+ /*
+ * We do not need to take the devices_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_entry_safe(dev, tmp, &kvm->devices, vm_node) {
list_del(&dev->vm_node);
dev->ops->destroy(dev);
@@ -2842,7 +2848,9 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
return ret;
}
+ mutex_lock(&kvm->devices_lock);
list_add(&dev->vm_node, &kvm->devices);
+ mutex_unlock(&kvm->devices_lock);
kvm_get_kvm(kvm);
cd->fd = ret;
return 0;
diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
index 1dd087d..ce5f859 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -266,11 +266,18 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type)
{
struct kvm_device *tmp;
struct kvm_vfio *kv;
+ int err = 0;
/* Only one VFIO "device" per VM */
+ mutex_lock(&dev->kvm->devices_lock);
list_for_each_entry(tmp, &dev->kvm->devices, vm_node)
- if (tmp->ops == &kvm_vfio_ops)
- return -EBUSY;
+ if (tmp->ops == &kvm_vfio_ops) {
+ err = -EBUSY;
+ break;
+ }
+ mutex_unlock(&dev->kvm->devices_lock);
+ if (err)
+ return err;
kv = kzalloc(sizeof(*kv), GFP_KERNEL);
if (!kv)
--
2.9.0
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
