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().
