* [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®