[PATCH] KVM: Fix srcu struct leakage

[PATCH] KVM: Fix srcu struct leakage

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Clean up the srcu struct and refactor its release on early errors.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 virt/kvm/kvm_main.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4111a4b..c80a44a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -401,23 +401,19 @@ static struct kvm *kvm_create_vm(void)
 	r = -ENOMEM;
 	kvm->memslots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL);
 	if (!kvm->memslots)
-		goto out_err;
+		goto out_err_nosrcu;
 	if (init_srcu_struct(&kvm->srcu))
-		goto out_err;
+		goto out_err_nosrcu;
 	for (i = 0; i < KVM_NR_BUSES; i++) {
 		kvm->buses[i] = kzalloc(sizeof(struct kvm_io_bus),
 					GFP_KERNEL);
-		if (!kvm->buses[i]) {
-			cleanup_srcu_struct(&kvm->srcu);
+		if (!kvm->buses[i])
 			goto out_err;
-		}
 	}
 
 	r = kvm_init_mmu_notifier(kvm);
-	if (r) {
-		cleanup_srcu_struct(&kvm->srcu);
+	if (r)
 		goto out_err;
-	}
 
 	kvm->mm = current->mm;
 	atomic_inc(&kvm->mm->mm_count);
@@ -435,6 +431,8 @@ out:
 	return kvm;
 
 out_err:
+	cleanup_srcu_struct(&kvm->srcu);
+out_err_nosrcu:
 	hardware_disable_all();
 out_err_nodisable:
 	for (i = 0; i < KVM_NR_BUSES; i++)
@@ -516,6 +514,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
 	kvm_arch_destroy_vm(kvm);
 	hardware_disable_all();
 	mmdrop(mm);
+	cleanup_srcu_struct(&kvm->srcu);
 }
 
 void kvm_get_kvm(struct kvm *kvm)
-- 
1.7.1

[PATCH] KVM: Fix srcu struct leakage

[Jan Kiszka]

Clean up the srcu struct on vm destruction.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 virt/kvm/kvm_main.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4111a4b..6ec58d1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
 #else
 	kvm_arch_flush_shadow(kvm);
 #endif
+	cleanup_srcu_struct(&kvm->srcu);
 	kvm_arch_destroy_vm(kvm);
 	hardware_disable_all();
 	mmdrop(mm);
-- 
1.7.1

Re: [PATCH] KVM: Fix srcu struct leakage

[Avi Kivity]

On 11/09/2010 01:41 PM, Jan Kiszka wrote:
> Clean up the srcu struct on vm destruction.

Applied and queued for 2.6.37; thanks.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: Fix srcu struct leakage

[Marcelo Tosatti]

On Tue, Nov 09, 2010 at 12:41:26PM +0100, Jan Kiszka wrote:
> Clean up the srcu struct on vm destruction.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  virt/kvm/kvm_main.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 4111a4b..6ec58d1 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
>  #else
>  	kvm_arch_flush_shadow(kvm);
>  #endif
> +	cleanup_srcu_struct(&kvm->srcu);
>  	kvm_arch_destroy_vm(kvm);
>  	hardware_disable_all();
>  	mmdrop(mm);
> -- 
> 1.7.1

kvm_arch_destroy_vm does it.

Re: [PATCH] KVM: Fix srcu struct leakage

[Jan Kiszka]

Am 09.11.2010 14:53, Marcelo Tosatti wrote:
> On Tue, Nov 09, 2010 at 12:41:26PM +0100, Jan Kiszka wrote:
>> Clean up the srcu struct on vm destruction.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  virt/kvm/kvm_main.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 4111a4b..6ec58d1 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
>>  #else
>>  	kvm_arch_flush_shadow(kvm);
>>  #endif
>> +	cleanup_srcu_struct(&kvm->srcu);
>>  	kvm_arch_destroy_vm(kvm);
>>  	hardware_disable_all();
>>  	mmdrop(mm);
>> -- 
>> 1.7.1
> 
> kvm_arch_destroy_vm does it.

Oh, indeed.

That I guess it's time to clean up, move generic allocation and release
into generic code. Having kvm_arch_create/destroy_vm manage the kvm
object allocation does not look very nice (and caused this confusion of
mine).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: Fix srcu struct leakage

[Avi Kivity]

On 11/09/2010 03:53 PM, Marcelo Tosatti wrote:
> On Tue, Nov 09, 2010 at 12:41:26PM +0100, Jan Kiszka wrote:
> >  Clean up the srcu struct on vm destruction.
> >
> >  Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >  ---
> >   virt/kvm/kvm_main.c |    1 +
> >   1 files changed, 1 insertions(+), 0 deletions(-)
> >
> >  diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >  index 4111a4b..6ec58d1 100644
> >  --- a/virt/kvm/kvm_main.c
> >  +++ b/virt/kvm/kvm_main.c
> >  @@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
> >   #else
> >   	kvm_arch_flush_shadow(kvm);
> >   #endif
> >  +	cleanup_srcu_struct(&kvm->srcu);
> >   	kvm_arch_destroy_vm(kvm);
> >   	hardware_disable_all();
> >   	mmdrop(mm);
> >  -- 
> >  1.7.1
>
> kvm_arch_destroy_vm does it.

True.  Dropped.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: Fix srcu struct leakage

[Marcelo Tosatti]

On Tue, Nov 09, 2010 at 02:59:45PM +0100, Jan Kiszka wrote:
> Am 09.11.2010 14:53, Marcelo Tosatti wrote:
> > On Tue, Nov 09, 2010 at 12:41:26PM +0100, Jan Kiszka wrote:
> >> Clean up the srcu struct on vm destruction.
> >>
> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >> ---
> >>  virt/kvm/kvm_main.c |    1 +
> >>  1 files changed, 1 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >> index 4111a4b..6ec58d1 100644
> >> --- a/virt/kvm/kvm_main.c
> >> +++ b/virt/kvm/kvm_main.c
> >> @@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
> >>  #else
> >>  	kvm_arch_flush_shadow(kvm);
> >>  #endif
> >> +	cleanup_srcu_struct(&kvm->srcu);
> >>  	kvm_arch_destroy_vm(kvm);
> >>  	hardware_disable_all();
> >>  	mmdrop(mm);
> >> -- 
> >> 1.7.1
> > 
> > kvm_arch_destroy_vm does it.
> 
> Oh, indeed.
> 
> That I guess it's time to clean up, move generic allocation and release
> into generic code. Having kvm_arch_create/destroy_vm manage the kvm
> object allocation does not look very nice (and caused this confusion of
> mine).
> 
> Jan

kvm_iommu_unmap_guest enters SRCU critical section, and "struct kvm" is
freed at the end of kvm_arch_destroy_vm. Thats why its not in generic
code.

But sure, it would be nicer if it could be moved to generic code.

void kvm_arch_destroy_vm(struct kvm *kvm)
{
        kvm_iommu_unmap_guest(kvm);
        kfree(kvm->arch.vpic);
        kfree(kvm->arch.vioapic);
        kvm_free_vcpus(kvm);
        kvm_free_physmem(kvm);
        if (kvm->arch.apic_access_page)
                put_page(kvm->arch.apic_access_page);
        if (kvm->arch.ept_identity_pagetable)
                put_page(kvm->arch.ept_identity_pagetable);
        cleanup_srcu_struct(&kvm->srcu);
        kfree(kvm);
}

Re: [PATCH] KVM: Fix srcu struct leakage

[Jan Kiszka]

Am 09.11.2010 14:59, Jan Kiszka wrote:
> Am 09.11.2010 14:53, Marcelo Tosatti wrote:
>> On Tue, Nov 09, 2010 at 12:41:26PM +0100, Jan Kiszka wrote:
>>> Clean up the srcu struct on vm destruction.
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>  virt/kvm/kvm_main.c |    1 +
>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>> index 4111a4b..6ec58d1 100644
>>> --- a/virt/kvm/kvm_main.c
>>> +++ b/virt/kvm/kvm_main.c
>>> @@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
>>>  #else
>>>  	kvm_arch_flush_shadow(kvm);
>>>  #endif
>>> +	cleanup_srcu_struct(&kvm->srcu);
>>>  	kvm_arch_destroy_vm(kvm);
>>>  	hardware_disable_all();
>>>  	mmdrop(mm);
>>> -- 
>>> 1.7.1
>>
>> kvm_arch_destroy_vm does it.
> 
> Oh, indeed.
> 
> That I guess it's time to clean up, move generic allocation and release
> into generic code. Having kvm_arch_create/destroy_vm manage the kvm
> object allocation does not look very nice (and caused this confusion of
> mine).

Oh, the setup/cleanup maze is a tribute to the IA64 zombie. Mmh...

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: Fix srcu struct leakage

[Jan Kiszka]

Am 09.11.2010 15:03, Marcelo Tosatti wrote:
> On Tue, Nov 09, 2010 at 02:59:45PM +0100, Jan Kiszka wrote:
>> Am 09.11.2010 14:53, Marcelo Tosatti wrote:
>>> On Tue, Nov 09, 2010 at 12:41:26PM +0100, Jan Kiszka wrote:
>>>> Clean up the srcu struct on vm destruction.
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>  virt/kvm/kvm_main.c |    1 +
>>>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>>> index 4111a4b..6ec58d1 100644
>>>> --- a/virt/kvm/kvm_main.c
>>>> +++ b/virt/kvm/kvm_main.c
>>>> @@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
>>>>  #else
>>>>  	kvm_arch_flush_shadow(kvm);
>>>>  #endif
>>>> +	cleanup_srcu_struct(&kvm->srcu);
>>>>  	kvm_arch_destroy_vm(kvm);
>>>>  	hardware_disable_all();
>>>>  	mmdrop(mm);
>>>> -- 
>>>> 1.7.1
>>>
>>> kvm_arch_destroy_vm does it.
>>
>> Oh, indeed.
>>
>> That I guess it's time to clean up, move generic allocation and release
>> into generic code. Having kvm_arch_create/destroy_vm manage the kvm
>> object allocation does not look very nice (and caused this confusion of
>> mine).
>>
>> Jan
> 
> kvm_iommu_unmap_guest enters SRCU critical section, and "struct kvm" is
> freed at the end of kvm_arch_destroy_vm. Thats why its not in generic
> code.

The problem is the arch-specific allocation of the kvm object on IA64.
All others simply call kmalloc/kzalloc for the struct itself, but IA64
embeds it into some larger context. Not sure why, though.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: Fix srcu struct leakage

[Avi Kivity]

On 11/09/2010 04:07 PM, Jan Kiszka wrote:
> >
> >  kvm_iommu_unmap_guest enters SRCU critical section, and "struct kvm" is
> >  freed at the end of kvm_arch_destroy_vm. Thats why its not in generic
> >  code.
>
> The problem is the arch-specific allocation of the kvm object on IA64.
> All others simply call kmalloc/kzalloc for the struct itself, but IA64
> embeds it into some larger context. Not sure why, though.

ia64 virt is special - the guest has a virtual address space for host 
data.  This data is shared with the host address space, but on different 
addresses.

I guess to reduce tlb costs ia64 kvm uses a large order allocation for 
both struct kvm and other random data, which is why the allocation is 
different.

It could be refactored to something like

#ifndef CONFIG_HAVE_SPECIAL_KVM_ALLOC

static inline struct kvm *kvm_arch_alloc_vm(void)
{
     return kzalloc(...);
}

static inline void kvm_arch_free_vm(struct kvm *kvm)
{
    kfree(kvm);
}

#endif

and have ia64 provide its special stuff.  The practice of duplicating 
common code just because of one outlier is bad, there are better ways.

(and I will accept untested ia64 patches provided kvm-ia64@ is copied).

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] KVM: Fix srcu struct leakage

[Jan Kiszka]

Am 09.11.2010 15:30, Avi Kivity wrote:
> On 11/09/2010 04:07 PM, Jan Kiszka wrote:
>>>
>>>  kvm_iommu_unmap_guest enters SRCU critical section, and "struct kvm" is
>>>  freed at the end of kvm_arch_destroy_vm. Thats why its not in generic
>>>  code.
>>
>> The problem is the arch-specific allocation of the kvm object on IA64.
>> All others simply call kmalloc/kzalloc for the struct itself, but IA64
>> embeds it into some larger context. Not sure why, though.
> 
> ia64 virt is special - the guest has a virtual address space for host 
> data.  This data is shared with the host address space, but on different 
> addresses.
> 
> I guess to reduce tlb costs ia64 kvm uses a large order allocation for 
> both struct kvm and other random data, which is why the allocation is 
> different.
> 
> It could be refactored to something like
> 
> #ifndef CONFIG_HAVE_SPECIAL_KVM_ALLOC
> 
> static inline struct kvm *kvm_arch_alloc_vm(void)
> {
>      return kzalloc(...);
> }
> 
> static inline void kvm_arch_free_vm(struct kvm *kvm)
> {
>     kfree(kvm);
> }
> 
> #endif

Yep, that's what I'm already working on.

> 
> and have ia64 provide its special stuff.  The practice of duplicating 
> common code just because of one outlier is bad, there are better ways.
> 
> (and I will accept untested ia64 patches provided kvm-ia64@ is copied).

OK, will come. Refactoring will affect all archs, so all need to test
and ack.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH] KVM: Fix srcu struct leakage

[Avi Kivity]

  On 11/09/2010 04:36 PM, Jan Kiszka wrote:
>
>> and have ia64 provide its special stuff.  The practice of duplicating
>> common code just because of one outlier is bad, there are better ways.
>>
>> (and I will accept untested ia64 patches provided kvm-ia64@ is copied).
> OK, will come. Refactoring will affect all archs, so all need to test
> and ack.

I can at least build on all of them.  Should set up a buildbot...