Re: [PATCH v1] KVM: destruct kvm_io_device while unregistering it from kvm_io_bus

On Tue, Jan 24, 2023, Michal Luczaj wrote:

On 1/24/23 00:25, Sean Christopherson wrote:

...

On Thu, Dec 29, 2022, Wei Wang wrote:

...

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; }

I was wondering: would it make sense to simplify from list_for_each_entry_safe() to list_for_each_entry() in this loop?

Ooh, yeah, that's super confusing, at least to me, because the "safe" part implies that the loop processes entries after kvm_io_bus_unregister_dev(), i.e. needs to guard against failure same as the coalesced MMIO case.

Wei, want to tack on a patch in v2?

...

...

@@ -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;
  

Returning an error code is unnecessary if unregister_dev() destroys the bus. Nothing ultimately consumes the result, e.g. kvm_vm_ioctl_unregister_coalesced_mmio() intentionally ignores the result other than to bail from the loop, and destroying the bus means it will immediately bail from the loop anyways.

But it is important to know _if_ the bus was destroyed, right? IOW, doesn't your comment from commit 5d3c4c79384a still hold?

/facepalm

Yes, it matters. I somehow got on the train of thought that list_for_each_entry_safe() magically bails if the list is purged, but the safe variant only plays nice with the _current_ entry being deleted.

So yeah, the return code needs to stay.

(...) But, it doesn't tell the caller that it obliterated the
bus and invoked the destructor for all devices that were on the bus.  In
the coalesced MMIO case, this can result in a deleted list entry
dereference due to attempting to continue iterating on coalesced_zones
after future entries (in the walk) have been deleted.

Michal