On Wed, Aug 03, 2016 at 01:24:41PM +0200, Paolo Bonzini wrote:
>
>
> On 03/08/2016 12:39, Christoffer Dall wrote:
> > 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);
>
> This is not enough for the VFIO test to be robust. I think it's easier
> to move ops->create entirely under the kvm_lock. This requires some
> auditing of the create functions, but many of them (e.g.
> kvm_vgic_create) already use that lock.
>
> The only complex case is xics. It takes kvm_lock only for part of the
> initialization, and the call to xics_debugfs_init would be better moved
> outside kvm_lock. For that we can add an ops->init(dev) function that
> is called outside kvm_lock and cannot fail.
>
Why do we need to move xics_debugfs_init outside of kvm->lock()? Merely
because it could sleep forever and we don't want that to block
everything or does it loop around and deadlock somehow that I cannot
easily see?
Thanks,
-Christoffer
--
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
