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

[Michal Luczaj]

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?

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

    (...) 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