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

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Junaid Shahid <junaids@google.com>,
	Jim Mattson <jmattson@google.com>,
	kvm@vger.kernel.org, John Sperbeck <jsperbeck@google.com>
Subject: Re: [PATCH] kvm: call kvm_arch_destroy_vm if vm creation fails
Date: Thu, 24 Oct 2019 11:14:03 -0700	[thread overview]
Message-ID: <20191024181403.GD20633@linux.intel.com> (raw)
In-Reply-To: <4d81887e-12d7-baaf-586b-b85020bd5eaf@redhat.com>

On Thu, Oct 24, 2019 at 12:08:29PM +0200, Paolo Bonzini wrote:
> On 24/10/19 04:59, Junaid Shahid wrote:
> > AFAICT the kvm->users_count is already 0 before kvm_arch_destroy_vm()
> > is called from kvm_destroy_vm() in the normal case.
> 
> Yes:
> 
>         if (refcount_dec_and_test(&kvm->users_count))
>                 kvm_destroy_vm(kvm);
> 
> where
> 
> | int atomic_inc_and_test(atomic_t *v);
> | int atomic_dec_and_test(atomic_t *v);
> |
> | These two routines increment and decrement by 1, respectively, the
> | given atomic counter.  They return a boolean indicating whether the
> | resulting counter value was zero or not.
> 
> > So there really
> > shouldn't be any arch that does a kvm_put_kvm() inside
> > kvm_arch_destroy_vm(). I think it might be better to keep the
> > kvm_arch_destroy_vm() call after the refcount_set() to be consistent
> > with the normal path.
> 
> I agree, so I am applying Jim's patch.

Junaid also pointed out that x86 will dereference a NULL kvm->memslots[].

> If anything, we may want to WARN if the refcount is not 1 before the
> refcount_set.

What about moving "refcount_set(&kvm->users_count, 1)" to right before the
VM is added to vm_list, i.e. after arch code and init'ing the mmu_notifier?
Along with a comment explaining the kvm_get_kvm() is illegal while the VM
is being created.

That'd eliminate the atmoic_set() in the error path, which is confusing,
at least for me.  It'd also obviate the need for an explicit WARN since
running with refcount debugging would immediately flag any arch that
tried to use kvm_get_kvm() during kvm_arch_create_vm().

Moving the refcount_set() could be done along with rearranging the memslots
and buses allocation/cleanup in a preparatory patch before adding the call
to kvm_arch_destroy_vm().