lib/generic-radix-tree.c: add kmemleak annotations
diff mbox series

Message ID 20191004065039.727564-1-ebiggers@kernel.org
State New
Headers show
Series
  • lib/generic-radix-tree.c: add kmemleak annotations
Related show

Commit Message

Eric Biggers Oct. 4, 2019, 6:50 a.m. UTC
From: Eric Biggers <ebiggers@google.com>

Kmemleak is falsely reporting a leak of the slab allocation in
sctp_stream_init_ext():

BUG: memory leak
unreferenced object 0xffff8881114f5d80 (size 96):
   comm "syz-executor934", pid 7160, jiffies 4294993058 (age 31.950s)
   hex dump (first 32 bytes):
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
   backtrace:
     [<00000000ce7a1326>] kmemleak_alloc_recursive  include/linux/kmemleak.h:55 [inline]
     [<00000000ce7a1326>] slab_post_alloc_hook mm/slab.h:439 [inline]
     [<00000000ce7a1326>] slab_alloc mm/slab.c:3326 [inline]
     [<00000000ce7a1326>] kmem_cache_alloc_trace+0x13d/0x280 mm/slab.c:3553
     [<000000007abb7ac9>] kmalloc include/linux/slab.h:547 [inline]
     [<000000007abb7ac9>] kzalloc include/linux/slab.h:742 [inline]
     [<000000007abb7ac9>] sctp_stream_init_ext+0x2b/0xa0  net/sctp/stream.c:157
     [<0000000048ecb9c1>] sctp_sendmsg_to_asoc+0x946/0xa00  net/sctp/socket.c:1882
     [<000000004483ca2b>] sctp_sendmsg+0x2a8/0x990 net/sctp/socket.c:2102
     [...]

But it's freed later.  Kmemleak misses the allocation because its
pointer is stored in the generic radix tree sctp_stream::out, and the
generic radix tree uses raw pages which aren't tracked by kmemleak.

Fix this by adding the kmemleak hooks to the generic radix tree code.

Reported-by: syzbot+7f3b6b106be8dcdcdeec@syzkaller.appspotmail.com
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 lib/generic-radix-tree.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

Comments

Marcelo Ricardo Leitner Oct. 4, 2019, 12:21 p.m. UTC | #1
On Thu, Oct 03, 2019 at 11:50:39PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Kmemleak is falsely reporting a leak of the slab allocation in
> sctp_stream_init_ext():
> 
> BUG: memory leak
> unreferenced object 0xffff8881114f5d80 (size 96):
>    comm "syz-executor934", pid 7160, jiffies 4294993058 (age 31.950s)
>    hex dump (first 32 bytes):
>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>    backtrace:
>      [<00000000ce7a1326>] kmemleak_alloc_recursive  include/linux/kmemleak.h:55 [inline]
>      [<00000000ce7a1326>] slab_post_alloc_hook mm/slab.h:439 [inline]
>      [<00000000ce7a1326>] slab_alloc mm/slab.c:3326 [inline]
>      [<00000000ce7a1326>] kmem_cache_alloc_trace+0x13d/0x280 mm/slab.c:3553
>      [<000000007abb7ac9>] kmalloc include/linux/slab.h:547 [inline]
>      [<000000007abb7ac9>] kzalloc include/linux/slab.h:742 [inline]
>      [<000000007abb7ac9>] sctp_stream_init_ext+0x2b/0xa0  net/sctp/stream.c:157
>      [<0000000048ecb9c1>] sctp_sendmsg_to_asoc+0x946/0xa00  net/sctp/socket.c:1882
>      [<000000004483ca2b>] sctp_sendmsg+0x2a8/0x990 net/sctp/socket.c:2102
>      [...]
> 
> But it's freed later.  Kmemleak misses the allocation because its
> pointer is stored in the generic radix tree sctp_stream::out, and the
> generic radix tree uses raw pages which aren't tracked by kmemleak.
> 
> Fix this by adding the kmemleak hooks to the generic radix tree code.

Nice, thanks Eric.

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> 
> Reported-by: syzbot+7f3b6b106be8dcdcdeec@syzkaller.appspotmail.com
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  lib/generic-radix-tree.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/generic-radix-tree.c b/lib/generic-radix-tree.c
> index ae25e2fa2187..f25eb111c051 100644
> --- a/lib/generic-radix-tree.c
> +++ b/lib/generic-radix-tree.c
> @@ -2,6 +2,7 @@
>  #include <linux/export.h>
>  #include <linux/generic-radix-tree.h>
>  #include <linux/gfp.h>
> +#include <linux/kmemleak.h>
>  
>  #define GENRADIX_ARY		(PAGE_SIZE / sizeof(struct genradix_node *))
>  #define GENRADIX_ARY_SHIFT	ilog2(GENRADIX_ARY)
> @@ -75,6 +76,27 @@ void *__genradix_ptr(struct __genradix *radix, size_t offset)
>  }
>  EXPORT_SYMBOL(__genradix_ptr);
>  
> +static inline struct genradix_node *genradix_alloc_node(gfp_t gfp_mask)
> +{
> +	struct genradix_node *node;
> +
> +	node = (struct genradix_node *)__get_free_page(gfp_mask|__GFP_ZERO);
> +
> +	/*
> +	 * We're using pages (not slab allocations) directly for kernel data
> +	 * structures, so we need to explicitly inform kmemleak of them in order
> +	 * to avoid false positive memory leak reports.
> +	 */
> +	kmemleak_alloc(node, PAGE_SIZE, 1, gfp_mask);
> +	return node;
> +}
> +
> +static inline void genradix_free_node(struct genradix_node *node)
> +{
> +	kmemleak_free(node);
> +	free_page((unsigned long)node);
> +}
> +
>  /*
>   * Returns pointer to the specified byte @offset within @radix, allocating it if
>   * necessary - newly allocated slots are always zeroed out:
> @@ -97,8 +119,7 @@ void *__genradix_ptr_alloc(struct __genradix *radix, size_t offset,
>  			break;
>  
>  		if (!new_node) {
> -			new_node = (void *)
> -				__get_free_page(gfp_mask|__GFP_ZERO);
> +			new_node = genradix_alloc_node(gfp_mask);
>  			if (!new_node)
>  				return NULL;
>  		}
> @@ -121,8 +142,7 @@ void *__genradix_ptr_alloc(struct __genradix *radix, size_t offset,
>  		n = READ_ONCE(*p);
>  		if (!n) {
>  			if (!new_node) {
> -				new_node = (void *)
> -					__get_free_page(gfp_mask|__GFP_ZERO);
> +				new_node = genradix_alloc_node(gfp_mask);
>  				if (!new_node)
>  					return NULL;
>  			}
> @@ -133,7 +153,7 @@ void *__genradix_ptr_alloc(struct __genradix *radix, size_t offset,
>  	}
>  
>  	if (new_node)
> -		free_page((unsigned long) new_node);
> +		genradix_free_node(new_node);
>  
>  	return &n->data[offset];
>  }
> @@ -191,7 +211,7 @@ static void genradix_free_recurse(struct genradix_node *n, unsigned level)
>  				genradix_free_recurse(n->children[i], level - 1);
>  	}
>  
> -	free_page((unsigned long) n);
> +	genradix_free_node(n);
>  }
>  
>  int __genradix_prealloc(struct __genradix *radix, size_t size,
> -- 
> 2.23.0
>
Neil Horman Oct. 4, 2019, 12:27 p.m. UTC | #2
On Thu, Oct 03, 2019 at 11:50:39PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Kmemleak is falsely reporting a leak of the slab allocation in
> sctp_stream_init_ext():
> 
> BUG: memory leak
> unreferenced object 0xffff8881114f5d80 (size 96):
>    comm "syz-executor934", pid 7160, jiffies 4294993058 (age 31.950s)
>    hex dump (first 32 bytes):
>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>    backtrace:
>      [<00000000ce7a1326>] kmemleak_alloc_recursive  include/linux/kmemleak.h:55 [inline]
>      [<00000000ce7a1326>] slab_post_alloc_hook mm/slab.h:439 [inline]
>      [<00000000ce7a1326>] slab_alloc mm/slab.c:3326 [inline]
>      [<00000000ce7a1326>] kmem_cache_alloc_trace+0x13d/0x280 mm/slab.c:3553
>      [<000000007abb7ac9>] kmalloc include/linux/slab.h:547 [inline]
>      [<000000007abb7ac9>] kzalloc include/linux/slab.h:742 [inline]
>      [<000000007abb7ac9>] sctp_stream_init_ext+0x2b/0xa0  net/sctp/stream.c:157
>      [<0000000048ecb9c1>] sctp_sendmsg_to_asoc+0x946/0xa00  net/sctp/socket.c:1882
>      [<000000004483ca2b>] sctp_sendmsg+0x2a8/0x990 net/sctp/socket.c:2102
>      [...]
> 
> But it's freed later.  Kmemleak misses the allocation because its
> pointer is stored in the generic radix tree sctp_stream::out, and the
> generic radix tree uses raw pages which aren't tracked by kmemleak.
> 
> Fix this by adding the kmemleak hooks to the generic radix tree code.
> 
> Reported-by: syzbot+7f3b6b106be8dcdcdeec@syzkaller.appspotmail.com
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  lib/generic-radix-tree.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/generic-radix-tree.c b/lib/generic-radix-tree.c
> index ae25e2fa2187..f25eb111c051 100644
> --- a/lib/generic-radix-tree.c
> +++ b/lib/generic-radix-tree.c
> @@ -2,6 +2,7 @@
>  #include <linux/export.h>
>  #include <linux/generic-radix-tree.h>
>  #include <linux/gfp.h>
> +#include <linux/kmemleak.h>
>  
>  #define GENRADIX_ARY		(PAGE_SIZE / sizeof(struct genradix_node *))
>  #define GENRADIX_ARY_SHIFT	ilog2(GENRADIX_ARY)
> @@ -75,6 +76,27 @@ void *__genradix_ptr(struct __genradix *radix, size_t offset)
>  }
>  EXPORT_SYMBOL(__genradix_ptr);
>  
> +static inline struct genradix_node *genradix_alloc_node(gfp_t gfp_mask)
> +{
> +	struct genradix_node *node;
> +
> +	node = (struct genradix_node *)__get_free_page(gfp_mask|__GFP_ZERO);
> +
> +	/*
> +	 * We're using pages (not slab allocations) directly for kernel data
> +	 * structures, so we need to explicitly inform kmemleak of them in order
> +	 * to avoid false positive memory leak reports.
> +	 */
> +	kmemleak_alloc(node, PAGE_SIZE, 1, gfp_mask);
> +	return node;
> +}
> +
> +static inline void genradix_free_node(struct genradix_node *node)
> +{
> +	kmemleak_free(node);
> +	free_page((unsigned long)node);
> +}
> +
>  /*
>   * Returns pointer to the specified byte @offset within @radix, allocating it if
>   * necessary - newly allocated slots are always zeroed out:
> @@ -97,8 +119,7 @@ void *__genradix_ptr_alloc(struct __genradix *radix, size_t offset,
>  			break;
>  
>  		if (!new_node) {
> -			new_node = (void *)
> -				__get_free_page(gfp_mask|__GFP_ZERO);
> +			new_node = genradix_alloc_node(gfp_mask);
>  			if (!new_node)
>  				return NULL;
>  		}
> @@ -121,8 +142,7 @@ void *__genradix_ptr_alloc(struct __genradix *radix, size_t offset,
>  		n = READ_ONCE(*p);
>  		if (!n) {
>  			if (!new_node) {
> -				new_node = (void *)
> -					__get_free_page(gfp_mask|__GFP_ZERO);
> +				new_node = genradix_alloc_node(gfp_mask);
>  				if (!new_node)
>  					return NULL;
>  			}
> @@ -133,7 +153,7 @@ void *__genradix_ptr_alloc(struct __genradix *radix, size_t offset,
>  	}
>  
>  	if (new_node)
> -		free_page((unsigned long) new_node);
> +		genradix_free_node(new_node);
>  
>  	return &n->data[offset];
>  }
> @@ -191,7 +211,7 @@ static void genradix_free_recurse(struct genradix_node *n, unsigned level)
>  				genradix_free_recurse(n->children[i], level - 1);
>  	}
>  
> -	free_page((unsigned long) n);
> +	genradix_free_node(n);
>  }
>  
>  int __genradix_prealloc(struct __genradix *radix, size_t size,
> -- 
> 2.23.0
> 
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Catalin Marinas Oct. 4, 2019, 2:48 p.m. UTC | #3
On Thu, Oct 03, 2019 at 11:50:39PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Kmemleak is falsely reporting a leak of the slab allocation in
> sctp_stream_init_ext():
> 
> BUG: memory leak
> unreferenced object 0xffff8881114f5d80 (size 96):
>    comm "syz-executor934", pid 7160, jiffies 4294993058 (age 31.950s)
>    hex dump (first 32 bytes):
>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>    backtrace:
>      [<00000000ce7a1326>] kmemleak_alloc_recursive  include/linux/kmemleak.h:55 [inline]
>      [<00000000ce7a1326>] slab_post_alloc_hook mm/slab.h:439 [inline]
>      [<00000000ce7a1326>] slab_alloc mm/slab.c:3326 [inline]
>      [<00000000ce7a1326>] kmem_cache_alloc_trace+0x13d/0x280 mm/slab.c:3553
>      [<000000007abb7ac9>] kmalloc include/linux/slab.h:547 [inline]
>      [<000000007abb7ac9>] kzalloc include/linux/slab.h:742 [inline]
>      [<000000007abb7ac9>] sctp_stream_init_ext+0x2b/0xa0  net/sctp/stream.c:157
>      [<0000000048ecb9c1>] sctp_sendmsg_to_asoc+0x946/0xa00  net/sctp/socket.c:1882
>      [<000000004483ca2b>] sctp_sendmsg+0x2a8/0x990 net/sctp/socket.c:2102
>      [...]
> 
> But it's freed later.  Kmemleak misses the allocation because its
> pointer is stored in the generic radix tree sctp_stream::out, and the
> generic radix tree uses raw pages which aren't tracked by kmemleak.
> 
> Fix this by adding the kmemleak hooks to the generic radix tree code.
> 
> Reported-by: syzbot+7f3b6b106be8dcdcdeec@syzkaller.appspotmail.com
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Patch
diff mbox series

diff --git a/lib/generic-radix-tree.c b/lib/generic-radix-tree.c
index ae25e2fa2187..f25eb111c051 100644
--- a/lib/generic-radix-tree.c
+++ b/lib/generic-radix-tree.c
@@ -2,6 +2,7 @@ 
 #include <linux/export.h>
 #include <linux/generic-radix-tree.h>
 #include <linux/gfp.h>
+#include <linux/kmemleak.h>
 
 #define GENRADIX_ARY		(PAGE_SIZE / sizeof(struct genradix_node *))
 #define GENRADIX_ARY_SHIFT	ilog2(GENRADIX_ARY)
@@ -75,6 +76,27 @@  void *__genradix_ptr(struct __genradix *radix, size_t offset)
 }
 EXPORT_SYMBOL(__genradix_ptr);
 
+static inline struct genradix_node *genradix_alloc_node(gfp_t gfp_mask)
+{
+	struct genradix_node *node;
+
+	node = (struct genradix_node *)__get_free_page(gfp_mask|__GFP_ZERO);
+
+	/*
+	 * We're using pages (not slab allocations) directly for kernel data
+	 * structures, so we need to explicitly inform kmemleak of them in order
+	 * to avoid false positive memory leak reports.
+	 */
+	kmemleak_alloc(node, PAGE_SIZE, 1, gfp_mask);
+	return node;
+}
+
+static inline void genradix_free_node(struct genradix_node *node)
+{
+	kmemleak_free(node);
+	free_page((unsigned long)node);
+}
+
 /*
  * Returns pointer to the specified byte @offset within @radix, allocating it if
  * necessary - newly allocated slots are always zeroed out:
@@ -97,8 +119,7 @@  void *__genradix_ptr_alloc(struct __genradix *radix, size_t offset,
 			break;
 
 		if (!new_node) {
-			new_node = (void *)
-				__get_free_page(gfp_mask|__GFP_ZERO);
+			new_node = genradix_alloc_node(gfp_mask);
 			if (!new_node)
 				return NULL;
 		}
@@ -121,8 +142,7 @@  void *__genradix_ptr_alloc(struct __genradix *radix, size_t offset,
 		n = READ_ONCE(*p);
 		if (!n) {
 			if (!new_node) {
-				new_node = (void *)
-					__get_free_page(gfp_mask|__GFP_ZERO);
+				new_node = genradix_alloc_node(gfp_mask);
 				if (!new_node)
 					return NULL;
 			}
@@ -133,7 +153,7 @@  void *__genradix_ptr_alloc(struct __genradix *radix, size_t offset,
 	}
 
 	if (new_node)
-		free_page((unsigned long) new_node);
+		genradix_free_node(new_node);
 
 	return &n->data[offset];
 }
@@ -191,7 +211,7 @@  static void genradix_free_recurse(struct genradix_node *n, unsigned level)
 				genradix_free_recurse(n->children[i], level - 1);
 	}
 
-	free_page((unsigned long) n);
+	genradix_free_node(n);
 }
 
 int __genradix_prealloc(struct __genradix *radix, size_t size,