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.
Finally, I change the list_add to a list_add_rcu and change the vfio
code to use list_for_each_entry_rcu to safely iterate the list. This
could also be done holding the devices lock, but we don't know if all
callers can hold a spinlock while iterating the list, so RCU seems to
be an elegant solution.
Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
---
include/linux/kvm_host.h | 1 +
virt/kvm/kvm_main.c | 14 +++++++++++++-
virt/kvm/vfio.c | 2 +-
3 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index aafd702..dd20af9 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;
+ spinlock_t 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..7cfee41 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);
+ spin_lock_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,13 @@ static int kvm_ioctl_create_device(struct kvm *kvm,
return ret;
}
- list_add(&dev->vm_node, &kvm->devices);
+ /*
+ * Take the devices_lock to synchronize multiple adders.
+ * Use list_add_rcu to allow lockless traversal of this list.
+ */
+ spin_lock(&kvm->devices_lock);
+ list_add_rcu(&dev->vm_node, &kvm->devices);
+ spin_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..4470fa1 100644
--- a/virt/kvm/vfio.c
+++ b/virt/kvm/vfio.c
@@ -268,7 +268,7 @@ static int kvm_vfio_create(struct kvm_device *dev, u32 type)
struct kvm_vfio *kv;
/* Only one VFIO "device" per VM */
- list_for_each_entry(tmp, &dev->kvm->devices, vm_node)
+ list_for_each_entry_rcu(tmp, &dev->kvm->devices, vm_node)
if (tmp->ops == &kvm_vfio_ops)
return -EBUSY;
--
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
