[PATCH] kvm: avoid page allocation failure in kvm_set_memory_region()

[PATCH] kvm: avoid page allocation failure in kvm_set_memory_region()

[Igor Mammedov]

KVM guest can fail to startup with following trace on host:

qemu-system-x86: page allocation failure: order:4, mode:0x40d0
Call Trace:
  dump_stack+0x47/0x67
  warn_alloc_failed+0xee/0x150
  __alloc_pages_direct_compact+0x14a/0x150
  __alloc_pages_nodemask+0x776/0xb80
  alloc_kmem_pages+0x3a/0x110
  kmalloc_order+0x13/0x50
  kmemdup+0x1b/0x40
  __kvm_set_memory_region+0x24a/0x9f0 [kvm]
  kvm_set_ioapic+0x130/0x130 [kvm]
  kvm_set_memory_region+0x21/0x40 [kvm]
  kvm_vm_ioctl+0x43f/0x750 [kvm]

Failure happens when attempting to allocate pages for
'struct kvm_memslots', however it doesn't have to be
present in physically contiguous (kmalloc-ed) address
space, change allocation to kvm_kvzalloc() so that
it will be vmalloc-ed when its size is more then a page.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
TODO:
 - work on follow up patches to allocate space for
   actual amount of memory_slots instead of possible maximum.
---
 virt/kvm/kvm_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a2214d9..7ed1f5c 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -871,10 +871,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
 			goto out_free;
 	}
 
-	slots = kmemdup(kvm->memslots, sizeof(struct kvm_memslots),
-			GFP_KERNEL);
+	slots = kvm_kvzalloc(sizeof(struct kvm_memslots));
 	if (!slots)
 		goto out_free;
+	memcpy(slots, kvm->memslots, sizeof(struct kvm_memslots));
 
 	if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE)) {
 		slot = id_to_memslot(slots, mem->slot);
@@ -936,7 +936,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	return 0;
 
 out_slots:
-	kfree(slots);
+	kvfree(slots);
 out_free:
 	kvm_free_physmem_slot(kvm, &new, &old);
 out:
-- 
1.8.3.1

Re: [PATCH] kvm: avoid page allocation failure in kvm_set_memory_region()

[Marcelo Tosatti]

On Fri, Mar 20, 2015 at 09:51:26AM +0000, Igor Mammedov wrote:
> KVM guest can fail to startup with following trace on host:
> 
> qemu-system-x86: page allocation failure: order:4, mode:0x40d0
> Call Trace:
>   dump_stack+0x47/0x67
>   warn_alloc_failed+0xee/0x150
>   __alloc_pages_direct_compact+0x14a/0x150
>   __alloc_pages_nodemask+0x776/0xb80
>   alloc_kmem_pages+0x3a/0x110
>   kmalloc_order+0x13/0x50
>   kmemdup+0x1b/0x40
>   __kvm_set_memory_region+0x24a/0x9f0 [kvm]
>   kvm_set_ioapic+0x130/0x130 [kvm]
>   kvm_set_memory_region+0x21/0x40 [kvm]
>   kvm_vm_ioctl+0x43f/0x750 [kvm]
> 
> Failure happens when attempting to allocate pages for
> 'struct kvm_memslots', however it doesn't have to be
> present in physically contiguous (kmalloc-ed) address
> space, change allocation to kvm_kvzalloc() so that
> it will be vmalloc-ed when its size is more then a page.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Igor, two things:

1) kvm_create_vm should also use vmalloc

        r = -ENOMEM;
        kvm->memslots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL);
        if (!kvm->memslots)
                goto out_err_no_srcu;

2) there are additional places where its necessary to use
proper freeing function, i believe:

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ce7888a..651ff2d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -522,7 +522,7 @@ out_err_no_srcu:
 out_err_no_disable:
 	for (i = 0; i < KVM_NR_BUSES; i++)
 		kfree(kvm->buses[i]);
-	kfree(kvm->memslots);
+	kvfree(kvm->memslots);
 	kvm_arch_free_vm(kvm);
 	return ERR_PTR(r);
 }
@@ -570,7 +570,7 @@ static void kvm_free_physmem(struct kvm *kvm)
 	kvm_for_each_memslot(memslot, slots)
 		kvm_free_physmem_slot(kvm, memslot, NULL);
 
-	kfree(kvm->memslots);
+	kvfree(kvm->memslots);
 }
 
 static void kvm_destroy_devices(struct kvm *kvm)
@@ -909,7 +922,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	kvm_arch_commit_memory_region(kvm, mem, &old, change);
 
 	kvm_free_physmem_slot(kvm, &old, &new);
-	kfree(old_memslots);
+	kvfree(old_memslots);
 
 	/*
 	 * IOMMU mapping:  New slots need to be mapped.  Old slots need to be

> ---
> TODO:
>  - work on follow up patches to allocate space for
>    actual amount of memory_slots instead of possible maximum.
> ---
>  virt/kvm/kvm_main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a2214d9..7ed1f5c 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -871,10 +871,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  			goto out_free;
>  	}
>  
> -	slots = kmemdup(kvm->memslots, sizeof(struct kvm_memslots),
> -			GFP_KERNEL);
> +	slots = kvm_kvzalloc(sizeof(struct kvm_memslots));
>  	if (!slots)
>  		goto out_free;
> +	memcpy(slots, kvm->memslots, sizeof(struct kvm_memslots));
>  
>  	if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE)) {
>  		slot = id_to_memslot(slots, mem->slot);
> @@ -936,7 +936,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  	return 0;
>  
>  out_slots:
> -	kfree(slots);
> +	kvfree(slots);
>  out_free:
>  	kvm_free_physmem_slot(kvm, &new, &old);
>  out:
> -- 
> 1.8.3.1

Re: [PATCH] kvm: avoid page allocation failure in kvm_set_memory_region()

[Igor Mammedov]

On Fri, 20 Mar 2015 08:59:03 -0300
Marcelo Tosatti <mtosatti@redhat.com> wrote:

> On Fri, Mar 20, 2015 at 09:51:26AM +0000, Igor Mammedov wrote:
> > KVM guest can fail to startup with following trace on host:
> > 
> > qemu-system-x86: page allocation failure: order:4, mode:0x40d0
> > Call Trace:
> >   dump_stack+0x47/0x67
> >   warn_alloc_failed+0xee/0x150
> >   __alloc_pages_direct_compact+0x14a/0x150
> >   __alloc_pages_nodemask+0x776/0xb80
> >   alloc_kmem_pages+0x3a/0x110
> >   kmalloc_order+0x13/0x50
> >   kmemdup+0x1b/0x40
> >   __kvm_set_memory_region+0x24a/0x9f0 [kvm]
> >   kvm_set_ioapic+0x130/0x130 [kvm]
> >   kvm_set_memory_region+0x21/0x40 [kvm]
> >   kvm_vm_ioctl+0x43f/0x750 [kvm]
> > 
> > Failure happens when attempting to allocate pages for
> > 'struct kvm_memslots', however it doesn't have to be
> > present in physically contiguous (kmalloc-ed) address
> > space, change allocation to kvm_kvzalloc() so that
> > it will be vmalloc-ed when its size is more then a page.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Igor, two things:
> 
> 1) kvm_create_vm should also use vmalloc
> 
>         r = -ENOMEM;
>         kvm->memslots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL);
>         if (!kvm->memslots)
>                 goto out_err_no_srcu;
> 
> 2) there are additional places where its necessary to use
> proper freeing function, i believe:
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index ce7888a..651ff2d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -522,7 +522,7 @@ out_err_no_srcu:
>  out_err_no_disable:
>  	for (i = 0; i < KVM_NR_BUSES; i++)
>  		kfree(kvm->buses[i]);
> -	kfree(kvm->memslots);
> +	kvfree(kvm->memslots);
>  	kvm_arch_free_vm(kvm);
>  	return ERR_PTR(r);
>  }
> @@ -570,7 +570,7 @@ static void kvm_free_physmem(struct kvm *kvm)
>  	kvm_for_each_memslot(memslot, slots)
>  		kvm_free_physmem_slot(kvm, memslot, NULL);
>  
> -	kfree(kvm->memslots);
> +	kvfree(kvm->memslots);
>  }
>  
>  static void kvm_destroy_devices(struct kvm *kvm)
> @@ -909,7 +922,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  	kvm_arch_commit_memory_region(kvm, mem, &old, change);
>  
>  	kvm_free_physmem_slot(kvm, &old, &new);
> -	kfree(old_memslots);
> +	kvfree(old_memslots);
>  
>  	/*
>  	 * IOMMU mapping:  New slots need to be mapped.  Old slots need to be
Thanks for review,
I'll respin patch after fixing it up.


> 
> > ---
> > TODO:
> >  - work on follow up patches to allocate space for
> >    actual amount of memory_slots instead of possible maximum.
> > ---
> >  virt/kvm/kvm_main.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index a2214d9..7ed1f5c 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -871,10 +871,10 @@ int __kvm_set_memory_region(struct kvm *kvm,
> >  			goto out_free;
> >  	}
> >  
> > -	slots = kmemdup(kvm->memslots, sizeof(struct kvm_memslots),
> > -			GFP_KERNEL);
> > +	slots = kvm_kvzalloc(sizeof(struct kvm_memslots));
> >  	if (!slots)
> >  		goto out_free;
> > +	memcpy(slots, kvm->memslots, sizeof(struct kvm_memslots));
> >  
> >  	if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE)) {
> >  		slot = id_to_memslot(slots, mem->slot);
> > @@ -936,7 +936,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
> >  	return 0;
> >  
> >  out_slots:
> > -	kfree(slots);
> > +	kvfree(slots);
> >  out_free:
> >  	kvm_free_physmem_slot(kvm, &new, &old);
> >  out:
> > -- 
> > 1.8.3.1