From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH] KVM: Fix srcu struct leakage
Date: Tue, 9 Nov 2010 12:03:51 -0200
Message-ID: <20101109140351.GA26116@amt.cnet>
References: <4CD93366.5000209@siemens.com>
 <20101109135350.GA25952@amt.cnet>
 <4CD953D1.90802@siemens.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Avi Kivity <avi@redhat.com>, kvm <kvm@vger.kernel.org>
To: Jan Kiszka <jan.kiszka@siemens.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:22952 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751464Ab0KIOEc (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 9 Nov 2010 09:04:32 -0500
Content-Disposition: inline
In-Reply-To: <4CD953D1.90802@siemens.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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