Re: [PATCH v3 3/3] kvm: call kvm_arch_destroy_vm if vm creation fails

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Fri, Oct 25, 2019 at 01:37:56PM +0200, Paolo Bonzini wrote:
> On 25/10/19 01:29, Sean Christopherson wrote:
> > On Thu, Oct 24, 2019 at 04:03:27PM -0700, Jim Mattson wrote:
> >> From: John Sperbeck <jsperbeck@google.com>
> >>
> >> In kvm_create_vm(), if we've successfully called kvm_arch_init_vm(), but
> >> then fail later in the function, we need to call kvm_arch_destroy_vm()
> >> so that it can do any necessary cleanup (like freeing memory).
> >>
> >> Fixes: 44a95dae1d229a ("KVM: x86: Detect and Initialize AVIC support")
> >>
> >> Signed-off-by: John Sperbeck <jsperbeck@google.com>
> >> Signed-off-by: Jim Mattson <jmattson@google.com>
> >> Reviewed-by: Junaid Shahid <junaids@google.com>
> >> ---
> >>  virt/kvm/kvm_main.c | 10 ++++++----
> >>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> Sorry for the back and forth on this---I actually preferred the version 
> that did not move refcount_set.  It seems to me that kvm_get_kvm() in 
> kvm_arch_init_vm() should be okay as long as it is balanced in 
> kvm_arch_destroy_vm().  So we can apply patch 2 first, and then:

No, this will effectively leak the VM because you'll end up with a cyclical
reference to kvm_put_kvm(), i.e. users_count will never hit zero.

void kvm_put_kvm(struct kvm *kvm)
{
	if (refcount_dec_and_test(&kvm->users_count))
		kvm_destroy_vm(kvm);
		|
		-> kvm_arch_destroy_vm()
		   |
		   -> kvm_put_kvm()
}