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/[email protected]/
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Wei Wang <[email protected]>
> ---
Reviewed-by: Sean Christopherson <[email protected]>
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 <[email protected]>
> ---
Reviewed-by: Sean Christopherson <[email protected]>
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