public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCHv2] KVM: x86: Fix memory leak in vmx.c
@ 2013-04-17 17:54 Andrew Honig
  2013-04-17 20:37 ` Eric Northup
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Andrew Honig @ 2013-04-17 17:54 UTC (permalink / raw)
  To: ahonig, kvm


If userspace creates and destroys multiple VMs within the same process
we leak 20k of memory in the userspace process context per VM.  This
patch frees the memory in kvm_arch_destroy_vm.  If the process exits
without closing the VM file descriptor or the file descriptor has been
shared with another process then we don't need to free the memory.

Signed-off-by: Andrew Honig <ahonig@google.com>
---
 arch/x86/kvm/x86.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e172132..e93e16b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6811,6 +6811,23 @@ void kvm_arch_sync_events(struct kvm *kvm)
 
 void kvm_arch_destroy_vm(struct kvm *kvm)
 {
+	if (current->mm == kvm->mm) {
+		/*
+		 * Free memory regions allocated on behalf of userspace,
+		 * unless the the memory map has changed due to process exit
+		 * or fd copying.
+		 */
+		struct kvm_userspace_memory_region mem;
+		memset(&mem, 0, sizeof(mem));
+		mem.slot = APIC_ACCESS_PAGE_PRIVATE_MEMSLOT;
+		kvm_set_memory_region(kvm, &mem, 0);
+
+		mem.slot = IDENTITY_PAGETABLE_PRIVATE_MEMSLOT;
+		kvm_set_memory_region(kvm, &mem, 0);
+
+		mem.slot = TSS_PRIVATE_MEMSLOT;
+		kvm_set_memory_region(kvm, &mem, 0);
+	}
 	kvm_iommu_unmap_guest(kvm);
 	kfree(kvm->arch.vpic);
 	kfree(kvm->arch.vioapic);
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCHv2] KVM: x86: Fix memory leak in vmx.c
  2013-04-17 17:54 [PATCHv2] KVM: x86: Fix memory leak in vmx.c Andrew Honig
@ 2013-04-17 20:37 ` Eric Northup
  2013-04-17 21:55 ` Paolo Bonzini
  2013-04-18 10:21 ` Gleb Natapov
  2 siblings, 0 replies; 7+ messages in thread
From: Eric Northup @ 2013-04-17 20:37 UTC (permalink / raw)
  To: Andrew Honig; +Cc: KVM

On Wed, Apr 17, 2013 at 10:54 AM, Andrew Honig <ahonig@google.com> wrote:
>
> If userspace creates and destroys multiple VMs within the same process
> we leak 20k of memory in the userspace process context per VM.  This
> patch frees the memory in kvm_arch_destroy_vm.  If the process exits
> without closing the VM file descriptor or the file descriptor has been
> shared with another process then we don't need to free the memory.

Technically, I think there's still a (temporary) leak in the case
where the last close happened from the wrong process: f_op->release()
gets called from a context where it won't whack the kvm memory
regions.  However, that's a perverse case not expected in practice --
it will get cleaned up when the original process exits and has it's mm
cleaned up.  Since the one affected (the original open()ing process of
/dev/kvm) is the one the one affected and also the one who misbehaved
(shared its file descriptor), I don't know that it's worth trying to
nail that case down as long as the host kernel isn't compromised (it
won't be).  Perhaps comment it though, at least in the changelog
entry?

>
> Signed-off-by: Andrew Honig <ahonig@google.com>
> ---
>  arch/x86/kvm/x86.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e172132..e93e16b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6811,6 +6811,23 @@ void kvm_arch_sync_events(struct kvm *kvm)
>
>  void kvm_arch_destroy_vm(struct kvm *kvm)
>  {
> +       if (current->mm == kvm->mm) {
> +               /*
> +                * Free memory regions allocated on behalf of userspace,
> +                * unless the the memory map has changed due to process exit
> +                * or fd copying.
> +                */
> +               struct kvm_userspace_memory_region mem;
> +               memset(&mem, 0, sizeof(mem));
> +               mem.slot = APIC_ACCESS_PAGE_PRIVATE_MEMSLOT;
> +               kvm_set_memory_region(kvm, &mem, 0);
> +
> +               mem.slot = IDENTITY_PAGETABLE_PRIVATE_MEMSLOT;
> +               kvm_set_memory_region(kvm, &mem, 0);
> +
> +               mem.slot = TSS_PRIVATE_MEMSLOT;
> +               kvm_set_memory_region(kvm, &mem, 0);
> +       }
>         kvm_iommu_unmap_guest(kvm);
>         kfree(kvm->arch.vpic);
>         kfree(kvm->arch.vioapic);
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCHv2] KVM: x86: Fix memory leak in vmx.c
  2013-04-17 17:54 [PATCHv2] KVM: x86: Fix memory leak in vmx.c Andrew Honig
  2013-04-17 20:37 ` Eric Northup
@ 2013-04-17 21:55 ` Paolo Bonzini
  2013-04-17 23:03   ` Andrew Honig
  2013-04-18 10:21 ` Gleb Natapov
  2 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2013-04-17 21:55 UTC (permalink / raw)
  To: Andrew Honig; +Cc: kvm

Il 17/04/2013 19:54, Andrew Honig ha scritto:
> 
> If userspace creates and destroys multiple VMs within the same process
> we leak 20k of memory in the userspace process context per VM.  This
> patch frees the memory in kvm_arch_destroy_vm.  If the process exits
> without closing the VM file descriptor or the file descriptor has been
> shared with another process then we don't need to free the memory.
> 
> Signed-off-by: Andrew Honig <ahonig@google.com>
> ---
>  arch/x86/kvm/x86.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

What about something like this (uncompiled/untested)


diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 8b3a9c0..6706134 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1563,7 +1563,8 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
 	return VM_FAULT_SIGBUS;
 }
 
-void kvm_arch_free_memslot(struct kvm_memory_slot *free,
+void kvm_arch_free_memslot(struct kvm *kvm,
+			   struct kvm_memory_slot *free,
 			   struct kvm_memory_slot *dont)
 {
 }
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 4d213b8..a654580 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -299,7 +299,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
 	return -EINVAL;
 }
 
-void kvm_arch_free_memslot(struct kvm_memory_slot *free,
+void kvm_arch_free_memslot(struct kvm *kvm,
+			   struct kvm_memory_slot *free,
 			   struct kvm_memory_slot *dont)
 {
 	if (!dont || free->arch.rmap != dont->arch.rmap) {
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index ecced9d..e2159c1 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -912,7 +912,8 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
 	return VM_FAULT_SIGBUS;
 }
 
-void kvm_arch_free_memslot(struct kvm_memory_slot *free,
+void kvm_arch_free_memslot(struct kvm *kvm,
+			   struct kvm_memory_slot *free,
 			   struct kvm_memory_slot *dont)
 {
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 224a7e7..f9fa0d1 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6357,11 +6367,26 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 	kfree(rcu_dereference_check(kvm->arch.apic_map, 1));
 }
 
-void kvm_arch_free_memslot(struct kvm_memory_slot *free,
+void kvm_arch_free_memslot(struct kvm *kvm,
+			   struct kvm_memory_slot *free,
 			   struct kvm_memory_slot *dont)
 {
 	int i;
 
+	if (current->mm == kvm->mm && free->user_alloc) {
+		if (!dont || !dont->user_alloc ||
+		    free->userspace_addr != dont->userspace_addr) {
+			int ret;
+
+			ret = vm_munmap(free->userspace_addr,
+					free->npages * PAGE_SIZE);
+			if (ret < 0)
+				printk(KERN_WARNING
+				       "kvm_vm_ioctl_set_memory_region: "
+				       "failed to munmap memory\n");
+		}
+	}
+
 	for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) {
 		if (!dont || free->arch.rmap[i] != dont->arch.rmap[i]) {
 			kvm_kvfree(free->arch.rmap[i]);
@@ -6453,7 +6478,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 	 *x86 needs to handle !user_alloc case.
 	 */
 	if (!user_alloc) {
-		if (npages && !old.npages) {
+		if (npages != old.npages) {
 			unsigned long userspace_addr;
 
 			userspace_addr = vm_mmap(NULL, 0,
@@ -6466,7 +6491,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 				return PTR_ERR((void *)userspace_addr);
 
 			memslot->userspace_addr = userspace_addr;
-		}
+		} else
+			memslot->userspace_addr = old.userspace_addr;
 	}
 
 
@@ -6481,17 +6507,6 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
 
 	int nr_mmu_pages = 0, npages = mem->memory_size >> PAGE_SHIFT;
 
-	if (!user_alloc && !old.user_alloc && old.npages && !npages) {
-		int ret;
-
-		ret = vm_munmap(old.userspace_addr,
-				old.npages * PAGE_SIZE);
-		if (ret < 0)
-			printk(KERN_WARNING
-			       "kvm_vm_ioctl_set_memory_region: "
-			       "failed to munmap memory\n");
-	}
-
 	if (!kvm->arch.n_requested_mmu_pages)
 		nr_mmu_pages = kvm_mmu_calculate_mmu_pages(kvm);
 
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index ecc5543..8f2a863 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -436,7 +436,8 @@ int kvm_set_memory_region(struct kvm *kvm,
 int __kvm_set_memory_region(struct kvm *kvm,
 			    struct kvm_userspace_memory_region *mem,
 			    int user_alloc);
-void kvm_arch_free_memslot(struct kvm_memory_slot *free,
+void kvm_arch_free_memslot(struct kvm *kvm,
+                           struct kvm_memory_slot *free,
 			   struct kvm_memory_slot *dont);
 int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages);
 int kvm_arch_prepare_memory_region(struct kvm *kvm,
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index be70035..ea63b9c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -546,13 +546,14 @@ static void kvm_destroy_dirty_bitmap(struct kvm_memory_slot *memslot)
 /*
  * Free any memory in @free but not in @dont.
  */
-static void kvm_free_physmem_slot(struct kvm_memory_slot *free,
+static void kvm_free_physmem_slot(struct kvm *kvm,
+				  struct kvm_memory_slot *free,
 				  struct kvm_memory_slot *dont)
 {
 	if (!dont || free->dirty_bitmap != dont->dirty_bitmap)
 		kvm_destroy_dirty_bitmap(free);
 
-	kvm_arch_free_memslot(free, dont);
+	kvm_arch_free_memslot(kvm, free, dont);
 
 	free->npages = 0;
 }
@@ -563,7 +564,7 @@ void kvm_free_physmem(struct kvm *kvm)
 	struct kvm_memory_slot *memslot;
 
 	kvm_for_each_memslot(memslot, slots)
-		kvm_free_physmem_slot(memslot, NULL);
+		kvm_free_physmem_slot(kvm, memslot, NULL);
 
 	kfree(kvm->memslots);
 }
@@ -851,13 +852,13 @@ int __kvm_set_memory_region(struct kvm *kvm,
 
 	kvm_arch_commit_memory_region(kvm, mem, old, user_alloc);
 
-	kvm_free_physmem_slot(&old, &new);
+	kvm_free_physmem_slot(kvm, &old, &new);
 	kfree(old_memslots);
 
 	return 0;
 
 out_free:
-	kvm_free_physmem_slot(&new, &old);
+	kvm_free_physmem_slot(kvm, &new, &old);
 out:
 	return r;
 

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCHv2] KVM: x86: Fix memory leak in vmx.c
  2013-04-17 21:55 ` Paolo Bonzini
@ 2013-04-17 23:03   ` Andrew Honig
  2013-04-18  8:20     ` Paolo Bonzini
  0 siblings, 1 reply; 7+ messages in thread
From: Andrew Honig @ 2013-04-17 23:03 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: kvm

I don't have a significant objection to freeing the memory in
kvm_arch_free_memslot, although I think it's a little harder to
understand.  I like the idea of being symmetric (memory is allocated
by calling kvm_set_memory_region and freed using the same technique).
That way if someone changes from vm_mmap to something else it will be
obvious that they need to change both.

Also, it looks like your patch is based on something several commits
behind HEAD on virt/kvm/kvm.git, which significantly affect your
patch.  In the HEAD version it assumes that user_alloc is always set
unless it's a private memslot.  This appears to already have been the
case and allows a bunch of simplifications, some of which would apply
to your patch.

>
> What about something like this (uncompiled/untested)
>
>
> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
> index 8b3a9c0..6706134 100644
> --- a/arch/ia64/kvm/kvm-ia64.c
> +++ b/arch/ia64/kvm/kvm-ia64.c
> @@ -1563,7 +1563,8 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
>         return VM_FAULT_SIGBUS;
>  }
>
> -void kvm_arch_free_memslot(struct kvm_memory_slot *free,
> +void kvm_arch_free_memslot(struct kvm *kvm,
> +                          struct kvm_memory_slot *free,
>                            struct kvm_memory_slot *dont)
>  {
>  }
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 4d213b8..a654580 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -299,7 +299,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
>         return -EINVAL;
>  }
>
> -void kvm_arch_free_memslot(struct kvm_memory_slot *free,
> +void kvm_arch_free_memslot(struct kvm *kvm,
> +                          struct kvm_memory_slot *free,
>                            struct kvm_memory_slot *dont)
>  {
>         if (!dont || free->arch.rmap != dont->arch.rmap) {
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index ecced9d..e2159c1 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -912,7 +912,8 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
>         return VM_FAULT_SIGBUS;
>  }
>
> -void kvm_arch_free_memslot(struct kvm_memory_slot *free,
> +void kvm_arch_free_memslot(struct kvm *kvm,
> +                          struct kvm_memory_slot *free,
>                            struct kvm_memory_slot *dont)
>  {
>  }
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 224a7e7..f9fa0d1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6357,11 +6367,26 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>         kfree(rcu_dereference_check(kvm->arch.apic_map, 1));
>  }
>
> -void kvm_arch_free_memslot(struct kvm_memory_slot *free,
> +void kvm_arch_free_memslot(struct kvm *kvm,
> +                          struct kvm_memory_slot *free,
>                            struct kvm_memory_slot *dont)
>  {
>         int i;
>
> +       if (current->mm == kvm->mm && free->user_alloc) {
I think you mean !free->user_alloc.  Also, you could check the
memslot->id instead so that we can remove the user_alloc field
entirely as it doesn't serve a useful function anymore.

> +               if (!dont || !dont->user_alloc ||
> +                   free->userspace_addr != dont->userspace_addr) {
> +                       int ret;
> +
> +                       ret = vm_munmap(free->userspace_addr,
> +                                       free->npages * PAGE_SIZE);
> +                       if (ret < 0)
> +                               printk(KERN_WARNING
> +                                      "kvm_vm_ioctl_set_memory_region: "
> +                                      "failed to munmap memory\n");
> +               }
> +       }
> +
>         for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) {
>                 if (!dont || free->arch.rmap[i] != dont->arch.rmap[i]) {
>                         kvm_kvfree(free->arch.rmap[i]);
> @@ -6453,7 +6478,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>          *x86 needs to handle !user_alloc case.
>          */
>         if (!user_alloc) {
> -               if (npages && !old.npages) {
> +               if (npages != old.npages) {
>                         unsigned long userspace_addr;
>
>                         userspace_addr = vm_mmap(NULL, 0,
> @@ -6466,7 +6491,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
>                                 return PTR_ERR((void *)userspace_addr);
>
>                         memslot->userspace_addr = userspace_addr;
> -               }
> +               } else
> +                       memslot->userspace_addr = old.userspace_addr;
>         }
>
>
> @@ -6481,17 +6507,6 @@ void kvm_arch_commit_memory_region(struct kvm *kvm,
>
>         int nr_mmu_pages = 0, npages = mem->memory_size >> PAGE_SHIFT;
>
> -       if (!user_alloc && !old.user_alloc && old.npages && !npages) {
> -               int ret;
> -
> -               ret = vm_munmap(old.userspace_addr,
> -                               old.npages * PAGE_SIZE);
> -               if (ret < 0)
> -                       printk(KERN_WARNING
> -                              "kvm_vm_ioctl_set_memory_region: "
> -                              "failed to munmap memory\n");
> -       }
> -
>         if (!kvm->arch.n_requested_mmu_pages)
>                 nr_mmu_pages = kvm_mmu_calculate_mmu_pages(kvm);
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index ecc5543..8f2a863 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -436,7 +436,8 @@ int kvm_set_memory_region(struct kvm *kvm,
>  int __kvm_set_memory_region(struct kvm *kvm,
>                             struct kvm_userspace_memory_region *mem,
>                             int user_alloc);
> -void kvm_arch_free_memslot(struct kvm_memory_slot *free,
> +void kvm_arch_free_memslot(struct kvm *kvm,
> +                           struct kvm_memory_slot *free,
>                            struct kvm_memory_slot *dont);
>  int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages);
>  int kvm_arch_prepare_memory_region(struct kvm *kvm,
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index be70035..ea63b9c 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -546,13 +546,14 @@ static void kvm_destroy_dirty_bitmap(struct kvm_memory_slot *memslot)
>  /*
>   * Free any memory in @free but not in @dont.
>   */
> -static void kvm_free_physmem_slot(struct kvm_memory_slot *free,
> +static void kvm_free_physmem_slot(struct kvm *kvm,
> +                                 struct kvm_memory_slot *free,
>                                   struct kvm_memory_slot *dont)
>  {
>         if (!dont || free->dirty_bitmap != dont->dirty_bitmap)
>                 kvm_destroy_dirty_bitmap(free);
>
> -       kvm_arch_free_memslot(free, dont);
> +       kvm_arch_free_memslot(kvm, free, dont);
>
>         free->npages = 0;
>  }
> @@ -563,7 +564,7 @@ void kvm_free_physmem(struct kvm *kvm)
>         struct kvm_memory_slot *memslot;
>
>         kvm_for_each_memslot(memslot, slots)
> -               kvm_free_physmem_slot(memslot, NULL);
> +               kvm_free_physmem_slot(kvm, memslot, NULL);
>
>         kfree(kvm->memslots);
>  }
> @@ -851,13 +852,13 @@ int __kvm_set_memory_region(struct kvm *kvm,
>
>         kvm_arch_commit_memory_region(kvm, mem, old, user_alloc);
>
> -       kvm_free_physmem_slot(&old, &new);
> +       kvm_free_physmem_slot(kvm, &old, &new);
>         kfree(old_memslots);
>
>         return 0;
>
>  out_free:
> -       kvm_free_physmem_slot(&new, &old);
> +       kvm_free_physmem_slot(kvm, &new, &old);
>  out:
>         return r;
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCHv2] KVM: x86: Fix memory leak in vmx.c
  2013-04-17 23:03   ` Andrew Honig
@ 2013-04-18  8:20     ` Paolo Bonzini
  2013-04-18  8:50       ` Gleb Natapov
  0 siblings, 1 reply; 7+ messages in thread
From: Paolo Bonzini @ 2013-04-18  8:20 UTC (permalink / raw)
  To: Andrew Honig, kvm@vger.kernel.org

Il 18/04/2013 01:03, Andrew Honig ha scritto:
> I don't have a significant objection to freeing the memory in
> kvm_arch_free_memslot, although I think it's a little harder to
> understand.  I like the idea of being symmetric (memory is allocated
> by calling kvm_set_memory_region and freed using the same technique).
> That way if someone changes from vm_mmap to something else it will be
> obvious that they need to change both.
> 
> Also, it looks like your patch is based on something several commits
> behind HEAD on virt/kvm/kvm.git,

Yeah, it was just whatever version I had checked out on the laptop. :)
So that maintainers can look at both approaches and see what they prefer.

Gleb, Marcelo, wdyt?

Paolo

> which significantly affect your
> patch.  In the HEAD version it assumes that user_alloc is always set
> unless it's a private memslot.  This appears to already have been the
> case and allows a bunch of simplifications, some of which would apply
> to your patch.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCHv2] KVM: x86: Fix memory leak in vmx.c
  2013-04-18  8:20     ` Paolo Bonzini
@ 2013-04-18  8:50       ` Gleb Natapov
  0 siblings, 0 replies; 7+ messages in thread
From: Gleb Natapov @ 2013-04-18  8:50 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Andrew Honig, kvm@vger.kernel.org

On Thu, Apr 18, 2013 at 10:20:05AM +0200, Paolo Bonzini wrote:
> Il 18/04/2013 01:03, Andrew Honig ha scritto:
> > I don't have a significant objection to freeing the memory in
> > kvm_arch_free_memslot, although I think it's a little harder to
> > understand.  I like the idea of being symmetric (memory is allocated
> > by calling kvm_set_memory_region and freed using the same technique).
> > That way if someone changes from vm_mmap to something else it will be
> > obvious that they need to change both.
> > 
> > Also, it looks like your patch is based on something several commits
> > behind HEAD on virt/kvm/kvm.git,
> 
> Yeah, it was just whatever version I had checked out on the laptop. :)
> So that maintainers can look at both approaches and see what they prefer.
> 
> Gleb, Marcelo, wdyt?
> 
I agree with Andrew. Having kvm_arch_free_memslot() unmap memory,
but only for subset of memslots is not cleanest approach. Userspace
interface for slot deletion is to "create" the slot with zero size,
Andrew patch uses the same, well tested, code path.

--
			Gleb.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCHv2] KVM: x86: Fix memory leak in vmx.c
  2013-04-17 17:54 [PATCHv2] KVM: x86: Fix memory leak in vmx.c Andrew Honig
  2013-04-17 20:37 ` Eric Northup
  2013-04-17 21:55 ` Paolo Bonzini
@ 2013-04-18 10:21 ` Gleb Natapov
  2 siblings, 0 replies; 7+ messages in thread
From: Gleb Natapov @ 2013-04-18 10:21 UTC (permalink / raw)
  To: Andrew Honig; +Cc: kvm

On Wed, Apr 17, 2013 at 10:54:32AM -0700, Andrew Honig wrote:
> 
> If userspace creates and destroys multiple VMs within the same process
> we leak 20k of memory in the userspace process context per VM.  This
> patch frees the memory in kvm_arch_destroy_vm.  If the process exits
> without closing the VM file descriptor or the file descriptor has been
> shared with another process then we don't need to free the memory.
> 
> Signed-off-by: Andrew Honig <ahonig@google.com>
> ---
>  arch/x86/kvm/x86.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e172132..e93e16b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6811,6 +6811,23 @@ void kvm_arch_sync_events(struct kvm *kvm)
>  
>  void kvm_arch_destroy_vm(struct kvm *kvm)
>  {
> +	if (current->mm == kvm->mm) {
> +		/*
> +		 * Free memory regions allocated on behalf of userspace,
> +		 * unless the the memory map has changed due to process exit
> +		 * or fd copying.
> +		 */
> +		struct kvm_userspace_memory_region mem;
> +		memset(&mem, 0, sizeof(mem));
> +		mem.slot = APIC_ACCESS_PAGE_PRIVATE_MEMSLOT;
> +		kvm_set_memory_region(kvm, &mem, 0);
> +
> +		mem.slot = IDENTITY_PAGETABLE_PRIVATE_MEMSLOT;
> +		kvm_set_memory_region(kvm, &mem, 0);
> +
> +		mem.slot = TSS_PRIVATE_MEMSLOT;
> +		kvm_set_memory_region(kvm, &mem, 0);
You should code it against next branch. kvm_set_memory_region() has only
two parameters there.

> +	}
>  	kvm_iommu_unmap_guest(kvm);
>  	kfree(kvm->arch.vpic);
>  	kfree(kvm->arch.vioapic);
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
			Gleb.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2013-04-18 10:21 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-17 17:54 [PATCHv2] KVM: x86: Fix memory leak in vmx.c Andrew Honig
2013-04-17 20:37 ` Eric Northup
2013-04-17 21:55 ` Paolo Bonzini
2013-04-17 23:03   ` Andrew Honig
2013-04-18  8:20     ` Paolo Bonzini
2013-04-18  8:50       ` Gleb Natapov
2013-04-18 10:21 ` Gleb Natapov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox