* [PATCH] kvm: call kvm_arch_destroy_vm if vm creation fails
@ 2019-10-23 17:14 Jim Mattson
2019-10-23 18:21 ` Sean Christopherson
0 siblings, 1 reply; 6+ messages in thread
From: Jim Mattson @ 2019-10-23 17:14 UTC (permalink / raw)
To: kvm, Paolo Bonzini; +Cc: John Sperbeck, 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>
---
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..10ac7ae03677b 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)
@@ -698,10 +698,12 @@ static struct kvm *kvm_create_vm(unsigned long type)
hardware_disable_all();
out_err_no_disable:
refcount_set(&kvm->users_count, 0);
+ kvm_arch_destroy_vm(kvm);
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);
--
2.23.0.866.gb869b98d4c-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] kvm: call kvm_arch_destroy_vm if vm creation fails
2019-10-23 17:14 [PATCH] kvm: call kvm_arch_destroy_vm if vm creation fails Jim Mattson
@ 2019-10-23 18:21 ` Sean Christopherson
2019-10-24 2:59 ` Junaid Shahid
0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2019-10-23 18:21 UTC (permalink / raw)
To: Jim Mattson; +Cc: kvm, Paolo Bonzini, John Sperbeck
On Wed, Oct 23, 2019 at 10:14:35AM -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>
> ---
> 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..10ac7ae03677b 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)
> @@ -698,10 +698,12 @@ static struct kvm *kvm_create_vm(unsigned long type)
> hardware_disable_all();
> out_err_no_disable:
> refcount_set(&kvm->users_count, 0);
> + kvm_arch_destroy_vm(kvm);
Calling destroy_vm() after zeroing the refcount could lead to a refcount
underrun (and a WARN with CONFIG_REFCOUNT_FULL=y) if an arch were to do
kvm_put_kvm() in destroy_vm() to pair with a kvm_get_kvm() in create_vm().
I doubt any arch actually does that, but it's technically possible since
kvm_arch_create_vm() is called with users_count=1.
If we wanted to be paranoid, a follow-up patch could change refcount_set()
to WARN_ON(!refcount_dec_and_dest()), e.g.:
kvm_arch_destroy_vm(kvm);
WARN_ON(!refcount_dec_and_dest(&kvm->users_count));
> 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);
> --
> 2.23.0.866.gb869b98d4c-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kvm: call kvm_arch_destroy_vm if vm creation fails
2019-10-23 18:21 ` Sean Christopherson
@ 2019-10-24 2:59 ` Junaid Shahid
2019-10-24 10:08 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Junaid Shahid @ 2019-10-24 2:59 UTC (permalink / raw)
To: Sean Christopherson, Jim Mattson; +Cc: kvm, Paolo Bonzini, John Sperbeck
On 10/23/19 11:21 AM, Sean Christopherson wrote:
>> out_err_no_disable:
>> refcount_set(&kvm->users_count, 0);
>> + kvm_arch_destroy_vm(kvm);
>
> Calling destroy_vm() after zeroing the refcount could lead to a refcount
> underrun (and a WARN with CONFIG_REFCOUNT_FULL=y) if an arch were to do
> kvm_put_kvm() in destroy_vm() to pair with a kvm_get_kvm() in create_vm().
> I doubt any arch actually does that, but it's technically possible since
> kvm_arch_create_vm() is called with users_count=1.
>
> If we wanted to be paranoid, a follow-up patch could change refcount_set()
> to WARN_ON(!refcount_dec_and_dest()), e.g.:
>
> kvm_arch_destroy_vm(kvm);
> WARN_ON(!refcount_dec_and_dest(&kvm->users_count));
>
AFAICT the kvm->users_count is already 0 before kvm_arch_destroy_vm() is called from kvm_destroy_vm() in the normal case. So there really shouldn't be any arch that does a kvm_put_kvm() inside kvm_arch_destroy_vm(). I think it might be better to keep the kvm_arch_destroy_vm() call after the refcount_set() to be consistent with the normal path.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kvm: call kvm_arch_destroy_vm if vm creation fails
2019-10-24 2:59 ` Junaid Shahid
@ 2019-10-24 10:08 ` Paolo Bonzini
2019-10-24 18:14 ` Sean Christopherson
0 siblings, 1 reply; 6+ messages in thread
From: Paolo Bonzini @ 2019-10-24 10:08 UTC (permalink / raw)
To: Junaid Shahid, Sean Christopherson, Jim Mattson; +Cc: kvm, John Sperbeck
On 24/10/19 04:59, Junaid Shahid wrote:
> AFAICT the kvm->users_count is already 0 before kvm_arch_destroy_vm()
> is called from kvm_destroy_vm() in the normal case.
Yes:
if (refcount_dec_and_test(&kvm->users_count))
kvm_destroy_vm(kvm);
where
| int atomic_inc_and_test(atomic_t *v);
| int atomic_dec_and_test(atomic_t *v);
|
| These two routines increment and decrement by 1, respectively, the
| given atomic counter. They return a boolean indicating whether the
| resulting counter value was zero or not.
> So there really
> shouldn't be any arch that does a kvm_put_kvm() inside
> kvm_arch_destroy_vm(). I think it might be better to keep the
> kvm_arch_destroy_vm() call after the refcount_set() to be consistent
> with the normal path.
I agree, so I am applying Jim's patch. If anything, we may want to WARN
if the refcount is not 1 before the refcount_set.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] kvm: call kvm_arch_destroy_vm if vm creation fails
2019-10-24 10:08 ` Paolo Bonzini
@ 2019-10-24 18:14 ` Sean Christopherson
2019-10-24 18:55 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2019-10-24 18:14 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Junaid Shahid, Jim Mattson, kvm, John Sperbeck
On Thu, Oct 24, 2019 at 12:08:29PM +0200, Paolo Bonzini wrote:
> On 24/10/19 04:59, Junaid Shahid wrote:
> > AFAICT the kvm->users_count is already 0 before kvm_arch_destroy_vm()
> > is called from kvm_destroy_vm() in the normal case.
>
> Yes:
>
> if (refcount_dec_and_test(&kvm->users_count))
> kvm_destroy_vm(kvm);
>
> where
>
> | int atomic_inc_and_test(atomic_t *v);
> | int atomic_dec_and_test(atomic_t *v);
> |
> | These two routines increment and decrement by 1, respectively, the
> | given atomic counter. They return a boolean indicating whether the
> | resulting counter value was zero or not.
>
> > So there really
> > shouldn't be any arch that does a kvm_put_kvm() inside
> > kvm_arch_destroy_vm(). I think it might be better to keep the
> > kvm_arch_destroy_vm() call after the refcount_set() to be consistent
> > with the normal path.
>
> I agree, so I am applying Jim's patch.
Junaid also pointed out that x86 will dereference a NULL kvm->memslots[].
> If anything, we may want to WARN if the refcount is not 1 before the
> refcount_set.
What about moving "refcount_set(&kvm->users_count, 1)" to right before the
VM is added to vm_list, i.e. after arch code and init'ing the mmu_notifier?
Along with a comment explaining the kvm_get_kvm() is illegal while the VM
is being created.
That'd eliminate the atmoic_set() in the error path, which is confusing,
at least for me. It'd also obviate the need for an explicit WARN since
running with refcount debugging would immediately flag any arch that
tried to use kvm_get_kvm() during kvm_arch_create_vm().
Moving the refcount_set() could be done along with rearranging the memslots
and buses allocation/cleanup in a preparatory patch before adding the call
to kvm_arch_destroy_vm().
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] kvm: call kvm_arch_destroy_vm if vm creation fails
2019-10-24 18:14 ` Sean Christopherson
@ 2019-10-24 18:55 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2019-10-24 18:55 UTC (permalink / raw)
To: Sean Christopherson; +Cc: Junaid Shahid, Jim Mattson, kvm, John Sperbeck
On 24/10/19 20:14, Sean Christopherson wrote:
> On Thu, Oct 24, 2019 at 12:08:29PM +0200, Paolo Bonzini wrote:
>> On 24/10/19 04:59, Junaid Shahid wrote:
>>> AFAICT the kvm->users_count is already 0 before kvm_arch_destroy_vm()
>>> is called from kvm_destroy_vm() in the normal case.
>>
>> Yes:
>>
>> if (refcount_dec_and_test(&kvm->users_count))
>> kvm_destroy_vm(kvm);
>>
>> where
>>
>> | int atomic_inc_and_test(atomic_t *v);
>> | int atomic_dec_and_test(atomic_t *v);
>> |
>> | These two routines increment and decrement by 1, respectively, the
>> | given atomic counter. They return a boolean indicating whether the
>> | resulting counter value was zero or not.
>>
>>> So there really
>>> shouldn't be any arch that does a kvm_put_kvm() inside
>>> kvm_arch_destroy_vm(). I think it might be better to keep the
>>> kvm_arch_destroy_vm() call after the refcount_set() to be consistent
>>> with the normal path.
>>
>> I agree, so I am applying Jim's patch.
>
> Junaid also pointed out that x86 will dereference a NULL kvm->memslots[].
>
>> If anything, we may want to WARN if the refcount is not 1 before the
>> refcount_set.
>
> What about moving "refcount_set(&kvm->users_count, 1)" to right before the
> VM is added to vm_list, i.e. after arch code and init'ing the mmu_notifier?
> Along with a comment explaining the kvm_get_kvm() is illegal while the VM
> is being created.
>
> That'd eliminate the atmoic_set() in the error path, which is confusing,
> at least for me. It'd also obviate the need for an explicit WARN since
> running with refcount debugging would immediately flag any arch that
> tried to use kvm_get_kvm() during kvm_arch_create_vm().
>
> Moving the refcount_set() could be done along with rearranging the memslots
> and buses allocation/cleanup in a preparatory patch before adding the call
> to kvm_arch_destroy_vm().
Sounds good.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-24 18:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-10-23 17:14 [PATCH] kvm: call kvm_arch_destroy_vm if vm creation fails Jim Mattson
2019-10-23 18:21 ` Sean Christopherson
2019-10-24 2:59 ` Junaid Shahid
2019-10-24 10:08 ` Paolo Bonzini
2019-10-24 18:14 ` Sean Christopherson
2019-10-24 18:55 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox