[PATCH v2 0/2] kvm_io_bus_unregister_dev cleanup

[PATCH v2 0/2] kvm_io_bus_unregister_dev cleanup

[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

[PATCH v2 1/2] KVM: destruct kvm_io_device while unregistering it from kvm_io_bus

[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

[PATCH v2 2/2] kvm/eventfd: use list_for_each_entry when deassign ioeventfd

[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

Re: [PATCH v2 2/2] kvm/eventfd: use list_for_each_entry when deassign ioeventfd

[Sean Christopherson]

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>

Re: [PATCH v2 1/2] KVM: destruct kvm_io_device while unregistering it from kvm_io_bus

[Sean Christopherson]

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>

Re: [PATCH v2 0/2] kvm_io_bus_unregister_dev cleanup

[Sean Christopherson]

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