[PATCH] Add kmemleak annotations to lmb.c

[PATCH] Add kmemleak annotations to lmb.c

[Michael Ellerman]

We don't actually want kmemleak to track the lmb allocations, so we
pass min_count as 0. However telling kmemleak about lmb allocations
allows it to scan that memory for pointers to other memory that is
tracked by kmemleak, ie. slab allocations etc.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 lib/lmb.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index e4a6482..b82779a 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -14,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/bitops.h>
 #include <linux/lmb.h>
+#include <linux/kmemleak.h>
 
 #define LMB_ALLOC_ANYWHERE	0
 
@@ -352,8 +353,10 @@ u64 __init lmb_alloc_nid(u64 size, u64 align, int nid,
 		u64 ret = lmb_alloc_nid_region(&mem->region[i],
 					       nid_range,
 					       size, align, nid);
-		if (ret != ~(u64)0)
+		if (ret != ~(u64)0) {
+			kmemleak_alloc(__va(ret), size, 0, 0);
 			return ret;
+		}
 	}
 
 	return lmb_alloc(size, align);
@@ -412,6 +415,8 @@ u64 __init __lmb_alloc_base(u64 size, u64 align, u64 max_addr)
 				/* this area isn't reserved, take it */
 				if (lmb_add_region(&lmb.reserved, base, size) < 0)
 					return 0;
+
+				kmemleak_alloc(__va(base), size, 0, 0);
 				return base;
 			}
 			res_base = lmb.reserved.region[j].base;
-- 
1.6.2.1

Re: [PATCH] Add kmemleak annotations to lmb.c

[Catalin Marinas]

On Thu, 2009-08-13 at 13:01 +1000, Michael Ellerman wrote:
> We don't actually want kmemleak to track the lmb allocations, so we
> pass min_count as 0. However telling kmemleak about lmb allocations
> allows it to scan that memory for pointers to other memory that is
> tracked by kmemleak, ie. slab allocations etc.

Looks alright to me (though I haven't tested it). You can add a
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

-- 
Catalin

Re: [PATCH] Add kmemleak annotations to lmb.c

[Benjamin Herrenschmidt]

On Thu, 2009-08-13 at 16:40 +0100, Catalin Marinas wrote:
> On Thu, 2009-08-13 at 13:01 +1000, Michael Ellerman wrote:
> > We don't actually want kmemleak to track the lmb allocations, so we
> > pass min_count as 0. However telling kmemleak about lmb allocations
> > allows it to scan that memory for pointers to other memory that is
> > tracked by kmemleak, ie. slab allocations etc.
> 
> Looks alright to me (though I haven't tested it). You can add a
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Actually, Milton pointed to me that we may not want to allow all
LMB chunks to be scanned by kmemleaks, things like the DART hole
that's taken out of the linear mapping for example may need to
be avoided, though I'm not sure what would be the right way to
do it.

Cheers,
Ben.

Re: [PATCH] Add kmemleak annotations to lmb.c

[Catalin Marinas]

On Fri, 2009-08-14 at 17:56 +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2009-08-13 at 16:40 +0100, Catalin Marinas wrote:
> > On Thu, 2009-08-13 at 13:01 +1000, Michael Ellerman wrote:
> > > We don't actually want kmemleak to track the lmb allocations, so we
> > > pass min_count as 0. However telling kmemleak about lmb allocations
> > > allows it to scan that memory for pointers to other memory that is
> > > tracked by kmemleak, ie. slab allocations etc.
> > 
> > Looks alright to me (though I haven't tested it). You can add a
> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> Actually, Milton pointed to me that we may not want to allow all
> LMB chunks to be scanned by kmemleaks, things like the DART hole
> that's taken out of the linear mapping for example may need to
> be avoided, though I'm not sure what would be the right way to
> do it.

I suspect there are more blocks to be scanned than those that shouldn't,
so maybe ignore the latter explicitly using kmemleak_ignore(). This was
raised recently on x86_64 as well which has a memory hole for some
aperture - http://lkml.org/lkml/2009/8/13/237.

-- 
Catalin

Re: [PATCH] Add kmemleak annotations to lmb.c

[David Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Fri, 14 Aug 2009 17:56:40 +1000

> On Thu, 2009-08-13 at 16:40 +0100, Catalin Marinas wrote:
>> On Thu, 2009-08-13 at 13:01 +1000, Michael Ellerman wrote:
>> > We don't actually want kmemleak to track the lmb allocations, so we
>> > pass min_count as 0. However telling kmemleak about lmb allocations
>> > allows it to scan that memory for pointers to other memory that is
>> > tracked by kmemleak, ie. slab allocations etc.
>> 
>> Looks alright to me (though I haven't tested it). You can add a
>> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> 
> Actually, Milton pointed to me that we may not want to allow all
> LMB chunks to be scanned by kmemleaks, things like the DART hole
> that's taken out of the linear mapping for example may need to
> be avoided, though I'm not sure what would be the right way to
> do it.

I think that annotating LMB for kmemleak may be more problems
that it's worth.

I can't think of any specific problems like the DART thing on
sparc64, but I'm sure that as soon as someone starts trying
to test this they'll run into one thing or another :-)

Re: [PATCH] Add kmemleak annotations to lmb.c

[Catalin Marinas]

On Fri, 2009-08-14 at 12:49 -0700, David Miller wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Fri, 14 Aug 2009 17:56:40 +1000
> 
> > On Thu, 2009-08-13 at 16:40 +0100, Catalin Marinas wrote:
> >> On Thu, 2009-08-13 at 13:01 +1000, Michael Ellerman wrote:
> >> > We don't actually want kmemleak to track the lmb allocations, so we
> >> > pass min_count as 0. However telling kmemleak about lmb allocations
> >> > allows it to scan that memory for pointers to other memory that is
> >> > tracked by kmemleak, ie. slab allocations etc.
> >> 
> >> Looks alright to me (though I haven't tested it). You can add a
> >> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> > 
> > Actually, Milton pointed to me that we may not want to allow all
> > LMB chunks to be scanned by kmemleaks, things like the DART hole
> > that's taken out of the linear mapping for example may need to
> > be avoided, though I'm not sure what would be the right way to
> > do it.
> 
> I think that annotating LMB for kmemleak may be more problems
> that it's worth.

BTW, are there many LMB allocations used for storing pointers to other
objects? If not, it may be worth just annotating those with
kmemleak_alloc() if you get false positives.

-- 
Catalin

Re: [PATCH] Add kmemleak annotations to lmb.c

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]

On Fri, 2009-08-14 at 22:57 +0100, Catalin Marinas wrote:
> On Fri, 2009-08-14 at 12:49 -0700, David Miller wrote:
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Date: Fri, 14 Aug 2009 17:56:40 +1000
> > 
> > > On Thu, 2009-08-13 at 16:40 +0100, Catalin Marinas wrote:
> > >> On Thu, 2009-08-13 at 13:01 +1000, Michael Ellerman wrote:
> > >> > We don't actually want kmemleak to track the lmb allocations, so we
> > >> > pass min_count as 0. However telling kmemleak about lmb allocations
> > >> > allows it to scan that memory for pointers to other memory that is
> > >> > tracked by kmemleak, ie. slab allocations etc.
> > >> 
> > >> Looks alright to me (though I haven't tested it). You can add a
> > >> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> > > 
> > > Actually, Milton pointed to me that we may not want to allow all
> > > LMB chunks to be scanned by kmemleaks, things like the DART hole
> > > that's taken out of the linear mapping for example may need to
> > > be avoided, though I'm not sure what would be the right way to
> > > do it.
> > 
> > I think that annotating LMB for kmemleak may be more problems
> > that it's worth.
> 
> BTW, are there many LMB allocations used for storing pointers to other
> objects? If not, it may be worth just annotating those with
> kmemleak_alloc() if you get false positives.

Yeah I think that's probably the safer approach. As Dave says even if
there's nothing obvious, lmb is used for very early allocs which are
more likely to be "special" and cause problems - and only when someone
boots with kmemleak enabled. So we're better to explicitly mark things
we want scanned.

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] Add kmemleak annotations to lmb.c

[Michael Ellerman]

We don't actually want kmemleak to track the lmb allocations, so we
pass min_count as 0. However telling kmemleak about lmb allocations
allows it to scan that memory for pointers to other memory that is
tracked by kmemleak, ie. slab allocations etc.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---
 lib/lmb.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index e4a6482..dc10bc5 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -352,8 +352,10 @@ u64 __init lmb_alloc_nid(u64 size, u64 align, int nid,
 		u64 ret = lmb_alloc_nid_region(&mem->region[i],
 					       nid_range,
 					       size, align, nid);
-		if (ret != ~(u64)0)
+		if (ret != ~(u64)0) {
+			kmemleak_alloc(__va(ret), size, 0, 0);
 			return ret;
+		}
 	}
 
 	return lmb_alloc(size, align);
@@ -412,6 +414,8 @@ u64 __init __lmb_alloc_base(u64 size, u64 align, u64 max_addr)
 				/* this area isn't reserved, take it */
 				if (lmb_add_region(&lmb.reserved, base, size) < 0)
 					return 0;
+
+				kmemleak_alloc(__va(base), size, 0, 0);
 				return base;
 			}
 			res_base = lmb.reserved.region[j].base;
-- 
1.6.2.1

Re: [PATCH] Add kmemleak annotations to lmb.c

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]

On Mon, 2009-08-10 at 15:05 +1000, Michael Ellerman wrote:
> We don't actually want kmemleak to track the lmb allocations, so we
> pass min_count as 0. However telling kmemleak about lmb allocations
> allows it to scan that memory for pointers to other memory that is
> tracked by kmemleak, ie. slab allocations etc.
> 
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> ---
>  lib/lmb.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/lmb.c b/lib/lmb.c
> index e4a6482..dc10bc5 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -352,8 +352,10 @@ u64 __init lmb_alloc_nid(u64 size, u64 align, int nid,
>  		u64 ret = lmb_alloc_nid_region(&mem->region[i],
>  					       nid_range,
>  					       size, align, nid);
> -		if (ret != ~(u64)0)
> +		if (ret != ~(u64)0) {
> +			kmemleak_alloc(__va(ret), size, 0, 0);
>  			return ret;
> +		}
>  	}
>  
>  	return lmb_alloc(size, align);
> @@ -412,6 +414,8 @@ u64 __init __lmb_alloc_base(u64 size, u64 align, u64 max_addr)
>  				/* this area isn't reserved, take it */
>  				if (lmb_add_region(&lmb.reserved, base, size) < 0)
>  					return 0;
> +
> +				kmemleak_alloc(__va(base), size, 0, 0);
>  				return base;
>  			}
>  			res_base = lmb.reserved.region[j].base;

This needs an include of kmemleak.h for some configs, new patch coming.

cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]