* [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 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).