* [PATCH v2 0/2] kvm_io_bus_unregister_dev cleanup
@ 2023-02-07 12:37 Wei Wang
2023-02-07 12:37 ` [PATCH v2 1/2] KVM: destruct kvm_io_device while unregistering it from kvm_io_bus Wei Wang
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Wei Wang @ 2023-02-07 12:37 UTC (permalink / raw)
To: pbonzini, seanjc, mhal; +Cc: kvm, linux-kernel, Wei Wang
This patchset moves kvm io_device destruction into
kvm_io_bus_unregister_dev. This reduces LOCs a bit for users and can
avoid the leakage of destructing the device explicitly.
Accordingly, below cleanups are included:
- remove the exposure of kvm_iodevice_destructor and the invocation in
the users as kvm_iodevice_destructor is now invoked in
kvm_io_bus_unregister_dev;
- Change kvm_deassign_ioeventfd_idx to use list_for_each_entry as the
loop ends at the entry that's founded and deleted.
The patches are rebased to
https://github.com/kvm-x86/linux/commit/b1cb1fac22ab
Changelog:
v1->v2:
- keep kfree(bus) when the new bus is successfully allocated
- add patch 2
Previous version:
https://lore.kernel.org/lkml/20221229123302.4083-1-wei.w.wang@intel.com/
Wei Wang (2):
KVM: destruct kvm_io_device while unregistering it from kvm_io_bus
kvm/eventfd: use list_for_each_entry when deassign ioeventfd
include/kvm/iodev.h | 6 ------
virt/kvm/coalesced_mmio.c | 9 ++-------
virt/kvm/eventfd.c | 6 ++----
virt/kvm/kvm_main.c | 23 +++++++++++++++--------
4 files changed, 19 insertions(+), 25 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2 1/2] KVM: destruct kvm_io_device while unregistering it from kvm_io_bus 2023-02-07 12:37 [PATCH v2 0/2] kvm_io_bus_unregister_dev cleanup Wei Wang @ 2023-02-07 12:37 ` Wei Wang 2023-03-23 15:43 ` Sean Christopherson 2023-02-07 12:37 ` [PATCH v2 2/2] kvm/eventfd: use list_for_each_entry when deassign ioeventfd Wei Wang 2023-06-13 23:21 ` [PATCH v2 0/2] kvm_io_bus_unregister_dev cleanup Sean Christopherson 2 siblings, 1 reply; 6+ messages in thread From: Wei Wang @ 2023-02-07 12:37 UTC (permalink / raw) To: pbonzini, seanjc, mhal; +Cc: kvm, linux-kernel, Wei Wang Current usage of kvm_io_device requires users to destruct it with an extra call of kvm_iodevice_destructor after the device gets unregistered from kvm_io_bus. This is not necessary and can cause errors if a user forgot to make the extra call. Simplify the usage by combining kvm_iodevice_destructor into kvm_io_bus_unregister_dev. This reduces LOCs a bit for users and can avoid the leakage of destructing the device explicitly. Signed-off-by: Wei Wang <wei.w.wang@intel.com> --- include/kvm/iodev.h | 6 ------ virt/kvm/coalesced_mmio.c | 9 ++------- virt/kvm/eventfd.c | 1 - virt/kvm/kvm_main.c | 23 +++++++++++++++-------- 4 files changed, 17 insertions(+), 22 deletions(-) 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 5ef88f5a0864..1b90acb6e3fe 100644 --- a/virt/kvm/coalesced_mmio.c +++ b/virt/kvm/coalesced_mmio.c @@ -186,15 +186,10 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm, coalesced_mmio_in_range(dev, zone->addr, zone->size)) { 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. Bail after destroying the target - * device, there's no need to restart the walk as there - * aren't any zones left. + * bus, including the target device. There's no need + * to restart the walk as there aren't any zones left. */ if (r) break; 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 71cc63640173..ce1f2c5b7c87 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -5275,6 +5275,12 @@ static void hardware_disable_all(void) } #endif /* CONFIG_KVM_GENERIC_HARDWARE_ENABLING */ +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; @@ -5498,7 +5504,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); @@ -5528,18 +5534,19 @@ 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; } + kvm_iodevice_destructor(dev); kfree(bus); - return new_bus ? 0 : -ENOMEM; + return 0; } struct kvm_io_device *kvm_io_bus_get_dev(struct kvm *kvm, enum kvm_bus bus_idx, -- 2.27.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] KVM: destruct kvm_io_device while unregistering it from kvm_io_bus 2023-02-07 12:37 ` [PATCH v2 1/2] KVM: destruct kvm_io_device while unregistering it from kvm_io_bus Wei Wang @ 2023-03-23 15:43 ` Sean Christopherson 0 siblings, 0 replies; 6+ messages in thread From: Sean Christopherson @ 2023-03-23 15:43 UTC (permalink / raw) To: Wei Wang; +Cc: pbonzini, mhal, kvm, linux-kernel On Tue, Feb 07, 2023, Wei Wang wrote: > Current usage of kvm_io_device requires users to destruct it with an extra > call of kvm_iodevice_destructor after the device gets unregistered from > kvm_io_bus. This is not necessary and can cause errors if a user forgot > to make the extra call. > > Simplify the usage by combining kvm_iodevice_destructor into > kvm_io_bus_unregister_dev. This reduces LOCs a bit for users and can > avoid the leakage of destructing the device explicitly. The changelog should really call out that coalesced_mmio_ops and ioeventfd_ops are the only kvm_io_device_ops instances that implement ->destructor. Without that info, this change looks super dangerous as it's not obvious other paths won't end up with a use-after-free. Paolo, if/when you take this, can you tack on something like: Note, coalesced_mmio_ops and ioeventfd_ops are the only instances of kvm_io_device_ops that implement a destructor, all other callers of kvm_io_bus_unregister_dev() are unaffected by this change. > Signed-off-by: Wei Wang <wei.w.wang@intel.com> > --- Reviewed-by: Sean Christopherson <seanjc@google.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] kvm/eventfd: use list_for_each_entry when deassign ioeventfd 2023-02-07 12:37 [PATCH v2 0/2] kvm_io_bus_unregister_dev cleanup Wei Wang 2023-02-07 12:37 ` [PATCH v2 1/2] KVM: destruct kvm_io_device while unregistering it from kvm_io_bus Wei Wang @ 2023-02-07 12:37 ` Wei Wang 2023-03-23 15:30 ` Sean Christopherson 2023-06-13 23:21 ` [PATCH v2 0/2] kvm_io_bus_unregister_dev cleanup Sean Christopherson 2 siblings, 1 reply; 6+ messages in thread From: Wei Wang @ 2023-02-07 12:37 UTC (permalink / raw) To: pbonzini, seanjc, mhal; +Cc: kvm, linux-kernel, Wei Wang Simpify kvm_deassign_ioeventfd_idx to use list_for_each_entry as the loop just ends at the entry that's founded and deleted. Signed-off-by: Wei Wang <wei.w.wang@intel.com> --- virt/kvm/eventfd.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c index 1b277afb545b..2b56a3c81957 100644 --- a/virt/kvm/eventfd.c +++ b/virt/kvm/eventfd.c @@ -868,7 +868,7 @@ static int kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx, struct kvm_ioeventfd *args) { - struct _ioeventfd *p, *tmp; + struct _ioeventfd *p; struct eventfd_ctx *eventfd; struct kvm_io_bus *bus; int ret = -ENOENT; @@ -882,8 +882,7 @@ kvm_deassign_ioeventfd_idx(struct kvm *kvm, enum kvm_bus bus_idx, mutex_lock(&kvm->slots_lock); - list_for_each_entry_safe(p, tmp, &kvm->ioeventfds, list) { - + list_for_each_entry(p, &kvm->ioeventfds, list) { if (p->bus_idx != bus_idx || p->eventfd != eventfd || p->addr != args->addr || -- 2.27.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] kvm/eventfd: use list_for_each_entry when deassign ioeventfd 2023-02-07 12:37 ` [PATCH v2 2/2] kvm/eventfd: use list_for_each_entry when deassign ioeventfd Wei Wang @ 2023-03-23 15:30 ` Sean Christopherson 0 siblings, 0 replies; 6+ messages in thread From: Sean Christopherson @ 2023-03-23 15:30 UTC (permalink / raw) To: Wei Wang; +Cc: pbonzini, mhal, kvm, linux-kernel On Tue, Feb 07, 2023, Wei Wang wrote: > Simpify kvm_deassign_ioeventfd_idx to use list_for_each_entry as the > loop just ends at the entry that's founded and deleted. > Suggested-by: Michal Luczaj <mhal@rbox.co> > Signed-off-by: Wei Wang <wei.w.wang@intel.com> > --- Reviewed-by: Sean Christopherson <seanjc@google.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/2] kvm_io_bus_unregister_dev cleanup 2023-02-07 12:37 [PATCH v2 0/2] kvm_io_bus_unregister_dev cleanup Wei Wang 2023-02-07 12:37 ` [PATCH v2 1/2] KVM: destruct kvm_io_device while unregistering it from kvm_io_bus Wei Wang 2023-02-07 12:37 ` [PATCH v2 2/2] kvm/eventfd: use list_for_each_entry when deassign ioeventfd Wei Wang @ 2023-06-13 23:21 ` Sean Christopherson 2 siblings, 0 replies; 6+ messages in thread From: Sean Christopherson @ 2023-06-13 23:21 UTC (permalink / raw) To: Sean Christopherson, pbonzini, mhal, Wei Wang; +Cc: kvm, linux-kernel On Tue, 07 Feb 2023 20:37:11 +0800, Wei Wang wrote: > This patchset moves kvm io_device destruction into > kvm_io_bus_unregister_dev. This reduces LOCs a bit for users and can > avoid the leakage of destructing the device explicitly. > Accordingly, below cleanups are included: > - remove the exposure of kvm_iodevice_destructor and the invocation in > the users as kvm_iodevice_destructor is now invoked in > kvm_io_bus_unregister_dev; > - Change kvm_deassign_ioeventfd_idx to use list_for_each_entry as the > loop ends at the entry that's founded and deleted. > > [...] Applied to kvm-x86 generic, thanks! [1/2] KVM: destruct kvm_io_device while unregistering it from kvm_io_bus https://github.com/kvm-x86/linux/commit/5ea5ca3c2b4b [2/2] kvm/eventfd: use list_for_each_entry when deassign ioeventfd https://github.com/kvm-x86/linux/commit/cc77b95acf3c -- https://github.com/kvm-x86/linux/tree/next https://github.com/kvm-x86/linux/tree/fixes ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-06-13 23:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-07 12:37 [PATCH v2 0/2] kvm_io_bus_unregister_dev cleanup Wei Wang 2023-02-07 12:37 ` [PATCH v2 1/2] KVM: destruct kvm_io_device while unregistering it from kvm_io_bus Wei Wang 2023-03-23 15:43 ` Sean Christopherson 2023-02-07 12:37 ` [PATCH v2 2/2] kvm/eventfd: use list_for_each_entry when deassign ioeventfd Wei Wang 2023-03-23 15:30 ` Sean Christopherson 2023-06-13 23:21 ` [PATCH v2 0/2] kvm_io_bus_unregister_dev cleanup Sean Christopherson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.