Re: [PATCH] kernel/module.c: Free lock-classes if parse_args failed

[Andrey Tsyvarev]

20.01.2015 9:37, Rusty Russell пишет:
> Andrey Tsyvarev <tsyvarev@xxxxxxxxx> writes:
> > parse_args call module parameters' .set handlers, which may use locks defined in the module.
> > So, these classes should be freed in case parse_args returns error(e.g. due to incorrect parameter passed).
> Thanks, this seems right.  Applied.
>
> But this makes me ask: where is lockdep_free_key_range() called on the
> module init code?  It doesn't seem to be at all...
As I understand, locks are not allowed to be defined in the module init 
section. So, no needs to call lockdep_free_key_range() for it.
This has a sense: objects from that section are allowed to be used only 
by module->init() function. But a single function call doesn't require 
any synchronization wrt itself.


If it helps, I used module below for verify effect of the patch:

test.c:
---
#include <linux/module.h>
#include <linux/moduleparam.h>

MODULE_AUTHOR("Tsyvarev Andrey");
MODULE_LICENSE("GPL");

static DEFINE_MUTEX(mutex_main);
static DEFINE_MUTEX(mutex_param);

static int param_set(const char* val, const struct kernel_param* kp)
{
    mutex_lock(&mutex_param); //Use mutex defined in the module
    mutex_unlock(&mutex_param);

    return -EINVAL; // Setting this parameter is always invalid
}

static struct kernel_param_ops param_ops =
{
    .set = param_set
};

module_param_cb(param, &param_ops, NULL, S_IRUGO);

static int minit(void)
{
    mutex_lock(&mutex_main); // Use another mutex
    mutex_unlock(&mutex_main);
    return 0;
}

static void mexit(void)
{
}

module_init(minit);
module_exit(mexit);
--

insmod test.ko param=1 # Will fail
insmod test.ko

If kernel is compiled with CONFIG_LOCKDEP, then, without patch, lockdep 
crashes on the second module insertion.

> Confused,
> Rusty.
>
> > Signed-off-by: Andrey Tsyvarev <tsyvarev@xxxxxxxxx>
> > ---
> >   kernel/module.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 3965511..2b44de4 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -3311,6 +3311,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
> >   	module_bug_cleanup(mod);
> >   	mutex_unlock(&module_mutex);
> >   
> > +	/* Free lock-classes: */
> > +	lockdep_free_key_range(mod->module_core, mod->core_size);
> > +
> >   	/* we can't deallocate the module until we clear memory protection */
> >   	unset_module_init_ro_nx(mod);
> >   	unset_module_core_ro_nx(mod);
> > --
> > 1.8.3.1

--
Best regards,

Andrey Tsyvarev
Linux Verification Center, ISPRAS
web:http://linuxtesting.org

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