Re: [PATCH] KVM: MMU: Fix rmap_remove() race

Andrea Arcangeli
2008-03-28T20:07:21Z

On Fri, Mar 28, 2008 at 03:01:13PM +0100, Andrea Arcangeli wrote:
> @@ -271,8 +292,12 @@ int __kvm_set_memory_region(struct kvm *kvm,
>
> r = -EINVAL;
> /* General sanity checks */
> + if (mem->userspace_addr & (PAGE_SIZE - 1))
> + goto out;
> if (mem->memory_size & (PAGE_SIZE - 1))
> goto out;
> + if (mem->userspace_addr + mem->memory_size < mem->userspace_addr)
> + goto out;

Those above (I suppose they're needed even if not strictly related to
the VM_LOCKED) broke vmx (only my laptop is vmx so I noticed it after
submission sorry!, the testing I did on svm test system before
submission worked fine and kvm was oom killed w/o swapping as
expected). I figured out the above likely broke older userland for the
same reason it broke vmx, so a new patch follows that should works for
vmx and should allow older userspace to still work and in VM_LOCKED
safe mode.

I'll try to make a new mmu notifier patch soon that will allow munmap
on a guest live gphys address space for the first time so ballooning
will be safe. Swapping is already safe and allowed with the current
kvm-swapping patch using the mmu notifiers, thanks to the mmu_lock
taken by invalidate_page that serializes the VM against
"spin_lock(mmu_lock); rmap_remove; tlbflush; spin_unlock(mmu_lock)".

Signed-off-by: Andrea Arcangeli <andrea@qumranet.com>

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 30bf832..8ece406 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -250,6 +250,24 @@ static int kvm_vm_release(struct inode *inode, struct file *filp)
return 0;
}

+#ifndef CONFIG_MMU_NOTIFIER
+static void memslot_mlock(unsigned long start, unsigned long len)
+{
+ struct vm_area_struct * vma;
+ unsigned long end = start+len;
+
+ vma = find_vma(current->mm, start);
+ if (!vma || vma->vm_start > start)
+ return;
+
+ /* go simple and don't split vmas */
+ for (;vma && vma->vm_start < end; vma = vma->vm_next) {
+ vma->vm_flags |= VM_LOCKED;
+ start = vma->vm_end;
+ }
+}
+#endif
+
/*
* Allocate some memory and give it an address in the guest physical address
* space.
@@ -273,6 +291,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
/* General sanity checks */
if (mem->memory_size & (PAGE_SIZE - 1))
goto out;
+ if (user_alloc &&
+ ((mem->userspace_addr & (PAGE_SIZE - 1)) ||
+ (mem->userspace_addr + mem->memory_size < mem->userspace_addr)))
+ goto out;
if (mem->guest_phys_addr & (PAGE_SIZE - 1))
goto out;
if (mem->slot >= KVM_MEMORY_SLOTS + KVM_PRIVATE_MEM_SLOTS)
@@ -366,6 +388,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
goto out_free;
}

+#ifndef CONFIG_MMU_NOTIFIER
+ memslot_mlock(mem->userspace_addr, mem->memory_size);
+#endif
+
kvm_free_physmem_slot(&old, &new);
return 0;