mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
@ 2018-02-08 21:35 David Rientjes
  2018-02-13 14:40 ` Paolo Bonzini
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: David Rientjes @ 2018-02-08 21:35 UTC (permalink / raw)
  To: Paolo Bonzini, Radim Krčmář; +Cc: linux-kernel, kvm

The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
This can be up to 4096 entries on architectures such as arm64 and s390
(and the upper bound may be increased on s390 eventually).

This can produce a vmalloc allocation failure warning:

vmalloc: allocation failure: 0 bytes, mode:0x24000c2(GFP_KERNEL|__GFP_HIGHMEM)
Call Trace:
 __dump_stack lib/dump_stack.c:15 [inline]
 dump_stack+0xf6/0x184 lib/dump_stack.c:51
 warn_alloc+0x208/0x230 mm/page_alloc.c:2930
 __vmalloc_node_range_memcg+0x510/0x670 mm/vmalloc.c:1711
 __vmalloc_node_memcg mm/vmalloc.c:1751 [inline]
 __vmalloc_node_memcg_flags mm/vmalloc.c:1788 [inline]
 vmalloc+0x69/0x70 mm/vmalloc.c:1803
 kvm_vm_ioctl+0x910/0x15e0 arch/x86/kvm/../../../virt/kvm/kvm_main.c:4153
 vfs_ioctl fs/ioctl.c:44 [inline]
 do_vfs_ioctl+0x842/0xee0 fs/ioctl.c:611
 SYSC_ioctl fs/ioctl.c:626 [inline]
 SyS_ioctl+0x94/0xc0 fs/ioctl.c:617
 entry_SYSCALL_64_fastpath+0x12/0x17

If the vmalloc address space is fully depleted, the ioctl can gracefully
fail.  Add __GFP_NOWARN to the allocation to suppress the warning.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 virt/kvm/kvm_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 001085b611ad..d0352dd45b95 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3063,7 +3063,8 @@ static long kvm_vm_ioctl(struct file *filp,
 			goto out;
 		if (routing.nr) {
 			r = -ENOMEM;
-			entries = vmalloc(routing.nr * sizeof(*entries));
+			entries = __vmalloc(routing.nr * sizeof(*entries),
+					GFP_KERNEL | __GFP_NOWARN, PAGE_KERNEL);
 			if (!entries)
 				goto out;
 			r = -EFAULT;

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-08 21:35 [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure David Rientjes
@ 2018-02-13 14:40 ` Paolo Bonzini
  2018-02-13 14:48 ` Michal Hocko
  2018-02-13 15:14 ` Christian Borntraeger
  2 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2018-02-13 14:40 UTC (permalink / raw)
  To: David Rientjes, Radim Krčmář; +Cc: linux-kernel, kvm

On 08/02/2018 22:35, David Rientjes wrote:
> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
> This can be up to 4096 entries on architectures such as arm64 and s390
> (and the upper bound may be increased on s390 eventually).
> 
> This can produce a vmalloc allocation failure warning:
> 
> vmalloc: allocation failure: 0 bytes, mode:0x24000c2(GFP_KERNEL|__GFP_HIGHMEM)
> Call Trace:
>  __dump_stack lib/dump_stack.c:15 [inline]
>  dump_stack+0xf6/0x184 lib/dump_stack.c:51
>  warn_alloc+0x208/0x230 mm/page_alloc.c:2930
>  __vmalloc_node_range_memcg+0x510/0x670 mm/vmalloc.c:1711
>  __vmalloc_node_memcg mm/vmalloc.c:1751 [inline]
>  __vmalloc_node_memcg_flags mm/vmalloc.c:1788 [inline]
>  vmalloc+0x69/0x70 mm/vmalloc.c:1803
>  kvm_vm_ioctl+0x910/0x15e0 arch/x86/kvm/../../../virt/kvm/kvm_main.c:4153
>  vfs_ioctl fs/ioctl.c:44 [inline]
>  do_vfs_ioctl+0x842/0xee0 fs/ioctl.c:611
>  SYSC_ioctl fs/ioctl.c:626 [inline]
>  SyS_ioctl+0x94/0xc0 fs/ioctl.c:617
>  entry_SYSCALL_64_fastpath+0x12/0x17
> 
> If the vmalloc address space is fully depleted, the ioctl can gracefully
> fail.  Add __GFP_NOWARN to the allocation to suppress the warning.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>

Queued, thanks.

Paolo

> ---
>  virt/kvm/kvm_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 001085b611ad..d0352dd45b95 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3063,7 +3063,8 @@ static long kvm_vm_ioctl(struct file *filp,
>  			goto out;
>  		if (routing.nr) {
>  			r = -ENOMEM;
> -			entries = vmalloc(routing.nr * sizeof(*entries));
> +			entries = __vmalloc(routing.nr * sizeof(*entries),
> +					GFP_KERNEL | __GFP_NOWARN, PAGE_KERNEL);
>  			if (!entries)
>  				goto out;
>  			r = -EFAULT;
> 

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-08 21:35 [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure David Rientjes
  2018-02-13 14:40 ` Paolo Bonzini
@ 2018-02-13 14:48 ` Michal Hocko
  2018-02-13 15:03   ` Paolo Bonzini
  2018-02-13 15:14 ` Christian Borntraeger
  2 siblings, 1 reply; 14+ messages in thread
From: Michal Hocko @ 2018-02-13 14:48 UTC (permalink / raw)
  To: David Rientjes
  Cc: Paolo Bonzini, Radim Krčmář, linux-kernel, kvm

On Thu 08-02-18 13:35:08, David Rientjes wrote:
> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
> This can be up to 4096 entries on architectures such as arm64 and s390
> (and the upper bound may be increased on s390 eventually).
> 
> This can produce a vmalloc allocation failure warning:
> 
> vmalloc: allocation failure: 0 bytes, mode:0x24000c2(GFP_KERNEL|__GFP_HIGHMEM)

I am not arguing about the kvm change but do we actaully want to warn
for 0 sized allocations? This just doesn't make much sense to me.
In other words don't we want this?

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 673942094328..c5d832510c54 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1748,7 +1748,9 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
 	unsigned long real_size = size;
 
 	size = PAGE_ALIGN(size);
-	if (!size || (size >> PAGE_SHIFT) > totalram_pages)
+	if (!size)
+		return NULL;
+	if ((size >> PAGE_SHIFT) > totalram_pages)
 		goto fail;
 
 	area = __get_vm_area_node(size, align, VM_ALLOC | VM_UNINITIALIZED |
-- 
Michal Hocko
SUSE Labs

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-13 14:48 ` Michal Hocko
@ 2018-02-13 15:03   ` Paolo Bonzini
  2018-02-13 15:44     ` Michal Hocko
  0 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2018-02-13 15:03 UTC (permalink / raw)
  To: Michal Hocko, David Rientjes
  Cc: Radim Krčmář, linux-kernel, kvm

On 13/02/2018 15:48, Michal Hocko wrote:
> On Thu 08-02-18 13:35:08, David Rientjes wrote:
>> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
>> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
>> This can be up to 4096 entries on architectures such as arm64 and s390
>> (and the upper bound may be increased on s390 eventually).
>>
>> This can produce a vmalloc allocation failure warning:
>>
>> vmalloc: allocation failure: 0 bytes, mode:0x24000c2(GFP_KERNEL|__GFP_HIGHMEM)
> 
> I am not arguing about the kvm change but do we actaully want to warn
> for 0 sized allocations? This just doesn't make much sense to me.
> In other words don't we want this?
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 673942094328..c5d832510c54 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1748,7 +1748,9 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
>  	unsigned long real_size = size;
>  
>  	size = PAGE_ALIGN(size);
> -	if (!size || (size >> PAGE_SHIFT) > totalram_pages)
> +	if (!size)
> +		return NULL;
> +	if ((size >> PAGE_SHIFT) > totalram_pages)
>  		goto fail;
>  
>  	area = __get_vm_area_node(size, align, VM_ALLOC | VM_UNINITIALIZED |
> 

There have been quite a few reports of this from syzkaller and generally
we've fixed them.  It does seem like a recipe for NULL-pointer
dereferences when the size is user-controlled (as in this case).

But here I'm actually not sure that the "allocation failure: 0 bytes"
can happen, since we have a check above for "if (routing.nr)", and there
is a check also so that the maximum allocation here is a meager 128 KiB.
 So I'm wondering if this patch is obsolete actually after commit
f8c1b85b2523.  David?

Thanks,

Paolo

Paolo

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-08 21:35 [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure David Rientjes
  2018-02-13 14:40 ` Paolo Bonzini
  2018-02-13 14:48 ` Michal Hocko
@ 2018-02-13 15:14 ` Christian Borntraeger
  2018-02-13 15:17   ` Paolo Bonzini
  2 siblings, 1 reply; 14+ messages in thread
From: Christian Borntraeger @ 2018-02-13 15:14 UTC (permalink / raw)
  To: David Rientjes, Paolo Bonzini, Radim Krčmář
  Cc: linux-kernel, kvm



On 02/08/2018 10:35 PM, David Rientjes wrote:
> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
> This can be up to 4096 entries on architectures such as arm64 and s390
> (and the upper bound may be increased on s390 eventually).
> 
> This can produce a vmalloc allocation failure warning:
> 
[...]
>  kvm_vm_ioctl+0x910/0x15e0 arch/x86/kvm/../../../virt/kvm/kvm_main.c:4153

                                                                       ^^^^^

> @@ -3063,7 +3063,8 @@ static long kvm_vm_ioctl(struct file *filp,

      ^^^^^


Are you sure that you got the right vmalloc?


>  			goto out;
>  		if (routing.nr) {
>  			r = -ENOMEM;
> -			entries = vmalloc(routing.nr * sizeof(*entries));
> +			entries = __vmalloc(routing.nr * sizeof(*entries),
> +					GFP_KERNEL | __GFP_NOWARN, PAGE_KERNEL);
>  			if (!entries)
>  				goto out;
>  			r = -EFAULT;
> 

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-13 15:14 ` Christian Borntraeger
@ 2018-02-13 15:17   ` Paolo Bonzini
  2018-02-14  1:03     ` David Rientjes
  0 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2018-02-13 15:17 UTC (permalink / raw)
  To: Christian Borntraeger, David Rientjes, Radim Krčmář
  Cc: linux-kernel, kvm

On 13/02/2018 16:14, Christian Borntraeger wrote:
> On 02/08/2018 10:35 PM, David Rientjes wrote:
>> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
>> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
>> This can be up to 4096 entries on architectures such as arm64 and s390
>> (and the upper bound may be increased on s390 eventually).
>>
>> This can produce a vmalloc allocation failure warning:
>>
> [...]
>>  kvm_vm_ioctl+0x910/0x15e0 arch/x86/kvm/../../../virt/kvm/kvm_main.c:4153
> 
>                                                                        ^^^^^
> 
>> @@ -3063,7 +3063,8 @@ static long kvm_vm_ioctl(struct file *filp,
> 
>       ^^^^^
> 
> 
> Are you sure that you got the right vmalloc?

Nice catch!  But well, it's the only one in the whole file. :)

That seems very much like an old patch then.  I'm unqueuing it.

Paolo

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-13 15:03   ` Paolo Bonzini
@ 2018-02-13 15:44     ` Michal Hocko
  2018-02-13 15:49       ` Paolo Bonzini
  0 siblings, 1 reply; 14+ messages in thread
From: Michal Hocko @ 2018-02-13 15:44 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: David Rientjes, Radim Krčmář, linux-kernel, kvm

On Tue 13-02-18 16:03:09, Paolo Bonzini wrote:
> On 13/02/2018 15:48, Michal Hocko wrote:
> > On Thu 08-02-18 13:35:08, David Rientjes wrote:
> >> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
> >> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
> >> This can be up to 4096 entries on architectures such as arm64 and s390
> >> (and the upper bound may be increased on s390 eventually).
> >>
> >> This can produce a vmalloc allocation failure warning:
> >>
> >> vmalloc: allocation failure: 0 bytes, mode:0x24000c2(GFP_KERNEL|__GFP_HIGHMEM)
> > 
> > I am not arguing about the kvm change but do we actaully want to warn
> > for 0 sized allocations? This just doesn't make much sense to me.
> > In other words don't we want this?
> > 
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 673942094328..c5d832510c54 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -1748,7 +1748,9 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
> >  	unsigned long real_size = size;
> >  
> >  	size = PAGE_ALIGN(size);
> > -	if (!size || (size >> PAGE_SHIFT) > totalram_pages)
> > +	if (!size)
> > +		return NULL;
> > +	if ((size >> PAGE_SHIFT) > totalram_pages)
> >  		goto fail;
> >  
> >  	area = __get_vm_area_node(size, align, VM_ALLOC | VM_UNINITIALIZED |
> > 
> 
> There have been quite a few reports of this from syzkaller and generally
> we've fixed them.  It does seem like a recipe for NULL-pointer
> dereferences when the size is user-controlled (as in this case).

We do return NULL for that case regardless the above. The patch just
doesn't warn. Or do you think it is helpful to warn?
-- 
Michal Hocko
SUSE Labs

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-13 15:44     ` Michal Hocko
@ 2018-02-13 15:49       ` Paolo Bonzini
  2018-02-13 15:58         ` Michal Hocko
  0 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2018-02-13 15:49 UTC (permalink / raw)
  To: Michal Hocko
  Cc: David Rientjes, Radim Krčmář, linux-kernel, kvm

On 13/02/2018 16:44, Michal Hocko wrote:
> On Tue 13-02-18 16:03:09, Paolo Bonzini wrote:
>> On 13/02/2018 15:48, Michal Hocko wrote:
>>> On Thu 08-02-18 13:35:08, David Rientjes wrote:
>>>> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
>>>> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
>>>> This can be up to 4096 entries on architectures such as arm64 and s390
>>>> (and the upper bound may be increased on s390 eventually).
>>>>
>>>> This can produce a vmalloc allocation failure warning:
>>>>
>>>> vmalloc: allocation failure: 0 bytes, mode:0x24000c2(GFP_KERNEL|__GFP_HIGHMEM)
>>>
>>> I am not arguing about the kvm change but do we actaully want to warn
>>> for 0 sized allocations? This just doesn't make much sense to me.
>>> In other words don't we want this?
>>>
>>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>>> index 673942094328..c5d832510c54 100644
>>> --- a/mm/vmalloc.c
>>> +++ b/mm/vmalloc.c
>>> @@ -1748,7 +1748,9 @@ void *__vmalloc_node_range(unsigned long size, unsigned long align,
>>>  	unsigned long real_size = size;
>>>  
>>>  	size = PAGE_ALIGN(size);
>>> -	if (!size || (size >> PAGE_SHIFT) > totalram_pages)
>>> +	if (!size)
>>> +		return NULL;
>>> +	if ((size >> PAGE_SHIFT) > totalram_pages)
>>>  		goto fail;
>>>  
>>>  	area = __get_vm_area_node(size, align, VM_ALLOC | VM_UNINITIALIZED |
>>>
>>
>> There have been quite a few reports of this from syzkaller and generally
>> we've fixed them.  It does seem like a recipe for NULL-pointer
>> dereferences when the size is user-controlled (as in this case).
> 
> We do return NULL for that case regardless the above. The patch just
> doesn't warn. Or do you think it is helpful to warn?

It certainly helps bringing potential issues in the spotlight (through
fuzzing, mostly).

Paolo

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-13 15:49       ` Paolo Bonzini
@ 2018-02-13 15:58         ` Michal Hocko
  0 siblings, 0 replies; 14+ messages in thread
From: Michal Hocko @ 2018-02-13 15:58 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: David Rientjes, Radim Krčmář, linux-kernel, kvm

On Tue 13-02-18 16:49:20, Paolo Bonzini wrote:
> On 13/02/2018 16:44, Michal Hocko wrote:
> > On Tue 13-02-18 16:03:09, Paolo Bonzini wrote:
[...]
> >> There have been quite a few reports of this from syzkaller and generally
> >> we've fixed them.  It does seem like a recipe for NULL-pointer
> >> dereferences when the size is user-controlled (as in this case).
> > 
> > We do return NULL for that case regardless the above. The patch just
> > doesn't warn. Or do you think it is helpful to warn?
> 
> It certainly helps bringing potential issues in the spotlight (through
> fuzzing, mostly).

Fair enough.

-- 
Michal Hocko
SUSE Labs

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-13 15:17   ` Paolo Bonzini
@ 2018-02-14  1:03     ` David Rientjes
  2018-02-14  8:30       ` Christian Borntraeger
  2018-02-14 10:10       ` Paolo Bonzini
  0 siblings, 2 replies; 14+ messages in thread
From: David Rientjes @ 2018-02-14  1:03 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Christian Borntraeger, Radim Krčmář, linux-kernel, kvm

On Tue, 13 Feb 2018, Paolo Bonzini wrote:

> >> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
> >> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
> >> This can be up to 4096 entries on architectures such as arm64 and s390
> >> (and the upper bound may be increased on s390 eventually).
> >>
> >> This can produce a vmalloc allocation failure warning:
> >>
> > [...]
> >>  kvm_vm_ioctl+0x910/0x15e0 arch/x86/kvm/../../../virt/kvm/kvm_main.c:4153
> > 
> >                                                                        ^^^^^
> > 
> >> @@ -3063,7 +3063,8 @@ static long kvm_vm_ioctl(struct file *filp,
> > 
> >       ^^^^^
> > 
> > 
> > Are you sure that you got the right vmalloc?
> 
> Nice catch!  But well, it's the only one in the whole file. :)
> 
> That seems very much like an old patch then.  I'm unqueuing it.
> 

It's not a catch at all, the fact that I saw this warning with an older 
kernel for KVM_SET_GSI_ROUTING doesn't mean that I can't patch it with an 
upstream kernel.  Would you prefer I remove the stack trace completely?

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-14  1:03     ` David Rientjes
@ 2018-02-14  8:30       ` Christian Borntraeger
  2018-02-14 10:10       ` Paolo Bonzini
  1 sibling, 0 replies; 14+ messages in thread
From: Christian Borntraeger @ 2018-02-14  8:30 UTC (permalink / raw)
  To: David Rientjes, Paolo Bonzini
  Cc: Radim Krčmář, linux-kernel, kvm



On 02/14/2018 02:03 AM, David Rientjes wrote:
> On Tue, 13 Feb 2018, Paolo Bonzini wrote:
> 
>>>> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
>>>> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
>>>> This can be up to 4096 entries on architectures such as arm64 and s390
>>>> (and the upper bound may be increased on s390 eventually).
>>>>
>>>> This can produce a vmalloc allocation failure warning:
>>>>
>>> [...]
>>>>  kvm_vm_ioctl+0x910/0x15e0 arch/x86/kvm/../../../virt/kvm/kvm_main.c:4153
>>>
>>>                                                                        ^^^^^
>>>
>>>> @@ -3063,7 +3063,8 @@ static long kvm_vm_ioctl(struct file *filp,
>>>
>>>       ^^^^^
>>>
>>>
>>> Are you sure that you got the right vmalloc?
>>
>> Nice catch!  But well, it's the only one in the whole file. :)
>>
>> That seems very much like an old patch then.  I'm unqueuing it.
>>
> 
> It's not a catch at all, the fact that I saw this warning with an older 
> kernel for KVM_SET_GSI_ROUTING doesn't mean that I can't patch it with an 
> upstream kernel.  Would you prefer I remove the stack trace completely?

FWIW, your stack trace did not complain about a too big allocation, it
complained about 0 allocation:

----- snip ------
vmalloc: allocation failure: 0 bytes, mode:0x24000c2(GFP_KERNEL|__GFP_HIGHMEM)
----- snip ------

After commit f8c1b85b2523 ("KVM: x86: avoid vmalloc(0) in the KVM_SET_CPUID)"
this case should be prevented. The only question is does your patch makes sense
nevertheless as we gracefully handle the ENOMEM case? So a reproducer on
a newer kernel would be good. Maybe use the "vmalloc" kernel parameter to force
this. 

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-14  1:03     ` David Rientjes
  2018-02-14  8:30       ` Christian Borntraeger
@ 2018-02-14 10:10       ` Paolo Bonzini
  2018-02-14 11:14         ` Christian Borntraeger
  1 sibling, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2018-02-14 10:10 UTC (permalink / raw)
  To: David Rientjes
  Cc: Christian Borntraeger, Radim Krčmář, linux-kernel, kvm

On 14/02/2018 02:03, David Rientjes wrote:
> On Tue, 13 Feb 2018, Paolo Bonzini wrote:
> 
>>>> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
>>>> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
>>>> This can be up to 4096 entries on architectures such as arm64 and s390
>>>> (and the upper bound may be increased on s390 eventually).
>>>>
>>>> This can produce a vmalloc allocation failure warning:
>>>>
>>> [...]
>>>>  kvm_vm_ioctl+0x910/0x15e0 arch/x86/kvm/../../../virt/kvm/kvm_main.c:4153
>>>
>>>                                                                        ^^^^^
>>>
>>>> @@ -3063,7 +3063,8 @@ static long kvm_vm_ioctl(struct file *filp,
>>>
>>>       ^^^^^
>>>
>>>
>>> Are you sure that you got the right vmalloc?
>>
>> Nice catch!  But well, it's the only one in the whole file. :)
>>
>> That seems very much like an old patch then.  I'm unqueuing it.
>>
> 
> It's not a catch at all, the fact that I saw this warning with an older 
> kernel for KVM_SET_GSI_ROUTING doesn't mean that I can't patch it with an 
> upstream kernel.  Would you prefer I remove the stack trace completely?

The upstream kernel doesn't warn.  It checks "if (routing.nr)" before
calling vmalloc.

Paolo

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-14 10:10       ` Paolo Bonzini
@ 2018-02-14 11:14         ` Christian Borntraeger
  2018-02-14 11:32           ` Paolo Bonzini
  0 siblings, 1 reply; 14+ messages in thread
From: Christian Borntraeger @ 2018-02-14 11:14 UTC (permalink / raw)
  To: Paolo Bonzini, David Rientjes
  Cc: Radim Krčmář, linux-kernel, kvm



On 02/14/2018 11:10 AM, Paolo Bonzini wrote:
> On 14/02/2018 02:03, David Rientjes wrote:
>> On Tue, 13 Feb 2018, Paolo Bonzini wrote:
>>
>>>>> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
>>>>> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
>>>>> This can be up to 4096 entries on architectures such as arm64 and s390
>>>>> (and the upper bound may be increased on s390 eventually).
>>>>>
>>>>> This can produce a vmalloc allocation failure warning:
>>>>>
>>>> [...]
>>>>>  kvm_vm_ioctl+0x910/0x15e0 arch/x86/kvm/../../../virt/kvm/kvm_main.c:4153
>>>>
>>>>                                                                        ^^^^^
>>>>
>>>>> @@ -3063,7 +3063,8 @@ static long kvm_vm_ioctl(struct file *filp,
>>>>
>>>>       ^^^^^
>>>>
>>>>
>>>> Are you sure that you got the right vmalloc?
>>>
>>> Nice catch!  But well, it's the only one in the whole file. :)
>>>
>>> That seems very much like an old patch then.  I'm unqueuing it.
>>>
>>
>> It's not a catch at all, the fact that I saw this warning with an older 
>> kernel for KVM_SET_GSI_ROUTING doesn't mean that I can't patch it with an 
>> upstream kernel.  Would you prefer I remove the stack trace completely?
> 
> The upstream kernel doesn't warn.  It checks "if (routing.nr)" before
> calling vmalloc.

It will warn of the vmalloc space is really exhausted. But then I really ask
myself if we really want to suppress this warning. This should be a big
ALERT to the host admin.

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

* Re: [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure
  2018-02-14 11:14         ` Christian Borntraeger
@ 2018-02-14 11:32           ` Paolo Bonzini
  0 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2018-02-14 11:32 UTC (permalink / raw)
  To: Christian Borntraeger, David Rientjes
  Cc: Radim Krčmář, linux-kernel, kvm

On 14/02/2018 12:14, Christian Borntraeger wrote:
> 
> 
> On 02/14/2018 11:10 AM, Paolo Bonzini wrote:
>> On 14/02/2018 02:03, David Rientjes wrote:
>>> On Tue, 13 Feb 2018, Paolo Bonzini wrote:
>>>
>>>>>> The KVM_SET_GSI_ROUTING ioctl does a vmalloc() of
>>>>>> sizeof(struct kvm_irq_routing_entry) multiplied by a user-supplied value.
>>>>>> This can be up to 4096 entries on architectures such as arm64 and s390
>>>>>> (and the upper bound may be increased on s390 eventually).
>>>>>>
>>>>>> This can produce a vmalloc allocation failure warning:
>>>>>>
>>>>> [...]
>>>>>>  kvm_vm_ioctl+0x910/0x15e0 arch/x86/kvm/../../../virt/kvm/kvm_main.c:4153
>>>>>
>>>>>                                                                        ^^^^^
>>>>>
>>>>>> @@ -3063,7 +3063,8 @@ static long kvm_vm_ioctl(struct file *filp,
>>>>>
>>>>>       ^^^^^
>>>>>
>>>>>
>>>>> Are you sure that you got the right vmalloc?
>>>>
>>>> Nice catch!  But well, it's the only one in the whole file. :)
>>>>
>>>> That seems very much like an old patch then.  I'm unqueuing it.
>>>>
>>>
>>> It's not a catch at all, the fact that I saw this warning with an older 
>>> kernel for KVM_SET_GSI_ROUTING doesn't mean that I can't patch it with an 
>>> upstream kernel.  Would you prefer I remove the stack trace completely?
>>
>> The upstream kernel doesn't warn.  It checks "if (routing.nr)" before
>> calling vmalloc.
> 
> It will warn of the vmalloc space is really exhausted. But then I really ask
> myself if we really want to suppress this warning. This should be a big
> ALERT to the host admin.

Especially since the biggest allocation KVM_SET_GSI_ROUTING can do is
128 KiB...

Paolo

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

end of thread, other threads:[~2018-02-14 11:32 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-08 21:35 [patch] kvm: suppress KVM_SET_GSI_ROUTING allocation failure David Rientjes
2018-02-13 14:40 ` Paolo Bonzini
2018-02-13 14:48 ` Michal Hocko
2018-02-13 15:03   ` Paolo Bonzini
2018-02-13 15:44     ` Michal Hocko
2018-02-13 15:49       ` Paolo Bonzini
2018-02-13 15:58         ` Michal Hocko
2018-02-13 15:14 ` Christian Borntraeger
2018-02-13 15:17   ` Paolo Bonzini
2018-02-14  1:03     ` David Rientjes
2018-02-14  8:30       ` Christian Borntraeger
2018-02-14 10:10       ` Paolo Bonzini
2018-02-14 11:14         ` Christian Borntraeger
2018-02-14 11:32           ` Paolo Bonzini

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

all inboxes | Powered by JetHome®