lkml.org 
[lkml]   [2006]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH] fix memory leak in mm/slab.c::alloc_kmemlist() (try #2)
    Date
    The Coverity checker found that we may leak memory in
    mm/slab.c::alloc_kmemlist()
    This should fix the leak and coverity bug #589

    Currently the only caller of alloc_kmemlist() will BUG() if alloc_kmemlist()
    fails, but that doesn't mean we shouldn't clean up properly IMHO. Also, the
    caller (do_tune_cpucache()) could maybe be changed in the future to do
    something more clever than just BUG() and in that case we really shouldn't
    be leaking memory when we return -ENOMEM.

    The patch introduces one more loop to the function in the failure path :-(
    But, since we are very unlikely to ever hit the failure path this shouldn't
    be too painfull.

    The patch has been compile and boot tested on x86, but since I'm not very
    intimate with the slab code I'd appreciate it if someone would take a close
    look on the changes before merging them.
    IMO this patch should not go into 2.6.16, but instead spend some time in -mm
    with the intention to merge it for 2.6.17 - although it does fix a real leak
    it's not a regression compared to 2.6.15.


    Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
    ---

    mm/slab.c | 35 +++++++++++++++++++++++++++++------
    1 files changed, 29 insertions(+), 6 deletions(-)

    --- linux-2.6.16-rc6-mm2-orig/mm/slab.c 2006-03-18 16:55:55.000000000 +0100
    +++ linux-2.6.16-rc6-mm2/mm/slab.c 2006-03-18 21:10:56.000000000 +0100
    @@ -3399,12 +3399,17 @@ EXPORT_SYMBOL_GPL(kmem_cache_name);
    static int alloc_kmemlist(struct kmem_cache *cachep)
    {
    int node;
    + int count = -1;
    struct kmem_list3 *l3;
    - int err = 0;
    + struct array_cache *new;
    + struct array_cache **new_alien;

    for_each_online_node(node) {
    - struct array_cache *nc = NULL, *new;
    - struct array_cache **new_alien = NULL;
    + struct array_cache *nc = NULL;
    +
    + new = NULL;
    + new_alien = NULL;
    + count++;
    #ifdef CONFIG_NUMA
    new_alien = alloc_alien_cache(node, cachep->limit);
    if (!new_alien)
    @@ -3447,10 +3452,28 @@ static int alloc_kmemlist(struct kmem_ca
    cachep->batchcount + cachep->num;
    cachep->nodelists[node] = l3;
    }
    - return err;
    + return 0;
    +/*
    + If one or more allocations fail we need to undo all allocations done up to
    + this point.
    + Unfortunately this means yet another loop, but since this only happens on
    + failure and frees up memory in a memory-tight situation, it's not too bad.
    + */
    fail:
    - err = -ENOMEM;
    - return err;
    + kfree(new);
    + free_alien_cache(new_alien);
    + for_each_online_node(node) {
    + if (count <= 0)
    + break;
    + if (cachep->nodelists[node]) {
    + kfree(cachep->nodelists[node]->shared);
    + free_alien_cache(cachep->nodelists[node]->alien);
    + kfree(cachep->nodelists[node]);
    + cachep->nodelists[node] = NULL;
    + }
    + count--;
    + }
    + return -ENOMEM;
    }

    struct ccupdate_struct {


    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-03-18 21:40    [W:0.027 / U:1.680 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site