In function z3fold_create_pool(), the memory allocated by
__alloc_percpu() is not released on the error path that pool->compact_wq
, which holds the return value of create_singlethread_workqueue(), is NULL.
This will result in a memory leak bug.
Signed-off-by: Xidong Wang <[email protected]>
---
mm/z3fold.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/mm/z3fold.c b/mm/z3fold.c
index d589d31..b987cc5 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -490,6 +490,7 @@ static struct z3fold_pool *z3fold_create_pool(const char *name, gfp_t gfp,
out_wq:
destroy_workqueue(pool->compact_wq);
out:
+ free_percpu(pool->unbuddied);
kfree(pool);
return NULL;
}
--
2.7.4
On Wed, 4 Apr 2018 08:51:51 +0800 Xidong Wang <[email protected]> wrote:
> In function z3fold_create_pool(), the memory allocated by
> __alloc_percpu() is not released on the error path that pool->compact_wq
> , which holds the return value of create_singlethread_workqueue(), is NULL.
> This will result in a memory leak bug.
>
> ...
>
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -490,6 +490,7 @@ static struct z3fold_pool *z3fold_create_pool(const char *name, gfp_t gfp,
> out_wq:
> destroy_workqueue(pool->compact_wq);
> out:
> + free_percpu(pool->unbuddied);
> kfree(pool);
> return NULL;
> }
That isn't right. If the initial kzallc fails we'll goto out with
pool==NULL.
Please check:
--- a/mm/z3fold.c~z3fold-fix-memory-leak-fix
+++ a/mm/z3fold.c
@@ -479,7 +479,7 @@ static struct z3fold_pool *z3fold_create
pool->name = name;
pool->compact_wq = create_singlethread_workqueue(pool->name);
if (!pool->compact_wq)
- goto out;
+ goto out_unbuddied;
pool->release_wq = create_singlethread_workqueue(pool->name);
if (!pool->release_wq)
goto out_wq;
@@ -489,9 +489,10 @@ static struct z3fold_pool *z3fold_create
out_wq:
destroy_workqueue(pool->compact_wq);
-out:
+out_unbuddied:
free_percpu(pool->unbuddied);
kfree(pool);
+out:
return NULL;
}
_
On Wed, 4 Apr 2018 15:20:39 -0700 Andrew Morton <[email protected]> wrote:
> On Wed, 4 Apr 2018 08:51:51 +0800 Xidong Wang <[email protected]> wrote:
>
> > In function z3fold_create_pool(), the memory allocated by
> > __alloc_percpu() is not released on the error path that pool->compact_wq
> > , which holds the return value of create_singlethread_workqueue(), is NULL.
> > This will result in a memory leak bug.
> >
> > ...
> >
> > --- a/mm/z3fold.c
> > +++ b/mm/z3fold.c
> > @@ -490,6 +490,7 @@ static struct z3fold_pool *z3fold_create_pool(const char *name, gfp_t gfp,
> > out_wq:
> > destroy_workqueue(pool->compact_wq);
> > out:
> > + free_percpu(pool->unbuddied);
> > kfree(pool);
> > return NULL;
> > }
>
> That isn't right. If the initial kzallc fails we'll goto out with
> pool==NULL.
>
> Please check:
>
> --- a/mm/z3fold.c~z3fold-fix-memory-leak-fix
> +++ a/mm/z3fold.c
> @@ -479,7 +479,7 @@ static struct z3fold_pool *z3fold_create
> pool->name = name;
> pool->compact_wq = create_singlethread_workqueue(pool->name);
> if (!pool->compact_wq)
> - goto out;
> + goto out_unbuddied;
> pool->release_wq = create_singlethread_workqueue(pool->name);
> if (!pool->release_wq)
> goto out_wq;
> @@ -489,9 +489,10 @@ static struct z3fold_pool *z3fold_create
>
> out_wq:
> destroy_workqueue(pool->compact_wq);
> -out:
> +out_unbuddied:
> free_percpu(pool->unbuddied);
> kfree(pool);
> +out:
> return NULL;
> }
We may as well check that __alloc_percpu() return value, too:
--- a/mm/z3fold.c~z3fold-fix-memory-leak-fix
+++ a/mm/z3fold.c
@@ -467,6 +467,8 @@ static struct z3fold_pool *z3fold_create
spin_lock_init(&pool->lock);
spin_lock_init(&pool->stale_lock);
pool->unbuddied = __alloc_percpu(sizeof(struct list_head)*NCHUNKS, 2);
+ if (!pool->unbuddied)
+ goto out_pool;
for_each_possible_cpu(cpu) {
struct list_head *unbuddied =
per_cpu_ptr(pool->unbuddied, cpu);
@@ -479,7 +481,7 @@ static struct z3fold_pool *z3fold_create
pool->name = name;
pool->compact_wq = create_singlethread_workqueue(pool->name);
if (!pool->compact_wq)
- goto out;
+ goto out_unbuddied;
pool->release_wq = create_singlethread_workqueue(pool->name);
if (!pool->release_wq)
goto out_wq;
@@ -489,9 +491,11 @@ static struct z3fold_pool *z3fold_create
out_wq:
destroy_workqueue(pool->compact_wq);
-out:
+out_unbuddied:
free_percpu(pool->unbuddied);
+out_pool:
kfree(pool);
+out:
return NULL;
}
End result:
static struct z3fold_pool *z3fold_create_pool(const char *name, gfp_t gfp,
const struct z3fold_ops *ops)
{
struct z3fold_pool *pool = NULL;
int i, cpu;
pool = kzalloc(sizeof(struct z3fold_pool), gfp);
if (!pool)
goto out;
spin_lock_init(&pool->lock);
spin_lock_init(&pool->stale_lock);
pool->unbuddied = __alloc_percpu(sizeof(struct list_head)*NCHUNKS, 2);
if (!pool->unbuddied)
goto out_pool;
for_each_possible_cpu(cpu) {
struct list_head *unbuddied =
per_cpu_ptr(pool->unbuddied, cpu);
for_each_unbuddied_list(i, 0)
INIT_LIST_HEAD(&unbuddied[i]);
}
INIT_LIST_HEAD(&pool->lru);
INIT_LIST_HEAD(&pool->stale);
atomic64_set(&pool->pages_nr, 0);
pool->name = name;
pool->compact_wq = create_singlethread_workqueue(pool->name);
if (!pool->compact_wq)
goto out_unbuddied;
pool->release_wq = create_singlethread_workqueue(pool->name);
if (!pool->release_wq)
goto out_wq;
INIT_WORK(&pool->work, free_pages_work);
pool->ops = ops;
return pool;
out_wq:
destroy_workqueue(pool->compact_wq);
out_unbuddied:
free_percpu(pool->unbuddied);
out_pool:
kfree(pool);
out:
return NULL;
}
Hi Xidong,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mmotm/master]
[also build test WARNING on v4.16 next-20180405]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Xidong-Wang/z3fold-fix-memory-leak/20180404-114952
base: git://git.cmpxchg.org/linux-mmotm.git master
smatch warnings:
mm/z3fold.c:493 z3fold_create_pool() error: potential null dereference 'pool'. (kzalloc returns null)
mm/z3fold.c:493 z3fold_create_pool() error: we previously assumed 'pool' could be null (see line 465)
vim +/pool +493 mm/z3fold.c
443
444
445 /*
446 * API Functions
447 */
448
449 /**
450 * z3fold_create_pool() - create a new z3fold pool
451 * @name: pool name
452 * @gfp: gfp flags when allocating the z3fold pool structure
453 * @ops: user-defined operations for the z3fold pool
454 *
455 * Return: pointer to the new z3fold pool or NULL if the metadata allocation
456 * failed.
457 */
458 static struct z3fold_pool *z3fold_create_pool(const char *name, gfp_t gfp,
459 const struct z3fold_ops *ops)
460 {
461 struct z3fold_pool *pool = NULL;
462 int i, cpu;
463
464 pool = kzalloc(sizeof(struct z3fold_pool), gfp);
> 465 if (!pool)
466 goto out;
467 spin_lock_init(&pool->lock);
468 spin_lock_init(&pool->stale_lock);
469 pool->unbuddied = __alloc_percpu(sizeof(struct list_head)*NCHUNKS, 2);
470 for_each_possible_cpu(cpu) {
471 struct list_head *unbuddied =
472 per_cpu_ptr(pool->unbuddied, cpu);
473 for_each_unbuddied_list(i, 0)
474 INIT_LIST_HEAD(&unbuddied[i]);
475 }
476 INIT_LIST_HEAD(&pool->lru);
477 INIT_LIST_HEAD(&pool->stale);
478 atomic64_set(&pool->pages_nr, 0);
479 pool->name = name;
480 pool->compact_wq = create_singlethread_workqueue(pool->name);
481 if (!pool->compact_wq)
482 goto out;
483 pool->release_wq = create_singlethread_workqueue(pool->name);
484 if (!pool->release_wq)
485 goto out_wq;
486 INIT_WORK(&pool->work, free_pages_work);
487 pool->ops = ops;
488 return pool;
489
490 out_wq:
491 destroy_workqueue(pool->compact_wq);
492 out:
> 493 free_percpu(pool->unbuddied);
494 kfree(pool);
495 return NULL;
496 }
497
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation