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 05:23:54PM +0200, Paolo Bonzini wrote:
> On 25/10/19 17:22, Sean Christopherson wrote:
> > On Fri, Oct 25, 2019 at 04:56:23PM +0200, Paolo Bonzini wrote:
> >> On 25/10/19 16:48, Sean Christopherson wrote:
> >>>> 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()
> >>> }
> >>
> >> There's two parts to this:
> >>
> >> - if kvm_arch_init_vm() calls kvm_get_kvm(), then kvm_arch_destroy_vm()
> >> won't be called until the corresponding kvm_put_kvm().
> >>
> >> - if the error case causes kvm_arch_destroy_vm() to be called early,
> >> however, that'd be okay and would not leak memory, as long as
> >> kvm_arch_destroy_vm() detects the situation and calls kvm_put_kvm() itself.
> >>
> >> One case could be where you have some kind of delayed work, where the
> >> callback does kvm_put_kvm.  You'd have to cancel the work item and call
> >> kvm_put_kvm in kvm_arch_destroy_vm, and you would go through that path
> >> if kvm_create_vm() fails after kvm_arch_init_vm().
> > 
> > But do we really want/need to allow handing out references to KVM during
> > kvm_arch_init_vm()?  AFAICT, it's not currently required by any arch.
> 
> Probably not, but the full code paths are long, so I don't see much
> value in outright forbidding it.  There are very few kvm_get_kvm() calls
> anyway in arch-dependent code, so it's easy to check that they're not
> causing reference cycles.

I wasn't thinking forbid it for all eternity, more like add a landmine to
force an arch to implement more robust handling in order to enable
kvm_get_kvm() during init_vm().