[PATCH] kmemleak: Reduce verbosity when memory allocation fails

[PATCH] kmemleak: Reduce verbosity when memory allocation fails

[Catalin Marinas]

This patch changes the kmemleak verbosity to avoid dumping the stack
trace when memory allocation for the kmemleak internal metadata fails.
It also passes __GFP_NOWARN to the slab allocator.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: Toralf Förster <toralf.foerster@gmx.de>
Cc: Pekka Enberg <penberg@cs.helsinki.fi>
Cc: David Rientjes <rientjes@google.com>
Cc: Ted Ts'o <tytso@mit.edu>
---
 mm/kmemleak.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index bd9bc21..eee8e31 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -113,7 +113,8 @@
 #define BYTES_PER_POINTER	sizeof(void *)
 
 /* GFP bitmask for kmemleak internal allocations */
-#define GFP_KMEMLEAK_MASK	(GFP_KERNEL | GFP_ATOMIC)
+#define gfp_kmemleak_mask(gfp)	((gfp) & (GFP_KERNEL | GFP_ATOMIC) | \
+				 __GFP_NOWARN)
 
 /* scanning area inside a memory block */
 struct kmemleak_scan_area {
@@ -511,9 +512,10 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
 	struct kmemleak_object *object;
 	struct prio_tree_node *node;
 
-	object = kmem_cache_alloc(object_cache, gfp & GFP_KMEMLEAK_MASK);
+	object = kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));
 	if (!object) {
-		kmemleak_stop("Cannot allocate a kmemleak_object structure\n");
+		pr_warning("Cannot allocate a kmemleak_object structure\n");
+		kmemleak_disable();
 		return NULL;
 	}
 
@@ -734,9 +736,9 @@ static void add_scan_area(unsigned long ptr, size_t size, gfp_t gfp)
 		return;
 	}
 
-	area = kmem_cache_alloc(scan_area_cache, gfp & GFP_KMEMLEAK_MASK);
+	area = kmem_cache_alloc(scan_area_cache, gfp_kmemleak_mask(gfp));
 	if (!area) {
-		kmemleak_warn("Cannot allocate a scan area\n");
+		pr_warning("Cannot allocate a scan area\n");
 		goto out;
 	}
 

Re: [PATCH] kmemleak: Reduce verbosity when memory allocation fails

[David Rientjes]

On Mon, 10 Jan 2011, Catalin Marinas wrote:

> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
> index bd9bc21..eee8e31 100644
> --- a/mm/kmemleak.c
> +++ b/mm/kmemleak.c
> @@ -113,7 +113,8 @@
>  #define BYTES_PER_POINTER	sizeof(void *)
>  
>  /* GFP bitmask for kmemleak internal allocations */
> -#define GFP_KMEMLEAK_MASK	(GFP_KERNEL | GFP_ATOMIC)
> +#define gfp_kmemleak_mask(gfp)	((gfp) & (GFP_KERNEL | GFP_ATOMIC) | \
> +				 __GFP_NOWARN)

It would be a shame if the allocation were __GFP_NORETRY and kmemleak 
ended up looping forever because it suppresses the bit for a single page, 
it uses __GFP_NOMEMALLOC and kmemleak ends up allocating from memory 
reserves, or it uses __GFP_HARDWALL and kmemleak is allocating metadata in 
a different cpuset.

I'm not sure why you're not just masking __GFP_NOFAIL and __GFP_REPEAT and 
then failing gracefully?  (And __GFP_ZERO and __GFP_COMP, too, of course.)

Re: [PATCH] kmemleak: Reduce verbosity when memory allocation fails

[Catalin Marinas]

On Monday, 10 January 2011, David Rientjes <rientjes@google.com> wrote:
> On Mon, 10 Jan 2011, Catalin Marinas wrote:
>
>> diff --git a/mm/kmemleak.c b/mm/kmemleak.c
>> index bd9bc21..eee8e31 100644
>> --- a/mm/kmemleak.c
>> +++ b/mm/kmemleak.c
>> @@ -113,7 +113,8 @@
>>  #define BYTES_PER_POINTER    sizeof(void *)
>>
>>  /* GFP bitmask for kmemleak internal allocations */
>> -#define GFP_KMEMLEAK_MASK    (GFP_KERNEL | GFP_ATOMIC)
>> +#define gfp_kmemleak_mask(gfp)       ((gfp) & (GFP_KERNEL | GFP_ATOMIC) | \
>> +                              __GFP_NOWARN)
>
> It would be a shame if the allocation were __GFP_NORETRY and kmemleak
> ended up looping forever because it suppresses the bit for a single page,
> it uses __GFP_NOMEMALLOC and kmemleak ends up allocating from memory
> reserves, or it uses __GFP_HARDWALL and kmemleak is allocating metadata in
> a different cpuset.
>
> I'm not sure why you're not just masking __GFP_NOFAIL and __GFP_REPEAT and
> then failing gracefully?  (And __GFP_ZERO and __GFP_COMP, too, of course.)

The reason I wouldn't allow kmemleak allocations to fail is that once
it happened kmemleak cannot recover. Once it missed an allocation for
its metadata, the corresponding memory block cannot be tracked nor
scanned leading to false positives. I had thought about even passing
__GFP_REPEAT but I think this is kind of implied for 0-order
allocations.

If the user calling the kernel alloc function cannot get memory,
kmemleak won't be called anyway.

-- 
Catalin

Re: [PATCH] kmemleak: Reduce verbosity when memory allocation fails

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1885 bytes --]

On Mon, 10 Jan 2011, Catalin Marinas wrote:

> > It would be a shame if the allocation were __GFP_NORETRY and kmemleak
> > ended up looping forever because it suppresses the bit for a single page,
> > it uses __GFP_NOMEMALLOC and kmemleak ends up allocating from memory
> > reserves, or it uses __GFP_HARDWALL and kmemleak is allocating metadata in
> > a different cpuset.
> >
> > I'm not sure why you're not just masking __GFP_NOFAIL and __GFP_REPEAT and
> > then failing gracefully?  (And __GFP_ZERO and __GFP_COMP, too, of course.)
> 
> The reason I wouldn't allow kmemleak allocations to fail is that once
> it happened kmemleak cannot recover. Once it missed an allocation for
> its metadata, the corresponding memory block cannot be tracked nor
> scanned leading to false positives. I had thought about even passing
> __GFP_REPEAT but I think this is kind of implied for 0-order
> allocations.
> 

Yes, __GFP_REPEAT is a no-op for order-0 allocations for the current 
implementation.

> If the user calling the kernel alloc function cannot get memory,
> kmemleak won't be called anyway.
> 

I'm talking about when the allocation is successful and the metadata 
allocation is not, such as what Toralf reported.  If you pass 
__GFP_NOFAIL, it's going to loop forever which is certainly not what we'd 
want: we'd rather just disable kmemleak and continue doing work.  If you 
pass __GFP_NOMEMALLOC, then kmemleak can allocate from memory reserves in 
the reclaim path which is not allowed.  And if you don't pass 
__GFP_HARDWALL, then these allocations can break memory isolation by 
allowing the metadata to be allocated from different cpusets.

In other words, I'm pretty sure you don't want to be masking these options 
off of the caller allocation when passed.  It makes no sense for the user 
to do a GFP_KERNEL | __GFP_NORETRY allocation and then have kmemleak loop 
forever.

Re: [PATCH] kmemleak: Reduce verbosity when memory allocation fails

[Catalin Marinas]

On Mon, 2011-01-10 at 23:31 +0000, David Rientjes wrote:
> On Mon, 10 Jan 2011, Catalin Marinas wrote:
> > > It would be a shame if the allocation were __GFP_NORETRY and kmemleak
> > > ended up looping forever because it suppresses the bit for a single page,
> > > it uses __GFP_NOMEMALLOC and kmemleak ends up allocating from memory
> > > reserves, or it uses __GFP_HARDWALL and kmemleak is allocating metadata in
> > > a different cpuset.
> > >
> > > I'm not sure why you're not just masking __GFP_NOFAIL and __GFP_REPEAT and
> > > then failing gracefully?  (And __GFP_ZERO and __GFP_COMP, too, of course.)
[...]
> > If the user calling the kernel alloc function cannot get memory,
> > kmemleak won't be called anyway.
> 
> I'm talking about when the allocation is successful and the metadata
> allocation is not, such as what Toralf reported.  If you pass
> __GFP_NOFAIL, it's going to loop forever which is certainly not what we'd
> want: we'd rather just disable kmemleak and continue doing work.  

I agree with this but kmemleak doesn't pass any __GFP_NOFAIL flag (it's
masked out). The reason it only keeps (GFP_KERNEL | GFP_ATOMIC) from the
caller flags is to know whether the kmemleak metadata allocation must be
atomic or not. All the other flags a user may pass like __GFP_NOFAIL are
masked out.

Of course, kmemleak can pass additional flags for allocating its
metadata, but I see this as unrelated to what the user passed (as long
as the atomicity is preserved).

Now, if the user passes __GFP_NOFAIL, do we want such flag for the
kmemleak allocation? IMHO we don't need this (hence it is masked out).
We just allow the kmemleak allocation to fail.

> If you
> pass __GFP_NOMEMALLOC, then kmemleak can allocate from memory reserves in
> the reclaim path which is not allowed.  And if you don't pass
> __GFP_HARDWALL, then these allocations can break memory isolation by
> allowing the metadata to be allocated from different cpusets.

As I said above, we can pass additional flags like __GFP_NOMEMALLOC. But
I think these should be hard-coded for kmemleak allocations irrespective
of the k*alloc user gfp flags.

As for the __GFP_HARDWALL, I don't think it matters much. Kmemleak
doesn't have per-CPU data anyway and the tree for storing the metada is
a global one. When scanning the memory, it does it on a single CPU no
matter where the object was allocated from.
> 
> In other words, I'm pretty sure you don't want to be masking these options
> off of the caller allocation when passed.  It makes no sense for the user
> to do a GFP_KERNEL | __GFP_NORETRY allocation and then have kmemleak loop
> forever.

__GFP_NORETRY is another flag we could force on kmemleak metadata
allocations. See below:


kmemleak: Allow kmemleak metadata allocations to fail

From: Catalin Marinas <catalin.marinas@arm.com>

This patch adds __GFP_NORETRY and __GFP_NOMEMALLOC flags to the kmemleak
metadata allocations so that it has a smaller effect on the users of the
kernel slab allocator. Since kmemleak allocations can now fail more
often, this patch also reduces the verbosity by passing __GFP_NOWARN and
not dumping the stack trace when a kmemleak allocation fails.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Reported-by: Toralf Förster <toralf.foerster@gmx.de>
Cc: Pekka Enberg <penberg@cs.helsinki.fi>
Cc: David Rientjes <rientjes@google.com>
Cc: Ted Ts'o <tytso@mit.edu>
---
 mm/kmemleak.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/mm/kmemleak.c b/mm/kmemleak.c
index bd9bc21..84225f3 100644
--- a/mm/kmemleak.c
+++ b/mm/kmemleak.c
@@ -113,7 +113,9 @@
 #define BYTES_PER_POINTER	sizeof(void *)
 
 /* GFP bitmask for kmemleak internal allocations */
-#define GFP_KMEMLEAK_MASK	(GFP_KERNEL | GFP_ATOMIC)
+#define gfp_kmemleak_mask(gfp)	(((gfp) & (GFP_KERNEL | GFP_ATOMIC)) | \
+				 __GFP_NORETRY | __GFP_NOMEMALLOC | \
+				 __GFP_NOWARN)
 
 /* scanning area inside a memory block */
 struct kmemleak_scan_area {
@@ -511,9 +513,10 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
 	struct kmemleak_object *object;
 	struct prio_tree_node *node;
 
-	object = kmem_cache_alloc(object_cache, gfp & GFP_KMEMLEAK_MASK);
+	object = kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));
 	if (!object) {
-		kmemleak_stop("Cannot allocate a kmemleak_object structure\n");
+		pr_warning("Cannot allocate a kmemleak_object structure\n");
+		kmemleak_disable();
 		return NULL;
 	}
 
@@ -734,9 +737,9 @@ static void add_scan_area(unsigned long ptr, size_t size, gfp_t gfp)
 		return;
 	}
 
-	area = kmem_cache_alloc(scan_area_cache, gfp & GFP_KMEMLEAK_MASK);
+	area = kmem_cache_alloc(scan_area_cache, gfp_kmemleak_mask(gfp));
 	if (!area) {
-		kmemleak_warn("Cannot allocate a scan area\n");
+		pr_warning("Cannot allocate a scan area\n");
 		goto out;
 	}
 


-- 
Catalin

Re: [PATCH] kmemleak: Reduce verbosity when memory allocation fails

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 799 bytes --]

On Tue, 11 Jan 2011, Catalin Marinas wrote:

> kmemleak: Allow kmemleak metadata allocations to fail
> 
> From: Catalin Marinas <catalin.marinas@arm.com>
> 
> This patch adds __GFP_NORETRY and __GFP_NOMEMALLOC flags to the kmemleak
> metadata allocations so that it has a smaller effect on the users of the
> kernel slab allocator. Since kmemleak allocations can now fail more
> often, this patch also reduces the verbosity by passing __GFP_NOWARN and
> not dumping the stack trace when a kmemleak allocation fails.
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Reported-by: Toralf Förster <toralf.foerster@gmx.de>
> Cc: Pekka Enberg <penberg@cs.helsinki.fi>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Ted Ts'o <tytso@mit.edu>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH] kmemleak: Reduce verbosity when memory allocation fails

[Pekka Enberg]

On 1/11/11 11:34 PM, David Rientjes wrote:
> On Tue, 11 Jan 2011, Catalin Marinas wrote:
>
>> kmemleak: Allow kmemleak metadata allocations to fail
>>
>> From: Catalin Marinas<catalin.marinas@arm.com>
>>
>> This patch adds __GFP_NORETRY and __GFP_NOMEMALLOC flags to the kmemleak
>> metadata allocations so that it has a smaller effect on the users of the
>> kernel slab allocator. Since kmemleak allocations can now fail more
>> often, this patch also reduces the verbosity by passing __GFP_NOWARN and
>> not dumping the stack trace when a kmemleak allocation fails.
>>
>> Signed-off-by: Catalin Marinas<catalin.marinas@arm.com>
>> Reported-by: Toralf Förster<toralf.foerster@gmx.de>
>> Cc: Pekka Enberg<penberg@cs.helsinki.fi>
>> Cc: David Rientjes<rientjes@google.com>
>> Cc: Ted Ts'o<tytso@mit.edu>
>
> Acked-by: David Rientjes<rientjes@google.com>

Acked-by: Pekka Enberg <penberg@kernel.org>