* [PATCH 0/2] KVM: Synchronize KVM devices list access and create ops @ 2016-08-09 12:20 Christoffer Dall 2016-08-09 12:20 ` [PATCH 1/2] KVM: PPC: Move xics_debugfs_init out of create Christoffer Dall 2016-08-09 12:20 ` [PATCH 2/2] KVM: Protect device ops->create and list_add with kvm->lock Christoffer Dall 0 siblings, 2 replies; 8+ messages in thread From: Christoffer Dall @ 2016-08-09 12:20 UTC (permalink / raw) To: kvm Cc: Andre Przywara, Paolo Bonzini, Radim Krčmář, Alexander Graf, borntraeger, paulus, kvmarm, Christoffer Dall Currently accesses the kvm->devices list is not synchronized by any mechanism which can potentially lead to data corruption. Further, a number of the create operations on the individual devices are racy and would allow creation of multiple devices, opposite to the intention. Factor out portions of the XICS create operation into a separate init operation and protect the remaining list accesses and create operations with the kvm->lock. Tested on arm/arm64 and compile-tested on powerpc for the xics changes. Tested-by on other archs would be appreciated. Christoffer Dall (2): KVM: PPC: Move xics_debugfs_init out of create KVM: Protect device ops->create and list_add with kvm->lock arch/arm/kvm/arm.c | 6 +++++- arch/powerpc/kvm/book3s_xics.c | 12 ++++++++---- include/linux/kvm_host.h | 6 ++++++ virt/kvm/arm/vgic/vgic-init.c | 17 ++++------------- virt/kvm/kvm_main.c | 16 +++++++++++++++- 5 files changed, 38 insertions(+), 19 deletions(-) -- 2.9.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] KVM: PPC: Move xics_debugfs_init out of create 2016-08-09 12:20 [PATCH 0/2] KVM: Synchronize KVM devices list access and create ops Christoffer Dall @ 2016-08-09 12:20 ` Christoffer Dall 2016-08-09 12:20 ` [PATCH 2/2] KVM: Protect device ops->create and list_add with kvm->lock Christoffer Dall 1 sibling, 0 replies; 8+ messages in thread From: Christoffer Dall @ 2016-08-09 12:20 UTC (permalink / raw) To: kvm Cc: Andre Przywara, Paolo Bonzini, Radim Krčmář, Alexander Graf, borntraeger, paulus, kvmarm, Christoffer Dall As we are about to hold the kvm->lock during the create operation on KVM devices, we should move the call to xics_debugfs_init into its own function, since holding a mutex over extended amounts of time might not be a good idea. Introduce an init operation on the kvm_device_ops struct which cannot fail and call this, if configured, after the device has been created. Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> --- arch/powerpc/kvm/book3s_xics.c | 10 ++++++++-- include/linux/kvm_host.h | 6 ++++++ virt/kvm/kvm_main.c | 3 +++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c index a75ba38..f2def8e 100644 --- a/arch/powerpc/kvm/book3s_xics.c +++ b/arch/powerpc/kvm/book3s_xics.c @@ -1341,8 +1341,6 @@ static int kvmppc_xics_create(struct kvm_device *dev, u32 type) return ret; } - xics_debugfs_init(xics); - #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE if (cpu_has_feature(CPU_FTR_ARCH_206)) { /* Enable real mode support */ @@ -1354,9 +1352,17 @@ static int kvmppc_xics_create(struct kvm_device *dev, u32 type) return 0; } +static void kvmppc_xics_init(struct kvm_device *dev) +{ + struct kvmppc_xics *xics = (struct kvmppc_xics *)dev->private; + + xics_debugfs_init(xics); +} + struct kvm_device_ops kvm_xics_ops = { .name = "kvm-xics", .create = kvmppc_xics_create, + .init = kvmppc_xics_init, .destroy = kvmppc_xics_free, .set_attr = xics_set_attr, .get_attr = xics_get_attr, diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 01e908a..d3c9b82 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1116,6 +1116,12 @@ struct kvm_device_ops { int (*create)(struct kvm_device *dev, u32 type); /* + * init is called after create if create is successful and is called + * outside of holding kvm->lock. + */ + void (*init)(struct kvm_device *dev); + + /* * Destroy is responsible for freeing dev. * * Destroy may be called before or after destructors are called diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index cc081cc..ae64245 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2838,6 +2838,9 @@ static int kvm_ioctl_create_device(struct kvm *kvm, return ret; } + if (ops->init) + ops->init(dev); + ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR | O_CLOEXEC); if (ret < 0) { ops->destroy(dev); -- 2.9.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] KVM: Protect device ops->create and list_add with kvm->lock 2016-08-09 12:20 [PATCH 0/2] KVM: Synchronize KVM devices list access and create ops Christoffer Dall 2016-08-09 12:20 ` [PATCH 1/2] KVM: PPC: Move xics_debugfs_init out of create Christoffer Dall @ 2016-08-09 12:20 ` Christoffer Dall 2016-08-09 12:37 ` Paolo Bonzini 1 sibling, 1 reply; 8+ messages in thread From: Christoffer Dall @ 2016-08-09 12:20 UTC (permalink / raw) To: kvm Cc: Andre Przywara, Paolo Bonzini, Radim Krčmář, Alexander Graf, borntraeger, paulus, kvmarm, Christoffer Dall 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> --- arch/arm/kvm/arm.c | 6 +++++- arch/powerpc/kvm/book3s_xics.c | 2 -- virt/kvm/arm/vgic/vgic-init.c | 17 ++++------------- virt/kvm/kvm_main.c | 13 ++++++++++++- 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index d94bb90..75f130e 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -1009,9 +1009,13 @@ long kvm_arch_vm_ioctl(struct file *filp, switch (ioctl) { case KVM_CREATE_IRQCHIP: { + int ret; if (!vgic_present) return -ENXIO; - return kvm_vgic_create(kvm, KVM_DEV_TYPE_ARM_VGIC_V2); + mutex_lock(&kvm->lock); + ret = kvm_vgic_create(kvm, KVM_DEV_TYPE_ARM_VGIC_V2); + mutex_unlock(&kvm->lock); + return ret; } case KVM_ARM_SET_DEVICE_ADDR: { struct kvm_arm_device_addr dev_addr; diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c index f2def8e..05aa113 100644 --- a/arch/powerpc/kvm/book3s_xics.c +++ b/arch/powerpc/kvm/book3s_xics.c @@ -1329,12 +1329,10 @@ static int kvmppc_xics_create(struct kvm_device *dev, u32 type) xics->kvm = kvm; /* Already there ? */ - mutex_lock(&kvm->lock); if (kvm->arch.xics) ret = -EEXIST; else kvm->arch.xics = xics; - mutex_unlock(&kvm->lock); if (ret) { kfree(xics); diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c index fb4b0a7..83777c1 100644 --- a/virt/kvm/arm/vgic/vgic-init.c +++ b/virt/kvm/arm/vgic/vgic-init.c @@ -73,12 +73,8 @@ int kvm_vgic_create(struct kvm *kvm, u32 type) int i, vcpu_lock_idx = -1, ret; struct kvm_vcpu *vcpu; - mutex_lock(&kvm->lock); - - if (irqchip_in_kernel(kvm)) { - ret = -EEXIST; - goto out; - } + if (irqchip_in_kernel(kvm)) + return -EEXIST; /* * This function is also called by the KVM_CREATE_IRQCHIP handler, @@ -87,10 +83,8 @@ int kvm_vgic_create(struct kvm *kvm, u32 type) * the proper checks already. */ if (type == KVM_DEV_TYPE_ARM_VGIC_V2 && - !kvm_vgic_global_state.can_emulate_gicv2) { - ret = -ENODEV; - goto out; - } + !kvm_vgic_global_state.can_emulate_gicv2) + return -ENODEV; /* * Any time a vcpu is run, vcpu_load is called which tries to grab the @@ -138,9 +132,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 ae64245..1950782 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -696,6 +696,11 @@ static void kvm_destroy_devices(struct kvm *kvm) { struct kvm_device *dev, *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_entry_safe(dev, tmp, &kvm->devices, vm_node) { list_del(&dev->vm_node); dev->ops->destroy(dev); @@ -2832,11 +2837,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); @@ -2844,10 +2853,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; -- 2.9.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] KVM: Protect device ops->create and list_add with kvm->lock 2016-08-09 12:20 ` [PATCH 2/2] KVM: Protect device ops->create and list_add with kvm->lock Christoffer Dall @ 2016-08-09 12:37 ` Paolo Bonzini 2016-08-09 12:55 ` Christoffer Dall 0 siblings, 1 reply; 8+ messages in thread From: Paolo Bonzini @ 2016-08-09 12:37 UTC (permalink / raw) To: Christoffer Dall, kvm; +Cc: Andre Przywara, paulus, borntraeger, kvmarm On 09/08/2016 14:20, Christoffer Dall wrote: > 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> Very nice (and small), but please add a comment to the create member in kvm_device_ops. Thanks, Paolo > --- > arch/arm/kvm/arm.c | 6 +++++- > arch/powerpc/kvm/book3s_xics.c | 2 -- > virt/kvm/arm/vgic/vgic-init.c | 17 ++++------------- > virt/kvm/kvm_main.c | 13 ++++++++++++- > 4 files changed, 21 insertions(+), 17 deletions(-) > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index d94bb90..75f130e 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -1009,9 +1009,13 @@ long kvm_arch_vm_ioctl(struct file *filp, > > switch (ioctl) { > case KVM_CREATE_IRQCHIP: { > + int ret; > if (!vgic_present) > return -ENXIO; > - return kvm_vgic_create(kvm, KVM_DEV_TYPE_ARM_VGIC_V2); > + mutex_lock(&kvm->lock); > + ret = kvm_vgic_create(kvm, KVM_DEV_TYPE_ARM_VGIC_V2); > + mutex_unlock(&kvm->lock); > + return ret; > } > case KVM_ARM_SET_DEVICE_ADDR: { > struct kvm_arm_device_addr dev_addr; > diff --git a/arch/powerpc/kvm/book3s_xics.c b/arch/powerpc/kvm/book3s_xics.c > index f2def8e..05aa113 100644 > --- a/arch/powerpc/kvm/book3s_xics.c > +++ b/arch/powerpc/kvm/book3s_xics.c > @@ -1329,12 +1329,10 @@ static int kvmppc_xics_create(struct kvm_device *dev, u32 type) > xics->kvm = kvm; > > /* Already there ? */ > - mutex_lock(&kvm->lock); > if (kvm->arch.xics) > ret = -EEXIST; > else > kvm->arch.xics = xics; > - mutex_unlock(&kvm->lock); > > if (ret) { > kfree(xics); > diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c > index fb4b0a7..83777c1 100644 > --- a/virt/kvm/arm/vgic/vgic-init.c > +++ b/virt/kvm/arm/vgic/vgic-init.c > @@ -73,12 +73,8 @@ int kvm_vgic_create(struct kvm *kvm, u32 type) > int i, vcpu_lock_idx = -1, ret; > struct kvm_vcpu *vcpu; > > - mutex_lock(&kvm->lock); > - > - if (irqchip_in_kernel(kvm)) { > - ret = -EEXIST; > - goto out; > - } > + if (irqchip_in_kernel(kvm)) > + return -EEXIST; > > /* > * This function is also called by the KVM_CREATE_IRQCHIP handler, > @@ -87,10 +83,8 @@ int kvm_vgic_create(struct kvm *kvm, u32 type) > * the proper checks already. > */ > if (type == KVM_DEV_TYPE_ARM_VGIC_V2 && > - !kvm_vgic_global_state.can_emulate_gicv2) { > - ret = -ENODEV; > - goto out; > - } > + !kvm_vgic_global_state.can_emulate_gicv2) > + return -ENODEV; > > /* > * Any time a vcpu is run, vcpu_load is called which tries to grab the > @@ -138,9 +132,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 ae64245..1950782 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -696,6 +696,11 @@ static void kvm_destroy_devices(struct kvm *kvm) > { > struct kvm_device *dev, *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_entry_safe(dev, tmp, &kvm->devices, vm_node) { > list_del(&dev->vm_node); > dev->ops->destroy(dev); > @@ -2832,11 +2837,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); > @@ -2844,10 +2853,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; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] KVM: Protect device ops->create and list_add with kvm->lock 2016-08-09 12:37 ` Paolo Bonzini @ 2016-08-09 12:55 ` Christoffer Dall 2016-08-09 13:16 ` Paolo Bonzini 0 siblings, 1 reply; 8+ messages in thread From: Christoffer Dall @ 2016-08-09 12:55 UTC (permalink / raw) To: Paolo Bonzini Cc: kvm, Andre Przywara, Radim Krčmář, Alexander Graf, borntraeger, paulus, kvmarm On Tue, Aug 09, 2016 at 02:37:43PM +0200, Paolo Bonzini wrote: > > > On 09/08/2016 14:20, Christoffer Dall wrote: > > 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> > > Very nice (and small), but please add a comment to the create member in > kvm_device_ops. Like this?: diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index d3c9b82..9c28b4d 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1113,6 +1113,12 @@ struct kvm_device { /* create, destroy, and name are mandatory */ struct kvm_device_ops { const char *name; + + /* + * create is called holding kvm->lock and any operations not suitable + * to do while holding the lock should be deferred to init (see + * below). + */ int (*create)(struct kvm_device *dev, u32 type); /* Thanks, -Christoffer ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] KVM: Protect device ops->create and list_add with kvm->lock 2016-08-09 12:55 ` Christoffer Dall @ 2016-08-09 13:16 ` Paolo Bonzini 2016-08-09 14:49 ` Christoffer Dall 0 siblings, 1 reply; 8+ messages in thread From: Paolo Bonzini @ 2016-08-09 13:16 UTC (permalink / raw) To: Christoffer Dall Cc: kvm, Andre Przywara, Radim Krčmář, Alexander Graf, borntraeger, paulus, kvmarm On 09/08/2016 14:55, Christoffer Dall wrote: > On Tue, Aug 09, 2016 at 02:37:43PM +0200, Paolo Bonzini wrote: >> >> >> On 09/08/2016 14:20, Christoffer Dall wrote: >>> 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> >> >> Very nice (and small), but please add a comment to the create member in >> kvm_device_ops. > > Like this?: > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index d3c9b82..9c28b4d 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1113,6 +1113,12 @@ struct kvm_device { > /* create, destroy, and name are mandatory */ > struct kvm_device_ops { > const char *name; > + > + /* > + * create is called holding kvm->lock and any operations not suitable > + * to do while holding the lock should be deferred to init (see > + * below). > + */ > int (*create)(struct kvm_device *dev, u32 type); > > /* > That's okay, series Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] KVM: Protect device ops->create and list_add with kvm->lock 2016-08-09 13:16 ` Paolo Bonzini @ 2016-08-09 14:49 ` Christoffer Dall 2016-08-09 15:20 ` Paolo Bonzini 0 siblings, 1 reply; 8+ messages in thread From: Christoffer Dall @ 2016-08-09 14:49 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kvm, Andre Przywara, paulus, borntraeger, kvmarm On Tue, Aug 09, 2016 at 03:16:26PM +0200, Paolo Bonzini wrote: > > > On 09/08/2016 14:55, Christoffer Dall wrote: > > On Tue, Aug 09, 2016 at 02:37:43PM +0200, Paolo Bonzini wrote: > >> > >> > >> On 09/08/2016 14:20, Christoffer Dall wrote: > >>> 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> > >> > >> Very nice (and small), but please add a comment to the create member in > >> kvm_device_ops. > > > > Like this?: > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index d3c9b82..9c28b4d 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -1113,6 +1113,12 @@ struct kvm_device { > > /* create, destroy, and name are mandatory */ > > struct kvm_device_ops { > > const char *name; > > + > > + /* > > + * create is called holding kvm->lock and any operations not suitable > > + * to do while holding the lock should be deferred to init (see > > + * below). > > + */ > > int (*create)(struct kvm_device *dev, u32 type); > > > > /* > > > > That's okay, series > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Thanks, I'll send a v2. Will you just apply the patches to kvm/master or would you like me to include it in my pull request for -rc2 ? Also, do you want to wait for a tested-by from the other arch maintainers? -Christoffer ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] KVM: Protect device ops->create and list_add with kvm->lock 2016-08-09 14:49 ` Christoffer Dall @ 2016-08-09 15:20 ` Paolo Bonzini 0 siblings, 0 replies; 8+ messages in thread From: Paolo Bonzini @ 2016-08-09 15:20 UTC (permalink / raw) To: Christoffer Dall; +Cc: kvm, Andre Przywara, paulus, borntraeger, kvmarm On 09/08/2016 16:49, Christoffer Dall wrote: > On Tue, Aug 09, 2016 at 03:16:26PM +0200, Paolo Bonzini wrote: >> >> >> On 09/08/2016 14:55, Christoffer Dall wrote: >>> On Tue, Aug 09, 2016 at 02:37:43PM +0200, Paolo Bonzini wrote: >>>> >>>> >>>> On 09/08/2016 14:20, Christoffer Dall wrote: >>>>> 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> >>>> >>>> Very nice (and small), but please add a comment to the create member in >>>> kvm_device_ops. >>> >>> Like this?: >>> >>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h >>> index d3c9b82..9c28b4d 100644 >>> --- a/include/linux/kvm_host.h >>> +++ b/include/linux/kvm_host.h >>> @@ -1113,6 +1113,12 @@ struct kvm_device { >>> /* create, destroy, and name are mandatory */ >>> struct kvm_device_ops { >>> const char *name; >>> + >>> + /* >>> + * create is called holding kvm->lock and any operations not suitable >>> + * to do while holding the lock should be deferred to init (see >>> + * below). >>> + */ >>> int (*create)(struct kvm_device *dev, u32 type); >>> >>> /* >>> >> >> That's okay, series >> >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > > Thanks, I'll send a v2. Will you just apply the patches to kvm/master > or would you like me to include it in my pull request for -rc2 ? The tree is currently in Radim's hands, but I expect this to be applied directly by one of us. > Also, do you want to wait for a tested-by from the other arch > maintainers? That would be nice, but not mandatory. Paolo ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-08-09 15:20 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-08-09 12:20 [PATCH 0/2] KVM: Synchronize KVM devices list access and create ops Christoffer Dall 2016-08-09 12:20 ` [PATCH 1/2] KVM: PPC: Move xics_debugfs_init out of create Christoffer Dall 2016-08-09 12:20 ` [PATCH 2/2] KVM: Protect device ops->create and list_add with kvm->lock Christoffer Dall 2016-08-09 12:37 ` Paolo Bonzini 2016-08-09 12:55 ` Christoffer Dall 2016-08-09 13:16 ` Paolo Bonzini 2016-08-09 14:49 ` Christoffer Dall 2016-08-09 15:20 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).