Re: [PATCH] KVM: Fix srcu struct leakage

[Marcelo Tosatti <mtosatti@redhat.com>]

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