From 3939a7fd0b94f11f376670592b2845451569675c Mon Sep 17 00:00:00 2001 From: Matthew Wilcox Date: Wed, 5 Dec 2018 15:43:27 -0500 Subject: [PATCH] maple_tree: Fix memory leak when two threads race If another thread succeeds in allocating the node while we're sleeping to allocate memory, we'll never use the ms->alloc, so it has to be freed by mas_nomem(). Signed-off-by: Matthew Wilcox --- include/linux/maple_tree.h | 4 ++++ lib/maple_tree.c | 16 +++++++++++----- lib/test_maple_tree.c | 24 ++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/include/linux/maple_tree.h b/include/linux/maple_tree.h index d42cb5da74be..7ba0432c8c40 100644 --- a/include/linux/maple_tree.h +++ b/include/linux/maple_tree.h @@ -150,6 +150,7 @@ struct maple_state { */ #define MAS_START ((struct maple_node *)1UL) #define MAS_ROOT ((struct maple_node *)3UL) +#define MA_ERROR(err) ((struct maple_node *)(((unsigned long)err << 2) | 2UL)) #define MAP_STATE(sname, stree, sindex, eindex)\ struct maple_state sname = { \ @@ -193,4 +194,7 @@ static inline bool mtree_empty(const struct maple_tree *mt) return mt->root == NULL; } +/* Advanced API */ + +bool mas_nomem(struct maple_state *, gfp_t); #endif diff --git a/lib/maple_tree.c b/lib/maple_tree.c index 6fd0ee8d3ef6..a1f0c041add7 100644 --- a/lib/maple_tree.c +++ b/lib/maple_tree.c @@ -6,8 +6,6 @@ #include #include -#define MA_ERROR(err) ((struct maple_node *)(((unsigned long)err << 2) | 2UL)) - static inline void mas_set_err(struct maple_state *mas, long err) { mas->node = MA_ERROR(err); @@ -155,11 +153,19 @@ list_failed: mas_set_err(ms, -ENOMEM); } -static bool __maple_nomem(struct maple_state *ms, gfp_t gfp) +bool mas_nomem(struct maple_state *ms, gfp_t gfp) __must_hold(ms->tree->lock) { - if (ms->node != MA_ERROR(-ENOMEM)) + if (ms->node != MA_ERROR(-ENOMEM)) { + struct maple_node *node = ma_get_alloc(ms); + if (node) { + kfree(node->slot[1]); + kfree(node->slot[0]); + kfree(node); + } + ms->alloc = NULL; return false; + } if (gfpflags_allow_blocking(gfp)) { mtree_unlock(ms->tree); @@ -734,7 +740,7 @@ retry: goto already_exists; ret = _maple_insert(&ms, entry); - if (__maple_nomem(&ms, gfp)) + if (mas_nomem(&ms, gfp)) goto retry; already_exists: diff --git a/lib/test_maple_tree.c b/lib/test_maple_tree.c index d7a4ec038575..e4bf5efbc06e 100644 --- a/lib/test_maple_tree.c +++ b/lib/test_maple_tree.c @@ -89,6 +89,8 @@ static noinline void check_load(struct maple_tree *mt, unsigned long index, static noinline void check_nomem(struct maple_tree *mt) { + MAP_STATE(ms, mt, 1, 1); + MT_BUG_ON(mt, !mtree_empty(mt)); /* Storing something at 1 requires memory allocation */ @@ -96,6 +98,28 @@ static noinline void check_nomem(struct maple_tree *mt) /* Storing something at 0 does not */ MT_BUG_ON(mt, mtree_insert_index(mt, 0, GFP_ATOMIC) != 0); + /* + * Simulate two threads racing; the first one fails to allocate + * memory to insert an entry at 1, then the second one succeeds + * in allocating memory to insert an entry at 2. The first one + * then needs to free the node it allocated. LeakSanitizer will + * notice this, as will the 'nr_allocated' debugging aid in the + * userspace test suite. + */ + mtree_lock(mt); + _maple_setup_insert(&ms); + MT_BUG_ON(mt, ms.node != MA_ERROR(-ENOMEM)); + _maple_insert(&ms, xa_mk_value(1)); + mas_nomem(&ms, GFP_KERNEL); + MT_BUG_ON(mt, ms.node != MAS_START); + mtree_unlock(mt); + MT_BUG_ON(mt, mtree_insert_index(mt, 2, GFP_KERNEL) != 0); + mtree_lock(mt); + _maple_setup_insert(&ms); + _maple_insert(&ms, xa_mk_value(1)); + mas_nomem(&ms, GFP_KERNEL); + mtree_unlock(mt); + mtree_destroy(mt); } -- 2.50.1