linux-kernel - Re: [PATCH v1] KVM: destruct kvm_io_device while unregistering it from kvm_io

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 24 Jan 2023 21:22:23 +0100
From:   Michal Luczaj <mhal@...x.co>
To:     Sean Christopherson <seanjc@...gle.com>,
        Wei Wang <wei.w.wang@...el.com>
Cc:     pbonzini@...hat.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        柳菁峰 <liujingfeng@...nxin.com>
Subject: Re: [PATCH v1] KVM: destruct kvm_io_device while unregistering it
 from kvm_io_bus

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