| Message ID | 20191004065039.727564-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lib/generic-radix-tree.c: add kmemleak annotations | expand |
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 >
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>
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>
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,