kvm: call kvm_arch_destroy_vm if vm creation fails

Defer setting the reference count, kvm->users_count, until the VM is guaranteed to be created, so that the reference count need not be cleared on the error path.

Suggested-by Sean Christopherson <[email protected]>

Signed-off-by Jim Mattson <[email protected]>

Reviewed-by Junaid Shahid <[email protected]>

---

This reorganization will allow us to call kvm_arch_destroy_vm in the event that kvm_create_vm fails after calling kvm_arch_init_vm.

Suggested-by Junaid Shahid <[email protected]>

Signed-off-by Jim Mattson <[email protected]>

Reviewed-by Junaid Shahid <[email protected]>

---
 virt/kvm/kvm_main.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

virt/kvm/kvm_main.c +9 -1

index 525e0dbc623f9..77819597d7e8e 100644
@@ -627,8 +627,9 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
 
 static struct kvm *kvm_create_vm(unsigned long type)
 {
-	int r, i;
 	struct kvm *kvm = kvm_arch_alloc_vm();
+	int r = -ENOMEM;
+	int i;
 
 	if (!kvm)
 		return ERR_PTR(-ENOMEM);

@@ -642,6 +643,25 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	mutex_init(&kvm->slots_lock);
 	INIT_LIST_HEAD(&kvm->devices);
 
+	BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
+
+	for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
+		struct kvm_memslots *slots = kvm_alloc_memslots();
+
+		if (!slots)
+			goto out_err_no_disable;

virt/kvm/kvm_main.c +2 -18

+	}
+
 	r = kvm_arch_init_vm(kvm, type);
 	if (r)
 		goto out_err_no_disable;

@@ -654,28 +674,10 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	INIT_HLIST_HEAD(&kvm->irq_ack_notifier_list);
 #endif
 
-	BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
-
-	r = -ENOMEM;
-	for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
-		struct kvm_memslots *slots = kvm_alloc_memslots();
-		if (!slots)
-			goto out_err_no_srcu;
-		/* Generations must be different for each address space. */
-		slots->generation = i;
-		rcu_assign_pointer(kvm->memslots[i], slots);
-	}
-
 	if (init_srcu_struct(&kvm->srcu))
 		goto out_err_no_srcu;
 	if (init_srcu_struct(&kvm->irq_srcu))
 		goto out_err_no_irq_srcu;
-	for (i = 0; i < KVM_NR_BUSES; i++) {
-		rcu_assign_pointer(kvm->buses[i],
-			kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT));
-		if (!kvm->buses[i])
-			goto out_err;
-	}
 
 	r = kvm_init_mmu_notifier(kvm);
 	if (r)

Signature · 1 line

2.24.0.rc0.303.g954a862665-goog

Personally I'd prefer to add labels for each stage of destruction instead of abusing the NULL handling of kfree() and kvm_free_memslots(), especially since not doing so forces the next patch to update these gotos.

From: John Sperbeck <[email protected]>

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 <[email protected]>

Signed-off-by Jim Mattson <[email protected]>

Reviewed-by Junaid Shahid <[email protected]>

---
 virt/kvm/kvm_main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

virt/kvm/kvm_main.c +3 -3

index 77819597d7e8e..f8f0106f8d20f 100644
@@ -649,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);

@@ -659,12 +659,12 @@ 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;
 	}
 
 	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)

virt/kvm/kvm_main.c +2 -0

 	 */
 	refcount_set(&kvm->users_count, 1);
 

@@ -704,6 +704,8 @@ 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);
+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++)

Signature · 1 line

2.24.0.rc0.303.g954a862665-goog

Quoted 6 lines · Jim Mattson

From: John Sperbeck <[email protected]>

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

Quoted 8 lines

Fixes 44a95dae1d229a ("KVM: x86: Detect and Initialize AVIC support")

Signed-off-by John Sperbeck <[email protected]>

Signed-off-by Jim Mattson <[email protected]>

Reviewed-by Junaid Shahid <[email protected]>

---
 virt/kvm/kvm_main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Quoted 7 lines · Sean Christopherson

Quoted 6 lines · Jim Mattson

From: John Sperbeck <[email protected]>

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

Quoted 8 lines

Quoted 8 lines

Fixes 44a95dae1d229a ("KVM: x86: Detect and Initialize AVIC support")

Signed-off-by John Sperbeck <[email protected]>

Signed-off-by Jim Mattson <[email protected]>

Reviewed-by Junaid Shahid <[email protected]>

---
 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:

Quoted 3 lines

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

Quoted 7 lines · Sean Christopherson

Quoted 3 lines

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)

Quoted 8 lines

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

Quoted 26 lines · Paolo Bonzini

Quoted 7 lines · Sean Christopherson

Quoted 3 lines

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)

Quoted 8 lines

{
	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.

Quoted 4 lines

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.

Quoted 32 lines · Sean Christopherson

Quoted 30 lines · Paolo Bonzini

Quoted 7 lines · Sean Christopherson

Quoted 3 lines

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)

Quoted 8 lines

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

Quoted 2 lines

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.