Re: [PATCH 2/2] KVM: Fix kvm_free_physmem_slot memory leak.

Re: [PATCH 2/2] KVM: Fix kvm_free_physmem_slot memory leak.

[François Diakhate]

[Updated the patch taking your comments into account]

Make sure that kvm_free_physmem_slot also frees the VM memory
if it was allocated by the kernel.

Signed-off-by: François Diakhaté <fdiakh@gmail.com>
---
 arch/x86/kvm/x86.c  |   10 +++++-----
 virt/kvm/kvm_main.c |   18 ++++++++++++++----
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 883c137..818220b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4179,13 +4179,13 @@ int kvm_arch_set_memory_region(struct kvm *kvm,
 		if (npages && !old.rmap) {
 			unsigned long userspace_addr;

-			down_write(&current->mm->mmap_sem);
+			down_write(&kvm->mm->mmap_sem);
 			userspace_addr = do_mmap(NULL, 0,
 						 npages * PAGE_SIZE,
 						 PROT_READ | PROT_WRITE,
 						 MAP_PRIVATE | MAP_ANONYMOUS,
 						 0);
-			up_write(&current->mm->mmap_sem);
+			up_write(&kvm->mm->mmap_sem);

 			if (IS_ERR((void *)userspace_addr))
 				return PTR_ERR((void *)userspace_addr);
@@ -4198,10 +4198,10 @@ int kvm_arch_set_memory_region(struct kvm *kvm,
 			if (!old.user_alloc && old.rmap) {
 				int ret;

-				down_write(&current->mm->mmap_sem);
-				ret = do_munmap(current->mm, old.userspace_addr,
+				down_write(&kvm->mm->mmap_sem);
+				ret = do_munmap(kvm->mm, old.userspace_addr,
 						old.npages * PAGE_SIZE);
-				up_write(&current->mm->mmap_sem);
+				up_write(&kvm->mm->mmap_sem);
 				if (ret < 0)
 					printk(KERN_WARNING
 				       "kvm_vm_ioctl_set_memory_region: "
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a87f45e..b420930 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -617,9 +617,19 @@ out:
 /*
  * 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->userspace_addr != dont->userspace_addr) {
+		struct kvm_userspace_memory_region mem = {
+			.slot = memslot_id(kvm, free),
+			.guest_phys_addr = free->base_gfn << PAGE_SHIFT,
+			.memory_size = 0,
+			.flags = 0,
+		};
+		kvm_arch_set_memory_region(kvm, &mem, *free, free->user_alloc);
+	}
+
 	if (!dont || free->rmap != dont->rmap)
 		vfree(free->rmap);

@@ -640,7 +650,7 @@ void kvm_free_physmem(struct kvm *kvm)
 	int i;

 	for (i = 0; i < kvm->nmemslots; ++i)
-		kvm_free_physmem_slot(&kvm->memslots[i], NULL);
+		kvm_free_physmem_slot(kvm, &kvm->memslots[i], NULL);
 }

 static void kvm_destroy_vm(struct kvm *kvm)
@@ -821,7 +831,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 		goto out_free;
 	}

-	kvm_free_physmem_slot(&old, &new);
+	kvm_free_physmem_slot(kvm, &old, &new);
 #ifdef CONFIG_DMAR
 	/* map the pages in iommu page table */
 	r = kvm_iommu_map_pages(kvm, base_gfn, npages);
@@ -831,7 +841,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	return 0;

 out_free:
-	kvm_free_physmem_slot(&new, &old);
+	kvm_free_physmem_slot(kvm, &new, &old);
 out:
 	return r;

-- 
1.6.0.3

Re: [PATCH 2/2] KVM: Fix kvm_free_physmem_slot memory leak.

[François Diakhate]

[Sorry, I realized I forgot to check style, here is the fixed patch]

Make sure that kvm_free_physmem_slot also frees the VM memory
if it was allocated by the kernel.

Signed-off-by: François Diakhaté <fdiakh@gmail.com>
---
 arch/x86/kvm/x86.c  |   10 +++++-----
 virt/kvm/kvm_main.c |   19 +++++++++++++++----
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 883c137..818220b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4179,13 +4179,13 @@ int kvm_arch_set_memory_region(struct kvm *kvm,
 		if (npages && !old.rmap) {
 			unsigned long userspace_addr;

-			down_write(&current->mm->mmap_sem);
+			down_write(&kvm->mm->mmap_sem);
 			userspace_addr = do_mmap(NULL, 0,
 						 npages * PAGE_SIZE,
 						 PROT_READ | PROT_WRITE,
 						 MAP_PRIVATE | MAP_ANONYMOUS,
 						 0);
-			up_write(&current->mm->mmap_sem);
+			up_write(&kvm->mm->mmap_sem);

 			if (IS_ERR((void *)userspace_addr))
 				return PTR_ERR((void *)userspace_addr);
@@ -4198,10 +4198,10 @@ int kvm_arch_set_memory_region(struct kvm *kvm,
 			if (!old.user_alloc && old.rmap) {
 				int ret;

-				down_write(&current->mm->mmap_sem);
-				ret = do_munmap(current->mm, old.userspace_addr,
+				down_write(&kvm->mm->mmap_sem);
+				ret = do_munmap(kvm->mm, old.userspace_addr,
 						old.npages * PAGE_SIZE);
-				up_write(&current->mm->mmap_sem);
+				up_write(&kvm->mm->mmap_sem);
 				if (ret < 0)
 					printk(KERN_WARNING
 				       "kvm_vm_ioctl_set_memory_region: "
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a87f45e..c7d6585 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -617,9 +617,20 @@ out:
 /*
  * 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->userspace_addr != dont->userspace_addr) {
+		struct kvm_userspace_memory_region mem = {
+			.slot = memslot_id(kvm, free),
+			.guest_phys_addr = free->base_gfn << PAGE_SHIFT,
+			.memory_size = 0,
+			.flags = 0,
+		};
+		kvm_arch_set_memory_region(kvm, &mem, *free, free->user_alloc);
+	}
+
 	if (!dont || free->rmap != dont->rmap)
 		vfree(free->rmap);

@@ -640,7 +651,7 @@ void kvm_free_physmem(struct kvm *kvm)
 	int i;

 	for (i = 0; i < kvm->nmemslots; ++i)
-		kvm_free_physmem_slot(&kvm->memslots[i], NULL);
+		kvm_free_physmem_slot(kvm, &kvm->memslots[i], NULL);
 }

 static void kvm_destroy_vm(struct kvm *kvm)
@@ -821,7 +832,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 		goto out_free;
 	}

-	kvm_free_physmem_slot(&old, &new);
+	kvm_free_physmem_slot(kvm, &old, &new);
 #ifdef CONFIG_DMAR
 	/* map the pages in iommu page table */
 	r = kvm_iommu_map_pages(kvm, base_gfn, npages);
@@ -831,7 +842,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	return 0;

 out_free:
-	kvm_free_physmem_slot(&new, &old);
+	kvm_free_physmem_slot(kvm, &new, &old);
 out:
 	return r;

-- 
1.6.0.3

Re: [PATCH 2/2] KVM: Fix kvm_free_physmem_slot memory leak.

[Avi Kivity]

(late reply, sorry)

François Diakhate wrote:
> [Sorry, I realized I forgot to check style, here is the fixed patch]
>
> Make sure that kvm_free_physmem_slot also frees the VM memory
> if it was allocated by the kernel.
>
>  /*
>   * 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->userspace_addr != dont->userspace_addr) {
> +		struct kvm_userspace_memory_region mem = {
> +			.slot = memslot_id(kvm, free),
> +			.guest_phys_addr = free->base_gfn << PAGE_SHIFT,
> +			.memory_size = 0,
> +			.flags = 0,
> +		};
> +		kvm_arch_set_memory_region(kvm, &mem, *free, free->user_alloc);
> +	}
> +
>  	if (!dont || free->rmap != dont->rmap)
>   

What happens here if the both free and dont have nonzero, differnt 
->userspace_addr values?  Is is even possible?

Also, the call chain is fishy.  set_memory_region calls 
free_physmem_slot which calls arch_set_memory_region.  This is turning 
into pasta.


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

Re: [PATCH 2/2] KVM: Fix kvm_free_physmem_slot memory leak.

[François Diakhate]

On Thu, Nov 6, 2008 at 4:14 PM, Avi Kivity <avi@redhat.com> wrote:
> What happens here if the both free and dont have nonzero, differnt
> ->userspace_addr values?  Is is even possible?

I dont think it can happen in the current kvm code, but I put that test in
order to respect the function behaviour of freeing any memory allocation
pointed to by free and not by dont (as described in the comment).

> Also, the call chain is fishy.  set_memory_region calls free_physmem_slot
> which calls arch_set_memory_region.  This is turning into pasta.

I agree, that's why I thought it would be better to put this outside
kvm_free_physmem_slot in my first patch. AFAICT, kvm_free_physmem_slot
is called by kvm_set_memory_region in order to free the memory holding
information regarding the slot but not the actual memory region held
by the slot: precisely because it is the role of kvm_set_memory_region
to free it.

So here is an attempt at something cleaner:
1 - Rename kvm_free_physmem_slot to kvm_free_physmem_slot_info
to indicate that it only frees the memory storing information about the slot
and not the memory region.

2- Make kvm_free_physmem free memory regions through
kvm_set_memory_region and let it free the slot info.


diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a87f45e..e59dc10 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -614,11 +614,24 @@ out:
 	return kvm;
 }

+static void kvm_free_physmem_slot(struct kvm *kvm,
+				  struct kvm_memory_slot *slot)
+{
+	struct kvm_userspace_memory_region mem = {
+		.slot = memslot_id(kvm, slot),
+		.guest_phys_addr = slot->base_gfn << PAGE_SHIFT,
+		.memory_size = 0,
+		.flags = 0,
+	};
+
+	kvm_set_memory_region(kvm, &mem, slot->user_alloc);
+}
+
 /*
  * Free any memory in @free but not in @dont.
  */
-static void kvm_free_physmem_slot(struct kvm_memory_slot *free,
-				  struct kvm_memory_slot *dont)
+static void kvm_free_physmem_slot_info(struct kvm_memory_slot *free,
+				       struct kvm_memory_slot *dont)
 {
 	if (!dont || free->rmap != dont->rmap)
 		vfree(free->rmap);
@@ -640,7 +653,7 @@ void kvm_free_physmem(struct kvm *kvm)
 	int i;

 	for (i = 0; i < kvm->nmemslots; ++i)
-		kvm_free_physmem_slot(&kvm->memslots[i], NULL);
+		kvm_free_physmem_slot(kvm, &kvm->memslots[i]);
 }

 static void kvm_destroy_vm(struct kvm *kvm)
@@ -745,10 +758,14 @@ int __kvm_set_memory_region(struct kvm *kvm,
 			goto out_free;
 	}

-	/* Free page dirty bitmap if unneeded */
+	/* Free any unneeded data */
 	if (!(new.flags & KVM_MEM_LOG_DIRTY_PAGES))
 		new.dirty_bitmap = NULL;

+	if (!npages) {
+		new.rmap = NULL;
+		new.lpage_info = NULL;
+	}
 	r = -ENOMEM;

 	/* Allocate if a slot is being created */
@@ -821,7 +838,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 		goto out_free;
 	}

-	kvm_free_physmem_slot(&old, &new);
+	kvm_free_physmem_slot_info(&old, &new);
 #ifdef CONFIG_DMAR
 	/* map the pages in iommu page table */
 	r = kvm_iommu_map_pages(kvm, base_gfn, npages);
@@ -831,7 +848,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	return 0;

 out_free:
-	kvm_free_physmem_slot(&new, &old);
+	kvm_free_physmem_slot_info(&new, &old);
 out:
 	return r;

Also, I've been reading a bit more about the linux mm and I now think
that to be able to use kvm->mm in arch_set_memory_region we need
to increase mm_users instead of mm_count. However, if we do that,
since memory maps wont be cleared when the process exits the kvm
fds which are still mapped in userspace will not be released so we will
have a bigger memory leak.
Any ideas on how to fix this properly?