* Re: [PATCH] lib/generic-radix-tree.c: add kmemleak annotations
2019-10-04 6:50 ` [PATCH] lib/generic-radix-tree.c: add kmemleak annotations Eric Biggers
@ 2019-10-04 12:21 ` Marcelo Ricardo Leitner
2019-10-04 12:27 ` Neil Horman
2019-10-04 14:48 ` Catalin Marinas
2 siblings, 0 replies; 4+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-10-04 12:21 UTC (permalink / raw)
To: Eric Biggers
Cc: Andrew Morton, linux-mm, linux-sctp, linux-kernel, syzkaller-bugs,
Catalin Marinas, Kent Overstreet, Vlad Yasevich, Xin Long,
Neil Horman
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] lib/generic-radix-tree.c: add kmemleak annotations
2019-10-04 6:50 ` [PATCH] lib/generic-radix-tree.c: add kmemleak annotations Eric Biggers
2019-10-04 12:21 ` Marcelo Ricardo Leitner
@ 2019-10-04 12:27 ` Neil Horman
2019-10-04 14:48 ` Catalin Marinas
2 siblings, 0 replies; 4+ messages in thread
From: Neil Horman @ 2019-10-04 12:27 UTC (permalink / raw)
To: Eric Biggers
Cc: Andrew Morton, linux-mm, linux-sctp, linux-kernel, syzkaller-bugs,
Catalin Marinas, Kent Overstreet, Vlad Yasevich, Xin Long,
Marcelo Ricardo Leitner
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] lib/generic-radix-tree.c: add kmemleak annotations
2019-10-04 6:50 ` [PATCH] lib/generic-radix-tree.c: add kmemleak annotations Eric Biggers
2019-10-04 12:21 ` Marcelo Ricardo Leitner
2019-10-04 12:27 ` Neil Horman
@ 2019-10-04 14:48 ` Catalin Marinas
2 siblings, 0 replies; 4+ messages in thread
From: Catalin Marinas @ 2019-10-04 14:48 UTC (permalink / raw)
To: Eric Biggers
Cc: Andrew Morton, linux-mm, linux-sctp, linux-kernel, syzkaller-bugs,
Kent Overstreet, Vlad Yasevich, Xin Long, Neil Horman,
Marcelo Ricardo Leitner
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>
^ permalink raw reply [flat|nested] 4+ messages in thread