slab would still spew a warning when a big allocation happens with the
__GFP_NOWARN fleg is set. Prevent that to conform to __GFP_NOWARN.
Signed-off-by: Sasha Levin <[email protected]>
---
mm/slab_common.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index ff3218a..2d41450 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -373,8 +373,10 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
{
int index;
- if (WARN_ON_ONCE(size > KMALLOC_MAX_SIZE))
+ if (size > KMALLOC_MAX_SIZE) {
+ WARN_ON_ONCE(!(flags & __GFP_NOWARN));
return NULL;
+ }
if (size <= 192) {
if (!size)
--
1.8.2.1
Hello Sasha,
On Mon, Jun 10, 2013 at 10:18 PM, Sasha Levin <[email protected]> wrote:
> slab would still spew a warning when a big allocation happens with the
> __GFP_NOWARN fleg is set. Prevent that to conform to __GFP_NOWARN.
>
> Signed-off-by: Sasha Levin <[email protected]>
> ---
> mm/slab_common.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index ff3218a..2d41450 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -373,8 +373,10 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
> {
> int index;
>
> - if (WARN_ON_ONCE(size > KMALLOC_MAX_SIZE))
> + if (size > KMALLOC_MAX_SIZE) {
> + WARN_ON_ONCE(!(flags & __GFP_NOWARN));
> return NULL;
> + }
Does this fix a real problem you're seeing? __GFP_NOWARN is about not
warning if a memory allocation fails but this particular WARN_ON
suggests a kernel bug.
Pekka
On 06/10/2013 03:31 PM, Pekka Enberg wrote:
> Hello Sasha,
>
> On Mon, Jun 10, 2013 at 10:18 PM, Sasha Levin <[email protected]> wrote:
>> slab would still spew a warning when a big allocation happens with the
>> __GFP_NOWARN fleg is set. Prevent that to conform to __GFP_NOWARN.
>>
>> Signed-off-by: Sasha Levin <[email protected]>
>> ---
>> mm/slab_common.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index ff3218a..2d41450 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -373,8 +373,10 @@ struct kmem_cache *kmalloc_slab(size_t size, gfp_t flags)
>> {
>> int index;
>>
>> - if (WARN_ON_ONCE(size > KMALLOC_MAX_SIZE))
>> + if (size > KMALLOC_MAX_SIZE) {
>> + WARN_ON_ONCE(!(flags & __GFP_NOWARN));
>> return NULL;
>> + }
>
> Does this fix a real problem you're seeing? __GFP_NOWARN is about not
> warning if a memory allocation fails but this particular WARN_ON
> suggests a kernel bug.
It fixes this warning:
[ 1691.703002] WARNING: CPU: 15 PID: 21519 at mm/slab_common.c:376
kmalloc_slab+0x2f/0xb0()
[ 1691.706906] can: request_module (can-proto-4) failed.
[ 1691.707827] mpoa: proc_mpc_write: could not parse ''
[ 1691.713952] Modules linked in:
[ 1691.715199] CPU: 15 PID: 21519 Comm: trinity-child15 Tainted: G
W 3.10.0-rc4-next-20130607-sasha-00011-gcd78395-dirty #2
[ 1691.719669] 0000000000000009 ffff880020a95e30 ffffffff83ff4041
0000000000000000
[ 1691.797744] ffff880020a95e68 ffffffff8111fe12 fffffffffffffff0
00000000000082d0
[ 1691.802822] 0000000000080000 0000000000080000 0000000001400000
ffff880020a95e78
[ 1691.807621] Call Trace:
[ 1691.809473] [<ffffffff83ff4041>] dump_stack+0x4e/0x82
[ 1691.812783] [<ffffffff8111fe12>] warn_slowpath_common+0x82/0xb0
[ 1691.817011] [<ffffffff8111fe55>] warn_slowpath_null+0x15/0x20
[ 1691.819936] [<ffffffff81243dcf>] kmalloc_slab+0x2f/0xb0
[ 1691.824942] [<ffffffff81278d54>] __kmalloc+0x24/0x4b0
[ 1691.827285] [<ffffffff8196ffe3>] ? security_capable+0x13/0x20
[ 1691.829405] [<ffffffff812a26b7>] ? pipe_fcntl+0x107/0x210
[ 1691.831827] [<ffffffff812a26b7>] pipe_fcntl+0x107/0x210
[ 1691.833651] [<ffffffff812b7ea0>] ? fget_raw_light+0x130/0x3f0
[ 1691.835343] [<ffffffff812aa5fb>] SyS_fcntl+0x60b/0x6a0
[ 1691.837008] [<ffffffff8403ca98>] tracesys+0xe1/0xe6
The caller specifically sets __GFP_NOWARN presumably to avoid this
warning on slub but I'm not sure if there's any other reason.
Thanks,
Sasha
On Mon, 10 Jun 2013, Sasha Levin wrote:
> [ 1691.807621] Call Trace:
> [ 1691.809473] [<ffffffff83ff4041>] dump_stack+0x4e/0x82
> [ 1691.812783] [<ffffffff8111fe12>] warn_slowpath_common+0x82/0xb0
> [ 1691.817011] [<ffffffff8111fe55>] warn_slowpath_null+0x15/0x20
> [ 1691.819936] [<ffffffff81243dcf>] kmalloc_slab+0x2f/0xb0
> [ 1691.824942] [<ffffffff81278d54>] __kmalloc+0x24/0x4b0
> [ 1691.827285] [<ffffffff8196ffe3>] ? security_capable+0x13/0x20
> [ 1691.829405] [<ffffffff812a26b7>] ? pipe_fcntl+0x107/0x210
> [ 1691.831827] [<ffffffff812a26b7>] pipe_fcntl+0x107/0x210
> [ 1691.833651] [<ffffffff812b7ea0>] ? fget_raw_light+0x130/0x3f0
> [ 1691.835343] [<ffffffff812aa5fb>] SyS_fcntl+0x60b/0x6a0
> [ 1691.837008] [<ffffffff8403ca98>] tracesys+0xe1/0xe6
>
> The caller specifically sets __GFP_NOWARN presumably to avoid this warning on
> slub but I'm not sure if there's any other reason.
There must be another reason. Lets fix this.
On 06/10/2013 07:40 PM, Christoph Lameter wrote:
> On Mon, 10 Jun 2013, Sasha Levin wrote:
>
>> [ 1691.807621] Call Trace:
>> [ 1691.809473] [<ffffffff83ff4041>] dump_stack+0x4e/0x82
>> [ 1691.812783] [<ffffffff8111fe12>] warn_slowpath_common+0x82/0xb0
>> [ 1691.817011] [<ffffffff8111fe55>] warn_slowpath_null+0x15/0x20
>> [ 1691.819936] [<ffffffff81243dcf>] kmalloc_slab+0x2f/0xb0
>> [ 1691.824942] [<ffffffff81278d54>] __kmalloc+0x24/0x4b0
>> [ 1691.827285] [<ffffffff8196ffe3>] ? security_capable+0x13/0x20
>> [ 1691.829405] [<ffffffff812a26b7>] ? pipe_fcntl+0x107/0x210
>> [ 1691.831827] [<ffffffff812a26b7>] pipe_fcntl+0x107/0x210
>> [ 1691.833651] [<ffffffff812b7ea0>] ? fget_raw_light+0x130/0x3f0
>> [ 1691.835343] [<ffffffff812aa5fb>] SyS_fcntl+0x60b/0x6a0
>> [ 1691.837008] [<ffffffff8403ca98>] tracesys+0xe1/0xe6
>>
>> The caller specifically sets __GFP_NOWARN presumably to avoid this warning on
>> slub but I'm not sure if there's any other reason.
>
> There must be another reason. Lets fix this.
My, I feel silly now.
I was the one who added __GFP_NOFAIL in the first place in
2ccd4f4d ("pipe: fail cleanly when root tries F_SETPIPE_SZ
with big size").
What happens is that root can go ahead and specify any size
it wants to be used as buffer size - and the kernel will
attempt to comply by allocation that buffer. Which fails
if the size is too big.
Either way, even if we do end up doing something different,
shouldn't we prevent slab from spewing a warning if
__GFP_NOWARN is passed?
Thanks,
Sasha
Hi Sasha,
On Tue, Jun 11, 2013 at 3:54 AM, Sasha Levin <[email protected]> wrote:
> On 06/10/2013 07:40 PM, Christoph Lameter wrote:
>>
>> On Mon, 10 Jun 2013, Sasha Levin wrote:
>>
>>> [ 1691.807621] Call Trace:
>>> [ 1691.809473] [<ffffffff83ff4041>] dump_stack+0x4e/0x82
>>> [ 1691.812783] [<ffffffff8111fe12>] warn_slowpath_common+0x82/0xb0
>>> [ 1691.817011] [<ffffffff8111fe55>] warn_slowpath_null+0x15/0x20
>>> [ 1691.819936] [<ffffffff81243dcf>] kmalloc_slab+0x2f/0xb0
>>> [ 1691.824942] [<ffffffff81278d54>] __kmalloc+0x24/0x4b0
>>> [ 1691.827285] [<ffffffff8196ffe3>] ? security_capable+0x13/0x20
>>> [ 1691.829405] [<ffffffff812a26b7>] ? pipe_fcntl+0x107/0x210
>>> [ 1691.831827] [<ffffffff812a26b7>] pipe_fcntl+0x107/0x210
>>> [ 1691.833651] [<ffffffff812b7ea0>] ? fget_raw_light+0x130/0x3f0
>>> [ 1691.835343] [<ffffffff812aa5fb>] SyS_fcntl+0x60b/0x6a0
>>> [ 1691.837008] [<ffffffff8403ca98>] tracesys+0xe1/0xe6
>>>
>>> The caller specifically sets __GFP_NOWARN presumably to avoid this
>>> warning on
>>> slub but I'm not sure if there's any other reason.
>>
>>
>> There must be another reason. Lets fix this.
>
> My, I feel silly now.
>
> I was the one who added __GFP_NOFAIL in the first place in
> 2ccd4f4d ("pipe: fail cleanly when root tries F_SETPIPE_SZ
> with big size").
>
> What happens is that root can go ahead and specify any size
> it wants to be used as buffer size - and the kernel will
> attempt to comply by allocation that buffer. Which fails
> if the size is too big.
>
> Either way, even if we do end up doing something different,
> shouldn't we prevent slab from spewing a warning if
> __GFP_NOWARN is passed?
Yeah, this is the size-from-userspace case I was thinking about. I think
we have two options: either use your patch or drop the WARN_ON
completely.
Christoph, which one do you prefer?
Pekka
On 06/11/2013 02:28 AM, Pekka Enberg wrote:
> Hi Sasha,
>
> On Tue, Jun 11, 2013 at 3:54 AM, Sasha Levin <[email protected]> wrote:
>> On 06/10/2013 07:40 PM, Christoph Lameter wrote:
>>>
>>> On Mon, 10 Jun 2013, Sasha Levin wrote:
>>>
>>>> [ 1691.807621] Call Trace:
>>>> [ 1691.809473] [<ffffffff83ff4041>] dump_stack+0x4e/0x82
>>>> [ 1691.812783] [<ffffffff8111fe12>] warn_slowpath_common+0x82/0xb0
>>>> [ 1691.817011] [<ffffffff8111fe55>] warn_slowpath_null+0x15/0x20
>>>> [ 1691.819936] [<ffffffff81243dcf>] kmalloc_slab+0x2f/0xb0
>>>> [ 1691.824942] [<ffffffff81278d54>] __kmalloc+0x24/0x4b0
>>>> [ 1691.827285] [<ffffffff8196ffe3>] ? security_capable+0x13/0x20
>>>> [ 1691.829405] [<ffffffff812a26b7>] ? pipe_fcntl+0x107/0x210
>>>> [ 1691.831827] [<ffffffff812a26b7>] pipe_fcntl+0x107/0x210
>>>> [ 1691.833651] [<ffffffff812b7ea0>] ? fget_raw_light+0x130/0x3f0
>>>> [ 1691.835343] [<ffffffff812aa5fb>] SyS_fcntl+0x60b/0x6a0
>>>> [ 1691.837008] [<ffffffff8403ca98>] tracesys+0xe1/0xe6
>>>>
>>>> The caller specifically sets __GFP_NOWARN presumably to avoid this
>>>> warning on
>>>> slub but I'm not sure if there's any other reason.
>>>
>>>
>>> There must be another reason. Lets fix this.
>>
>> My, I feel silly now.
>>
>> I was the one who added __GFP_NOFAIL in the first place in
>> 2ccd4f4d ("pipe: fail cleanly when root tries F_SETPIPE_SZ
>> with big size").
>>
>> What happens is that root can go ahead and specify any size
>> it wants to be used as buffer size - and the kernel will
>> attempt to comply by allocation that buffer. Which fails
>> if the size is too big.
>>
>> Either way, even if we do end up doing something different,
>> shouldn't we prevent slab from spewing a warning if
>> __GFP_NOWARN is passed?
>
> Yeah, this is the size-from-userspace case I was thinking about. I think
> we have two options: either use your patch or drop the WARN_ON
> completely.
>
> Christoph, which one do you prefer?
I think that leaving the warning makes sense to catch similar
things which are actually bugs - we had a similar issue with
/dev/kmsg (if I remember correctly) which actually pointed to
a bug.
Thanks,
Sasha
On Tue, 11 Jun 2013, Sasha Levin wrote:
> I think that leaving the warning makes sense to catch similar
> things which are actually bugs - we had a similar issue with
> /dev/kmsg (if I remember correctly) which actually pointed to
> a bug.
Right. Requesting an allocation larger than even supported by the page
allocator from the slab allocators that are specializing in allocations of
small objects is usually an indication of a problem in the code.
On Tue, 11 Jun 2013, Sasha Levin wrote:
>> I think that leaving the warning makes sense to catch similar
>> things which are actually bugs - we had a similar issue with
>> /dev/kmsg (if I remember correctly) which actually pointed to
>> a bug.
On 6/11/13 6:14 PM, Christoph Lameter wrote:
> Right. Requesting an allocation larger than even supported by the page
> allocator from the slab allocators that are specializing in allocations of
> small objects is usually an indication of a problem in the code.
So you're OK with going forward with Sasha's patch? It's needed
because __GFP_NOWARN was specifically added there to fix this
issue earlier.
Pekka
On Mon, 10 Jun 2013, Sasha Levin wrote:
> > There must be another reason. Lets fix this.
>
> My, I feel silly now.
>
> I was the one who added __GFP_NOFAIL in the first place in
> 2ccd4f4d ("pipe: fail cleanly when root tries F_SETPIPE_SZ
> with big size").
>
> What happens is that root can go ahead and specify any size
> it wants to be used as buffer size - and the kernel will
> attempt to comply by allocation that buffer. Which fails
> if the size is too big.
Could you check that against a boundary? Use vmalloc if larger than a
couple of pages? Maybe PAGE_COSTLY_ORDER or so?
THe higher the order the more likely it is that the allocation will fail.
The PAGE_ORDER_COSTLY (or so) is a reasonable limit as to what size of a
linear contiguous allocation that can be expected to be successful.
On Tue, 11 Jun 2013, Pekka Enberg wrote:
> So you're OK with going forward with Sasha's patch? It's needed
> because __GFP_NOWARN was specifically added there to fix this
> issue earlier.
Why dont we fix the call site to use vmalloc instead for larger allocs?
On 06/11/2013 11:23 AM, Christoph Lameter wrote:
> On Tue, 11 Jun 2013, Pekka Enberg wrote:
>
>> So you're OK with going forward with Sasha's patch? It's needed
>> because __GFP_NOWARN was specifically added there to fix this
>> issue earlier.
>
> Why dont we fix the call site to use vmalloc instead for larger allocs?
>
We should probably be doing both.
Thanks,
Sasha
On Tue, 2013-06-11 at 11:44 -0400, Sasha Levin wrote:
> On 06/11/2013 11:23 AM, Christoph Lameter wrote:
> > On Tue, 11 Jun 2013, Pekka Enberg wrote:
> >
> >> So you're OK with going forward with Sasha's patch? It's needed
> >> because __GFP_NOWARN was specifically added there to fix this
> >> issue earlier.
> >
> > Why dont we fix the call site to use vmalloc instead for larger allocs?
> >
>
> We should probably be doing both.
Allowing a pipe to store thousands of page refs seems quite useless and
dangerous.
Having to use vmalloc()/vfree() for every splice()/vmsplice() would be a
performance loss anyway.
(fs/splice.c splice_grow_spd() will also want to allocate big kmalloc()
chunks)
On 06/11/2013 12:13 PM, Eric Dumazet wrote:
> On Tue, 2013-06-11 at 11:44 -0400, Sasha Levin wrote:
>> On 06/11/2013 11:23 AM, Christoph Lameter wrote:
>>> On Tue, 11 Jun 2013, Pekka Enberg wrote:
>>>
>>>> So you're OK with going forward with Sasha's patch? It's needed
>>>> because __GFP_NOWARN was specifically added there to fix this
>>>> issue earlier.
>>>
>>> Why dont we fix the call site to use vmalloc instead for larger allocs?
>>>
>>
>> We should probably be doing both.
>
> Allowing a pipe to store thousands of page refs seems quite useless and
> dangerous.
It might be, but you need CAP_SYS_RESOURCE to go into the dangerous
zone (>pipe_max_size).
So if root (or someone with that cap) wants to go there, as Rusty says:
"Root asked, we do."
Thanks,
Sasha
On Tue, 11 Jun 2013, Eric Dumazet wrote:
> Allowing a pipe to store thousands of page refs seems quite useless and
> dangerous.
>
> Having to use vmalloc()/vfree() for every splice()/vmsplice() would be a
> performance loss anyway.
>
> (fs/splice.c splice_grow_spd() will also want to allocate big kmalloc()
> chunks)
Why is it not using the page allocator for large allocations?
On Tue, 2013-06-11 at 12:19 -0400, Sasha Levin wrote:
> It might be, but you need CAP_SYS_RESOURCE to go into the dangerous
> zone (>pipe_max_size).
>
> So if root (or someone with that cap) wants to go there, as Rusty says:
> "Root asked, we do."
Yes and no : adding a test to select vmalloc()/vfree() instead of
kmalloc()/kfree() will slow down regular users asking 32 pages in their
pipe.
If there is no _sensible_ use for large pipes even for root, please do
not bloat the code just because we can.
On Tue, Jun 11, 2013 at 09:37:35AM -0700, Eric Dumazet wrote:
> On Tue, 2013-06-11 at 12:19 -0400, Sasha Levin wrote:
>
> > It might be, but you need CAP_SYS_RESOURCE to go into the dangerous
> > zone (>pipe_max_size).
> >
> > So if root (or someone with that cap) wants to go there, as Rusty says:
> > "Root asked, we do."
>
> Yes and no : adding a test to select vmalloc()/vfree() instead of
> kmalloc()/kfree() will slow down regular users asking 32 pages in their
> pipe.
>
> If there is no _sensible_ use for large pipes even for root, please do
> not bloat the code just because we can.
It's not even that this is the only place that this happens.
I've reported similar traces from the ieee802.154 code for some time.
I wouldn't be surprised to find that there are other similar cases where
a user can ask the kernel to do some incredibly huge allocation.
For my fuzz testing runs, I ended up with the patch below to stop the page allocator warnings.
Dave
--- /home/davej/src/kernel/git-trees/linux/net/ieee802154/af_ieee802154.c 2013-01-04 18:57:17.677270225 -0500
+++ linux-dj/net/ieee802154/af_ieee802154.c 2013-05-06 20:34:30.702926471 -0400
@@ -108,6 +108,12 @@ static int ieee802154_sock_sendmsg(struc
{
struct sock *sk = sock->sk;
+ if (len > MAX_ORDER_NR_PAGES * PAGE_SIZE) {
+ printk_once("Massive alloc in %s!: %d > %d\n", __func__,
+ (unsigned int) len, (unsigned int) (MAX_ORDER_NR_PAGES * PAGE_SIZE));
+ return -EINVAL;
+ }
+
return sk->sk_prot->sendmsg(iocb, sk, msg, len);
}
On 06/11/2013 12:37 PM, Eric Dumazet wrote:
> On Tue, 2013-06-11 at 12:19 -0400, Sasha Levin wrote:
>
>> It might be, but you need CAP_SYS_RESOURCE to go into the dangerous
>> zone (>pipe_max_size).
>>
>> So if root (or someone with that cap) wants to go there, as Rusty says:
>> "Root asked, we do."
>
> Yes and no : adding a test to select vmalloc()/vfree() instead of
> kmalloc()/kfree() will slow down regular users asking 32 pages in their
> pipe.
>
> If there is no _sensible_ use for large pipes even for root, please do
> not bloat the code just because we can.
The code to allow root to grow pipes is quite ancient.
Either we drop it or we fix it, leaving it broken as it is is silly.
Thanks,
Sasha
On Tue, 11 Jun 2013, Christoph Lameter wrote:
> > I think that leaving the warning makes sense to catch similar
> > things which are actually bugs - we had a similar issue with
> > /dev/kmsg (if I remember correctly) which actually pointed to
> > a bug.
>
> Right. Requesting an allocation larger than even supported by the page
> allocator from the slab allocators that are specializing in allocations of
> small objects is usually an indication of a problem in the code.
>
I think we can remove the kmalloc_slab() warning that Sasha is pointing to
and just fallback to the page allocator for sizes that are too large?
Then the page allocator can return NULL and warn, if necessary, for orders
larger than MAX_ORDER.
On Tue, 11 Jun 2013 18:16:08 +0300 Pekka Enberg <[email protected]> wrote:
> On Tue, 11 Jun 2013, Sasha Levin wrote:
> >> I think that leaving the warning makes sense to catch similar
> >> things which are actually bugs - we had a similar issue with
> >> /dev/kmsg (if I remember correctly) which actually pointed to
> >> a bug.
>
> On 6/11/13 6:14 PM, Christoph Lameter wrote:
> > Right. Requesting an allocation larger than even supported by the page
> > allocator from the slab allocators that are specializing in allocations of
> > small objects is usually an indication of a problem in the code.
>
> So you're OK with going forward with Sasha's patch?
Yes please. slab should honour __GFP_NOWARN.
__GFP_NOWARN is frequently used by kernel code to probe for "how big an
allocation can I get". That's a bit lame, but it's used on slow paths
and is pretty simple.
In the case of pipe_set_size(), it's userspace who is doing the
probing: an application can request a huge pipe buffer and if that
fails, try again with a smaller one. It's just wrong to emit a kernel
warning in this case. Plus, we've already reported the failure
anyway, by returning -ENOMEM from pipe_fcntl().
On Tue, 11 Jun 2013, Eric Dumazet wrote:
> Yes and no : adding a test to select vmalloc()/vfree() instead of
> kmalloc()/kfree() will slow down regular users asking 32 pages in their
> pipe.
But vmalloc would allow buffers larger than MAX_PAGE_ORDER. The allocation
would not fail. You could have 1G or so pipes if necessary.
> If there is no _sensible_ use for large pipes even for root, please do
> not bloat the code just because we can.
Some code bloat to enable super sized pipes may be ok?
Huge page support could be useful to increase speed?
On Wed, Jun 12, 2013 at 1:34 AM, Andrew Morton
<[email protected]> wrote:
> __GFP_NOWARN is frequently used by kernel code to probe for "how big an
> allocation can I get". That's a bit lame, but it's used on slow paths
> and is pretty simple.
Applied to slab/urgent, thanks guys!