ubi fastmap memory leak

ubi fastmap memory leak

[wang.bo116]

Hello，
        When use ubi fastmap,i think there is a memory leak in 
"ubi_attach" function.Think about this scene：
 
        1. In "ubi_attach" function, "alloc_ai" function called to alloc a 
slab cache named "ubi_aeb_slab_cache".
        Then use "ai->aeb_slab_cache" to record the cache. 
 
        2. ubi_attach -> scan_fast -> ubi_scan_fastmap -> 
ubi_attach_fastmap,
        in "ubi_attach_fastmap" function, slab cache named 
"ubi_ainf_peb_slab" will be alloced. 
        Because of this slab cache also recorded in "ai->aeb_slab_cache", 
so, in fact it overwrite the previously one.
 
        3. When "destroy_ai" called , it only free "ubi_ainf_peb_slab".

        I made a patch base on Linux 3.9-rc6, it fix this problem use a 
temporary ai for fastmap scan.
        During my test, this patch like work good.
 
 
--- attach.c    2013-04-08 03:49:54.000000000 +0000
+++ attach1.c   2013-04-11 11:29:00.239412000 +0000
@@ -90,6 +90,7 @@
 #include "ubi.h"
 
 static int self_check_ai(struct ubi_device *ubi, struct ubi_attach_info 
*ai);
+static struct ubi_attach_info *alloc_ai(const char *slab_name);
 
 /* Temporary variables used during scanning */
 static struct ubi_ec_hdr *ech;
@@ -1314,9 +1315,14 @@ static int scan_fast(struct ubi_device *
 {
        int err, pnum, fm_anchor = -1;
        unsigned long long max_sqnum = 0;
-
+       struct ubi_attach_info *fastmap_temp_ai = NULL;
+ 
        err = -ENOMEM;
 
+       fastmap_temp_ai = alloc_ai("ubi_scan_fastmap_slab_cache");
+       if (!fastmap_temp_ai)
+               goto out;
+
        ech = kzalloc(ubi->ec_hdr_alsize, GFP_KERNEL);
        if (!ech)
                goto out;
@@ -1331,7 +1337,7 @@ static int scan_fast(struct ubi_device *
                cond_resched();
 
                dbg_gen("process PEB %d", pnum);
-               err = scan_peb(ubi, ai, pnum, &vol_id, &sqnum);
+               err = scan_peb(ubi, fastmap_temp_ai, pnum, &vol_id, 
&sqnum);
                if (err < 0)
                        goto out_vidh;
 
@@ -1343,6 +1349,7 @@ static int scan_fast(struct ubi_device *
 
        ubi_free_vid_hdr(ubi, vidh);
        kfree(ech);
+       destroy_ai(fastmap_temp_ai);
 
        if (fm_anchor < 0)
                return UBI_NO_FASTMAP;
@@ -1351,6 +1358,7 @@ static int scan_fast(struct ubi_device *
 
 out_vidh:
        ubi_free_vid_hdr(ubi, vidh);
+       destroy_ai(fastmap_temp_ai);
 out_ech:
        kfree(ech);
 out:
@@ -1419,7 +1427,7 @@ int ubi_attach(struct ubi_device *ubi, i
                                        return -ENOMEM;
                        }
 
-                       err = scan_all(ubi, ai, UBI_FM_MAX_START);
+                       err = scan_all(ubi, ai, 0);
                }
        }
 #else

 
 
 
--- fastmap.c   2013-04-08 03:49:54.000000000 +0000
+++ fastmap1.c  2013-04-11 03:22:54.432000000 +0000
@@ -559,7 +559,8 @@ static int ubi_attach_fastmap(struct ubi
        ai->volumes = RB_ROOT;
        ai->min_ec = UBI_MAX_ERASECOUNTER;
 
-       ai->aeb_slab_cache = kmem_cache_create("ubi_ainf_peb_slab",
+       if(!ai->aeb_slab_cache)
+               ai->aeb_slab_cache = 
kmem_cache_create("ubi_ainf_peb_slab",
                                               sizeof(struct 
ubi_ainf_peb),
                                               0, 0, NULL);
        if (!ai->aeb_slab_cache) {

Re: ubi fastmap memory leak

[richard -rw- weinberger]

Hi!

On Thu, Apr 11, 2013 at 11:23 AM,  <wang.bo116@zte.com.cn> wrote:
> Hello，
>         When use ubi fastmap,i think there is a memory leak in
> "ubi_attach" function.Think about this scene：
>
>         1. In "ubi_attach" function, "alloc_ai" function called to alloc a
> slab cache named "ubi_aeb_slab_cache".
>         Then use "ai->aeb_slab_cache" to record the cache.
>
>         2. ubi_attach -> scan_fast -> ubi_scan_fastmap ->
> ubi_attach_fastmap,
>         in "ubi_attach_fastmap" function, slab cache named
> "ubi_ainf_peb_slab" will be alloced.
>         Because of this slab cache also recorded in "ai->aeb_slab_cache",
> so, in fact it overwrite the previously one.
>
>         3. When "destroy_ai" called , it only free "ubi_ainf_peb_slab".
>
>         I made a patch base on Linux 3.9-rc6, it fix this problem use a
> temporary ai for fastmap scan.
>         During my test, this patch like work good.

So, we call ubi_scan_fastmap() with an ai where ->aeb_slab_cache
points already to a
slab cache?
I think it would be best to remove the kmem_cache_create() from
ubi_scan_fastmap()
completely.

What do you think?

--
Thanks,
//richard

答复: Re: ubi fastmap memory leak

[wang.bo116]

Hi，

richard -rw- weinberger <richard.weinberger@gmail.com> 写于 2013-04-11 
22:53:15:

> Hi!
> 
> On Thu, Apr 11, 2013 at 11:23 AM,  <wang.bo116@zte.com.cn> wrote:
> > Hello，
> >         When use ubi fastmap,i think there is a memory leak in
> > "ubi_attach" function.Think about this scene：
> >
> >         1. In "ubi_attach" function, "alloc_ai" function called to 
alloc a
> > slab cache named "ubi_aeb_slab_cache".
> >         Then use "ai->aeb_slab_cache" to record the cache.
> >
> >         2. ubi_attach -> scan_fast -> ubi_scan_fastmap ->
> > ubi_attach_fastmap,
> >         in "ubi_attach_fastmap" function, slab cache named
> > "ubi_ainf_peb_slab" will be alloced.
> >         Because of this slab cache also recorded in 
"ai->aeb_slab_cache",
> > so, in fact it overwrite the previously one.
> >
> >         3. When "destroy_ai" called , it only free 
"ubi_ainf_peb_slab".
> >
> >         I made a patch base on Linux 3.9-rc6, it fix this problem use 
a
> > temporary ai for fastmap scan.
> >         During my test, this patch like work good.
> 
> So, we call ubi_scan_fastmap() with an ai where ->aeb_slab_cache
> points already to a
> slab cache?
> I think it would be best to remove the kmem_cache_create() from
> ubi_scan_fastmap()
> completely.
> 
> What do you think?
> 
> --
> Thanks,
> //richard



Yes，i think ubi_attach_fastmap() no need to alloc slab cache, it already 
be alloced in "ubi_attach".
i think the fllowed code in "ubi_attach_fastmap" is needless.

        ai->aeb_slab_cache = kmem_cache_create("ubi_ainf_peb_slab",
                                               sizeof(struct 
ubi_ainf_peb),
                                               0, 0, NULL);
        if (!ai->aeb_slab_cache) {
                ret = -ENOMEM;
                goto fail;
        }

I notice that "scan_fast" function will scan PEB 0 to UBI_FM_MAX_START-1 
to find a fastmap.
During this scan, it will call "kmem_cache_alloc" to alloc "ubi_ainf_peb" 
form slab, and add it to a list in ai, 
but "ubi_attach_fastmap" expect the list is empty, so it will reinit 
ai->free list and so on.

If ubi_attach_fastmap() want to reuse the slab cache, the alloced slabs 
must be freed before the list(ai->free and so on) be inited.

Compare with free slab before ubi_attach_fastmap function, use a temporary 
ai in scan_fast can use "destroy_ai" function directly.