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]

Message-ID: <Y9BFqIK04V6fBMz7@google.com>
Date:   Tue, 24 Jan 2023 20:55:04 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Michal Luczaj <mhal@...x.co>
Cc:     Wei Wang <wei.w.wang@...el.com>, 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 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
>