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.
If an actual use case comes along then we can move refcount_set() back,
but with the requirement that the arch/user provide a mechanism to
handle the reference with respect to kvm_arch_destroy_vm(). As opposed
to the current behavior, which allows an arch to naively do get()/put()
in init_vm()/destroy_vm() without any hint that what they're doing is
broken.
