[PATCH v3 0/3] kvm: call kvm_arch_destroy_vm if vm creation fails

[PATCH v3 0/3] kvm: call kvm_arch_destroy_vm if vm creation fails

[Jim Mattson]

Beginning with commit 44a95dae1d229a ("KVM: x86: Detect and Initialize
AVIC support"), AMD's version of kvm_arch_init_vm() will allocate
memory if the module parameter, avic, is enabled. (Note that this
module parameter is disabled by default.) However, there are many
possible failure exits from kvm_create_vm() *after* the call to
kvm_arch_init_vm(), and the memory allocated by kvm_arch_init_vm() was
leaked on these failure paths.

The obvious solution is to call kvm_arch_destroy_vm() on these failure
paths, since it will free the memory allocated by
kvm_arch_init_vm(). However, kvm_arch_destroy_vm() may reference
memslots and buses that were allocated later in kvm_create_vm(). So,
before we can call kvm_arch_destroy_vm() on the failure paths out of
kvm_create_vm(), we need to hoist the memslot and bus allocation up
before the call to kvm_arch_init_vm().

The call to clear the reference count on (some) failure paths out of
kvm_create_vm() just added to the potential confusion. By sinking the
call to set the reference count below any possible failure exits, we
can eliminate the call to clear the reference count on the failure
paths.

v1 -> v2: Call kvm_arch_destroy_vm before refcount_set
v2 -> v3: Added two preparatory changes

Jim Mattson (2):
  kvm: Don't clear reference count on kvm_create_vm() error path
  kvm: Allocate memslots and buses before calling kvm_arch_init_vm

John Sperbeck (1):
  kvm: call kvm_arch_destroy_vm if vm creation fails

 virt/kvm/kvm_main.c | 52 ++++++++++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 22 deletions(-)

-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH v3 1/3] kvm: Don't clear reference count on kvm_create_vm() error path

[Jim Mattson]

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 <sean.j.christopherson@intel.com>
Signed-off-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Junaid Shahid <junaids@google.com>
---
 virt/kvm/kvm_main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index fd68fbe0a75d2..525e0dbc623f9 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -640,7 +640,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);
 
 	r = kvm_arch_init_vm(kvm, type);
@@ -682,6 +681,12 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	if (r)
 		goto out_err;
 
+	/*
+	 * kvm_get_kvm() isn't legal while the vm is being created
+	 * (e.g. in kvm_arch_init_vm).
+	 */
+	refcount_set(&kvm->users_count, 1);
+
 	mutex_lock(&kvm_lock);
 	list_add(&kvm->vm_list, &vm_list);
 	mutex_unlock(&kvm_lock);
@@ -697,7 +702,6 @@ 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);
 	for (i = 0; i < KVM_NR_BUSES; i++)
 		kfree(kvm_get_bus(kvm, i));
 	for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++)
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH v3 2/3] kvm: Allocate memslots and buses before calling kvm_arch_init_vm

[Jim Mattson]

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 <junaids@google.com>
Signed-off-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Junaid Shahid <junaids@google.com>
---
 virt/kvm/kvm_main.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 525e0dbc623f9..77819597d7e8e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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;
+		/* Generations must be different for each address space. */
+		slots->generation = i;
+		rcu_assign_pointer(kvm->memslots[i], slots);
+	}
+
+	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_no_disable;
+	}
+
 	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)
-- 
2.24.0.rc0.303.g954a862665-goog

[PATCH v3 3/3] kvm: call kvm_arch_destroy_vm if vm creation fails

[Jim Mattson]

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: Junaid Shahid <junaids@google.com>
---
 virt/kvm/kvm_main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 77819597d7e8e..f8f0106f8d20f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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)
@@ -685,7 +685,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
 
 	/*
 	 * kvm_get_kvm() isn't legal while the vm is being created
-	 * (e.g. in kvm_arch_init_vm).
+	 * (e.g. in kvm_arch_init_vm or kvm_arch_destroy_vm).
 	 */
 	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++)
-- 
2.24.0.rc0.303.g954a862665-goog

Re: [PATCH v3 1/3] kvm: Don't clear reference count on kvm_create_vm() error path

[Sean Christopherson]

On Thu, Oct 24, 2019 at 04:03:25PM -0700, Jim Mattson wrote:
> 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 <sean.j.christopherson@intel.com>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Junaid Shahid <junaids@google.com>
> ---

Reviewed-and-tested-by: Sean Christopherson <sean.j.christopherson@intel.com>

Re: [PATCH v3 2/3] kvm: Allocate memslots and buses before calling kvm_arch_init_vm

[Sean Christopherson]

On Thu, Oct 24, 2019 at 04:03:26PM -0700, Jim Mattson wrote:
> 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 <junaids@google.com>
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Junaid Shahid <junaids@google.com>
> ---
>  virt/kvm/kvm_main.c | 40 +++++++++++++++++++++-------------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 525e0dbc623f9..77819597d7e8e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -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;
> +		/* Generations must be different for each address space. */
> +		slots->generation = i;
> +		rcu_assign_pointer(kvm->memslots[i], slots);
> +	}
> +
> +	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_no_disable;

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.

Inverting the labels to describe what's being done instead of what's not
being done might help with the readability and naming.

> +	}
> +
>  	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)
> -- 
> 2.24.0.rc0.303.g954a862665-goog
> 

Re: [PATCH v3 3/3] kvm: call kvm_arch_destroy_vm if vm creation fails

[Sean Christopherson]

On Thu, Oct 24, 2019 at 04:03:27PM -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: Junaid Shahid <junaids@google.com>
> ---
>  virt/kvm/kvm_main.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 77819597d7e8e..f8f0106f8d20f 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -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)
> @@ -685,7 +685,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
>  
>  	/*
>  	 * kvm_get_kvm() isn't legal while the vm is being created
> -	 * (e.g. in kvm_arch_init_vm).
> +	 * (e.g. in kvm_arch_init_vm or kvm_arch_destroy_vm).

LOL, even I don't think this one is necessary ;-)

>  	 */
>  	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++)
> -- 
> 2.24.0.rc0.303.g954a862665-goog
> 

Re: [PATCH v3 2/3] kvm: Allocate memslots and buses before calling kvm_arch_init_vm

[Paolo Bonzini]

On 25/10/19 01:28, Sean Christopherson wrote:
> 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.

I'm not sure the two are related, and the NULL handling is definitely a
feature.

Regarding naming the gotos positively vs. negatively, I find the
negative naming slightly easier to review:

		init();
		if (foo())		   /* 1 */
			goto out_no_unfoo; /* 1 */
		bar();

	out:
		unbar();		 /* 2 */
	out_no_unbar:			 /* 2 */
		unfoo();
	out_no_unfoo;
		uninit();

vs.

		init();			 /* 1 */
		if (foo())
			goto out_init;   /* 1 */
		bar();
	
	out:
		unbar();
	out_foo:
		unfoo();
	out_init:			 /* 2 */
		uninit();		 /* 2 */

Perhaps I would name the labels "no_enable" and "no_arch_init_vm", but
these patches can be applied first anyway.

Paolo

Re: [PATCH v3 3/3] kvm: call kvm_arch_destroy_vm if vm creation fails

[Paolo Bonzini]

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@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: Junaid Shahid <junaids@google.com>
>> ---
>>  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

Re: [PATCH v3 3/3] kvm: call kvm_arch_destroy_vm if vm creation fails

[Sean Christopherson]

On Fri, Oct 25, 2019 at 01:37:56PM +0200, Paolo Bonzini wrote:
> 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@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: Junaid Shahid <junaids@google.com>
> >> ---
> >>  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:

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

Re: [PATCH v3 3/3] kvm: call kvm_arch_destroy_vm if vm creation fails

[Paolo Bonzini]

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

Paolo

Re: [PATCH v3 3/3] kvm: call kvm_arch_destroy_vm if vm creation fails

[Sean Christopherson]

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.

Re: [PATCH v3 3/3] kvm: call kvm_arch_destroy_vm if vm creation fails

[Paolo Bonzini]

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.

Paolo

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

Re: [PATCH v3 3/3] kvm: call kvm_arch_destroy_vm if vm creation fails

[Sean Christopherson]

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