On 25/10/19 01:29, Sean Christopherson wrote:
> On Thu, Oct 24, 2019 at 04:03:27PM -0700, Jim Mattson wrote:
>> From: John Sperbeck <jsperbeck@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx>
>> Reviewed-by: Junaid Shahid <junaids@xxxxxxxxxx>
>> ---
>> virt/kvm/kvm_main.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
Sorry for the back and forth on this---I actually preferred the version
that did not move refcount_set. 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:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ec14dae2f538..d6f0696d98ef 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -641,7 +641,6 @@ static struct kvm *kvm_create_vm(unsigned long type)
mutex_init(&kvm->lock);
mutex_init(&kvm->irq_lock);
mutex_init(&kvm->slots_lock);
- refcount_set(&kvm->users_count, 1);
INIT_LIST_HEAD(&kvm->devices);
BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
@@ -650,7 +649,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
struct kvm_memslots *slots = kvm_alloc_memslots();
if (!slots)
- goto out_err_no_disable;
+ goto out_err_no_arch_destroy_vm;
/* Generations must be different for each address space. */
slots->generation = i;
rcu_assign_pointer(kvm->memslots[i], slots);
@@ -660,12 +659,13 @@ static struct kvm *kvm_create_vm(unsigned long type)
rcu_assign_pointer(kvm->buses[i],
kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT));
if (!kvm->buses[i])
- goto out_err_no_disable;
+ goto out_err_no_arch_destroy_vm;
}
+ refcount_set(&kvm->users_count, 1);
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)
@@ -699,7 +699,9 @@ static struct kvm *kvm_create_vm(unsigned long type)
out_err_no_srcu:
hardware_disable_all();
out_err_no_disable:
- refcount_set(&kvm->users_count, 0);
+ kvm_arch_destroy_vm(kvm);
+ WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
+out_err_no_arch_destroy_vm:
for (i = 0; i < KVM_NR_BUSES; i++)
kfree(kvm_get_bus(kvm, i));
for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++)
Moving the refcount_set is not strictly necessary, but it is nicer as
set+init is matched by the destroy+dec_and_test pair in the unwind path.
If it's okay, I can just commit it.
Paolo
