Messages in this thread | | | Date | Tue, 20 Dec 2022 14:05:17 +0800 | Subject | Re: [PATCH] KVM: Destroy target device if coalesced MMIO unregistration fails | From | Binbin Wu <> |
| |
On 12/20/2022 11:04 AM, Wang, Wei W wrote: > On Tuesday, December 20, 2022 1:19 AM, Sean Christopherson wrote: >> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c index >> 0be80c213f7f..5ef88f5a0864 100644 >> --- a/virt/kvm/coalesced_mmio.c >> +++ b/virt/kvm/coalesced_mmio.c >> @@ -187,15 +187,17 @@ int >> kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm, >> r = kvm_io_bus_unregister_dev(kvm, >> zone->pio ? KVM_PIO_BUS : KVM_MMIO_BUS, &dev->dev); >> >> + kvm_iodevice_destructor(&dev->dev); >> + >> /* >> * On failure, unregister destroys all devices on the >> * bus _except_ the target device, i.e. coalesced_zones >> - * has been modified. No need to restart the walk as >> - * there aren't any zones left. >> + * has been modified. Bail after destroying the target >> + * device, there's no need to restart the walk as there >> + * aren't any zones left. >> */ >> if (r) >> break; >> - kvm_iodevice_destructor(&dev->dev); >> } >> } > Another option is to let kvm_io_bus_unregister_dev handle this, and no need for callers > to make the extra kvm_iodevice_destructor() call. This simplifies the usage for callers > (e.g. reducing LOCs and no leakages like this):
One vote for this option : )
> > diff --git a/include/kvm/iodev.h b/include/kvm/iodev.h > index d75fc4365746..56619e33251e 100644 > --- a/include/kvm/iodev.h > +++ b/include/kvm/iodev.h > @@ -55,10 +55,4 @@ static inline int kvm_iodevice_write(struct kvm_vcpu *vcpu, > : -EOPNOTSUPP; > } > > -static inline void kvm_iodevice_destructor(struct kvm_io_device *dev) > -{ > - if (dev->ops->destructor) > - dev->ops->destructor(dev); > -} > - > #endif /* __KVM_IODEV_H__ */ > diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c > index 0be80c213f7f..d7135a5e76f8 100644 > --- a/virt/kvm/coalesced_mmio.c > +++ b/virt/kvm/coalesced_mmio.c > @@ -195,7 +195,6 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm, > */ > if (r) > break; > - kvm_iodevice_destructor(&dev->dev); > } > } > > diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c > index 2a3ed401ce46..1b277afb545b 100644 > --- a/virt/kvm/eventfd.c > +++ b/virt/kvm/eventfd.c > @@ -898,7 +898,6 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx, > bus = kvm_get_bus(kvm, bus_idx); > if (bus) > bus->ioeventfd_count--; > - ioeventfd_release(p); > ret = 0; > break; > } > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 13e88297f999..582757ebdce6 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -5200,6 +5200,12 @@ static struct notifier_block kvm_reboot_notifier = { > .priority = 0, > }; > > +static void kvm_iodevice_destructor(struct kvm_io_device *dev) > +{ > + if (dev->ops->destructor) > + dev->ops->destructor(dev); > +} > + > static void kvm_io_bus_destroy(struct kvm_io_bus *bus) > { > int i; > @@ -5423,7 +5429,7 @@ int kvm_io_bus_register_dev(struct kvm *kvm, enum kvm_bus bus_idx, gpa_t addr, > int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > struct kvm_io_device *dev) > { > - int i, j; > + int i; > struct kvm_io_bus *new_bus, *bus; > > lockdep_assert_held(&kvm->slots_lock); > @@ -5453,18 +5459,18 @@ int kvm_io_bus_unregister_dev(struct kvm *kvm, enum kvm_bus bus_idx, > rcu_assign_pointer(kvm->buses[bus_idx], new_bus); > synchronize_srcu_expedited(&kvm->srcu); > > - /* Destroy the old bus _after_ installing the (null) bus. */ > + /* > + * If (null) bus is installed, destroy the old bus, including all the > + * attached devices. Otherwise, destroy the caller's device only. > + */ > if (!new_bus) { > pr_err("kvm: failed to shrink bus, removing it completely\n"); > - for (j = 0; j < bus->dev_count; j++) { > - if (j == i) > - continue; > - kvm_iodevice_destructor(bus->range[j].dev); > - } > + kvm_io_bus_destroy(bus); > + return -ENOMEM; > } > > - kfree(bus); > - return new_bus ? 0 : -ENOMEM; > + kvm_iodevice_destructor(dev); > + return 0; > } > > struct kvm_io_device *kvm_io_bus_get_dev(struct kvm *kvm, enum kvm_bus bus_idx,
| |