Re: [PATCH 1/1] z3fold: fix memory leak

[Andrew Morton]

On Wed, 4 Apr 2018 15:20:39 -0700 Andrew Morton <a...@linux-foundation.org> 
wrote:

> On Wed,  4 Apr 2018 08:51:51 +0800 Xidong Wang <wangxidong...@163.com> 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;
}