lkml.org 
[lkml]   [2009]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: Poking ieee80211_default_rc_algo causes kernel lockups
    Date
    On Monday 16 February 2009 06:05:23 Frederic Weisbecker wrote:
    > So the problem seems not related to mac80211 but actually to module parameter management.
    > I'm not sure what is the right fix.

    Good spotting. And in fact this bug has been here for quite a while...

    The "simple" fix is to kstrdup, but that leaks.

    How's this?

    param: fix charp parameters set via sysfs (eg. ieee80211_default_rc_algo)

    The module_param type "charp" simply sets a char * pointer in the
    module to the parameter in the commandline string: this is why we keep
    the (mangled) module command line around. But when set via sysfs (as
    about 11 charp parameters can be) this memory is freed on the way
    out of the write().

    So we kstrdup instead: this is fine, but it means we have to note when
    we've used it so we can reliably kfree the parameter when it's next
    overwritten, and also on module unload.

    Reported-by: Frederic Weisbecker <fweisbec@gmail.com>
    Cc: Frederic Weisbecker <fweisbec@gmail.com>

    diff --git a/include/linux/module.h b/include/linux/module.h
    --- a/include/linux/module.h
    +++ b/include/linux/module.h
    @@ -247,6 +247,10 @@ struct module
    const struct kernel_symbol *syms;
    const unsigned long *crcs;
    unsigned int num_syms;
    +
    + /* Kernel parameters. */
    + struct kernel_param *kp;
    + unsigned int num_kp;

    /* GPL-only exported symbols. */
    unsigned int num_gpl_syms;
    diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
    --- a/include/linux/moduleparam.h
    +++ b/include/linux/moduleparam.h
    @@ -138,6 +138,9 @@ extern int parse_args(const char *name,
    unsigned num,
    int (*unknown)(char *param, char *val));

    +/* Called by module remove. */
    +extern void destroy_params(const struct kernel_param *params, unsigned num);
    +
    /* All the helper functions */
    /* The macros to do compile-time type checking stolen from Jakub
    Jelinek, who IIRC came up with this idea for the 2.4 module init code. */
    diff --git a/kernel/module.c b/kernel/module.c
    --- a/kernel/module.c
    +++ b/kernel/module.c
    @@ -1457,6 +1457,9 @@ static void free_module(struct module *m
    /* Module unload stuff */
    module_unload_free(mod);

    + /* Free any allocated parameters. */
    + destroy_params(mod->kp, mod->num_kp);
    +
    /* release any pointers to mcount in this module */
    ftrace_release(mod->module_core, mod->core_size);

    @@ -1870,8 +1873,7 @@ static noinline struct module *load_modu
    unsigned int symindex = 0;
    unsigned int strindex = 0;
    unsigned int modindex, versindex, infoindex, pcpuindex;
    - unsigned int num_kp, num_mcount;
    - struct kernel_param *kp;
    + unsigned int num_mcount;
    struct module *mod;
    long err = 0;
    void *percpu = NULL, *ptr = NULL; /* Stops spurious gcc warning */
    @@ -2116,8 +2118,8 @@ static noinline struct module *load_modu

    /* Now we've got everything in the final locations, we can
    * find optional sections. */
    - kp = section_objs(hdr, sechdrs, secstrings, "__param", sizeof(*kp),
    - &num_kp);
    + mod->kp = section_objs(hdr, sechdrs, secstrings, "__param",
    + sizeof(*mod->kp), &mod->num_kp);
    mod->syms = section_objs(hdr, sechdrs, secstrings, "__ksymtab",
    sizeof(*mod->syms), &mod->num_syms);
    mod->crcs = section_addr(hdr, sechdrs, secstrings, "__kcrctab");
    @@ -2262,11 +2264,11 @@ static noinline struct module *load_modu
    */
    list_add_rcu(&mod->list, &modules);

    - err = parse_args(mod->name, mod->args, kp, num_kp, NULL);
    + err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL);
    if (err < 0)
    goto unlink;

    - err = mod_sysfs_setup(mod, kp, num_kp);
    + err = mod_sysfs_setup(mod, mod->kp, mod->num_kp);
    if (err < 0)
    goto unlink;
    add_sect_attrs(mod, hdr->e_shnum, secstrings, sechdrs);
    diff --git a/kernel/params.c b/kernel/params.c
    --- a/kernel/params.c
    +++ b/kernel/params.c
    @@ -23,6 +23,9 @@
    #include <linux/device.h>
    #include <linux/err.h>
    #include <linux/slab.h>
    +
    +/* We abuse the high bits of "perm" to record whether we kmalloc'ed. */
    +#define KPARAM_KMALLOCED 0x80000000

    #if 0
    #define DEBUGP printk
    @@ -217,7 +220,13 @@ int param_set_charp(const char *val, str
    return -ENOSPC;
    }

    - *(char **)kp->arg = (char *)val;
    + if (kp->perm & KPARAM_KMALLOCED)
    + kfree(*(char **)kp->arg);
    +
    + kp->perm |= KPARAM_KMALLOCED;
    + *(char **)kp->arg = kstrdup(val, GFP_KERNEL);
    + if (!kp->arg)
    + return -ENOMEM;
    return 0;
    }

    @@ -571,6 +580,15 @@ void module_param_sysfs_remove(struct mo
    }
    #endif

    +void destroy_params(const struct kernel_param *params, unsigned num)
    +{
    + unsigned int i;
    +
    + for (i = 0; i < num; i++)
    + if (params[i].perm & KPARAM_KMALLOCED)
    + kfree(*(char **)params[i].arg);
    +}
    +
    static void __init kernel_add_sysfs_param(const char *name,
    struct kernel_param *kparam,
    unsigned int name_skip)

    \
     
     \ /
      Last update: 2009-02-18 12:23    [W:3.458 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site