| Message ID | 20191023203214.93252-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] kvm: call kvm_arch_destroy_vm if vm creation fails | expand |
On Wed, Oct 23, 2019 at 01:32:14PM -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: Sean Christopherson <sean.j.christopherson@intel.com> > v1 -> v2: Call kvm_arch_destroy_vm before refcount_set > > virt/kvm/kvm_main.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index fd68fbe0a75d2..c1a1cc2aa7a80 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -645,7 +645,7 @@ static struct kvm *kvm_create_vm(unsigned long type) > > r = kvm_arch_init_vm(kvm, type); > if (r) > - goto out_err_no_disable; > + goto out_err_no_arch_destroy_vm; > > r = hardware_enable_all(); > if (r) > @@ -697,11 +697,13 @@ static struct kvm *kvm_create_vm(unsigned long type) > out_err_no_srcu: > hardware_disable_all(); > out_err_no_disable: > + kvm_arch_destroy_vm(kvm); > refcount_set(&kvm->users_count, 0); > for (i = 0; i < KVM_NR_BUSES; i++) > kfree(kvm_get_bus(kvm, i)); > for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) > kvm_free_memslots(kvm, __kvm_memslots(kvm, i)); Side topic, the loops to free the buses and memslots belong higher up, the arrays aren't initialized until after hardware_enable(). Probably doesn't harm anything but it's a waste of cycles. I'll send a patch. > +out_err_no_arch_destroy_vm: > kvm_arch_free_vm(kvm); > mmdrop(current->mm); > return ERR_PTR(r); > -- > 2.24.0.rc0.303.g954a862665-goog >
[Plain-text resend] On 10/23/19 5:05 PM, Sean Christopherson wrote: > > Side topic, the loops to free the buses and memslots belong higher up, > the arrays aren't initialized until after hardware_enable(). Probably > doesn't harm anything but it's a waste of cycles. I'll send a patch. > Aren't the x86_set_memory_region() calls inside kvm_arch_destroy_vm() going to be problematic if hardware_enable_all() fails? Perhaps we should move the memslots allocation before kvm_arch_init_vm(), or check for NULL memslots in kvm_arch_destroy_vm().
On Wed, Oct 23, 2019 at 06:18:35PM -0700, Junaid Shahid wrote: > [Plain-text resend] > > On 10/23/19 5:05 PM, Sean Christopherson wrote: > > > > Side topic, the loops to free the buses and memslots belong higher up, > > the arrays aren't initialized until after hardware_enable(). Probably > > doesn't harm anything but it's a waste of cycles. I'll send a patch. > > > > Aren't the x86_set_memory_region() calls inside kvm_arch_destroy_vm() going > to be problematic if hardware_enable_all() fails? Perhaps we should move the > memslots allocation before kvm_arch_init_vm(), or check for NULL memslots in > kvm_arch_destroy_vm(). Oof, that does appear to be the case. Initializing memslots and buses before calling into arch code seems like the way to go.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index fd68fbe0a75d2..c1a1cc2aa7a80 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -645,7 +645,7 @@ static struct kvm *kvm_create_vm(unsigned long type) r = kvm_arch_init_vm(kvm, type); if (r) - goto out_err_no_disable; + goto out_err_no_arch_destroy_vm; r = hardware_enable_all(); if (r) @@ -697,11 +697,13 @@ static struct kvm *kvm_create_vm(unsigned long type) out_err_no_srcu: hardware_disable_all(); out_err_no_disable: + kvm_arch_destroy_vm(kvm); refcount_set(&kvm->users_count, 0); for (i = 0; i < KVM_NR_BUSES; i++) kfree(kvm_get_bus(kvm, i)); for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) kvm_free_memslots(kvm, __kvm_memslots(kvm, i)); +out_err_no_arch_destroy_vm: kvm_arch_free_vm(kvm); mmdrop(current->mm); return ERR_PTR(r);