Patchwork ubi fastmap memory leak

login
register
mail settings
Submitter wang.bo116@zte.com.cn
Date April 11, 2013, 9:23 a.m.
Message ID <OF132C4FA1.3FE4C814-ON48257B4A.00337A2A-48257B4A.0033A6D2@zte.com.cn>
Download mbox | patch
Permalink /patch/235709/
State New
Headers show

Comments

wang.bo116@zte.com.cn - April 11, 2013, 9:23 a.m.
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.
 
 
                                               0, 0, NULL);
        if (!ai->aeb_slab_cache) {
richard -rw- weinberger - April 11, 2013, 2:53 p.m.
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
wang.bo116@zte.com.cn - April 12, 2013, 6:58 a.m.
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.

Patch

--- 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),