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
>
