[PATCH 1/2] module: fix symbol waiting when module fails before init

[PATCH 1/2] module: fix symbol waiting when module fails before init

[Rusty Russell]

We use resolve_symbol_wait(), which blocks if the module containing
the symbol is still loading.  However:

1) The module_wq we use is only woken after calling the modules' init
   function, but there are other failure paths after the module is
   placed in the linked list where we need to do the same thing.

2) wake_up() only wakes one waiter, and our waitqueue is shared by all
   modules, so we need to wake them all.

3) wake_up_all() doesn't imply a memory barrier: I feel happier calling
   it after we've grabbed and dropped the module_mutex, not just after
   the state assignment.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 kernel/module.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2959,7 +2959,7 @@ static struct module *load_module(void _
 	/* Unlink carefully: kallsyms could be walking list. */
 	list_del_rcu(&mod->list);
 	module_bug_cleanup(mod);
-
+	wake_up_all(&module_wq);
  ddebug:
 	dynamic_debug_remove(info.debug);
  unlock:
@@ -3034,7 +3034,7 @@ SYSCALL_DEFINE3(init_module, void __user
 		blocking_notifier_call_chain(&module_notify_list,
 					     MODULE_STATE_GOING, mod);
 		free_module(mod);
-		wake_up(&module_wq);
+		wake_up_all(&module_wq);
 		return ret;
 	}
 	if (ret > 0) {
@@ -3046,9 +3046,8 @@ SYSCALL_DEFINE3(init_module, void __user
 		dump_stack();
 	}
 
-	/* Now it's a first class citizen!  Wake up anyone waiting for it. */
+	/* Now it's a first class citizen! */
 	mod->state = MODULE_STATE_LIVE;
-	wake_up(&module_wq);
 	blocking_notifier_call_chain(&module_notify_list,
 				     MODULE_STATE_LIVE, mod);
 
@@ -3071,6 +3070,7 @@ SYSCALL_DEFINE3(init_module, void __user
 	mod->init_ro_size = 0;
 	mod->init_text_size = 0;
 	mutex_unlock(&module_mutex);
+	wake_up_all(&module_wq);
 
 	return 0;
 }

[PATCH 2/2] module: wait when loading a module which is currently initializing.

[Rusty Russell]

The original module-init-tools module loader used a fnctl lock on the
.ko file to avoid attempts to simultaneously load a module.
Unfortunately, you can't get an exclusive fcntl lock on a read-only
fd, making this not work for read-only mounted filesystems.
module-init-tools has a hacky sleep-and-loop for this now.

It's not that hard to wait in the kernel, and only return -EEXIST once
the first module has finished loading (or continue loading the module
if the first one failed to initialize for some reason).  It's also
consistent with what we do for dependent modules which are still loading.

Suggested-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 kernel/module.c |   28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2845,6 +2845,20 @@ static int post_relocation(struct module
 	return module_finalize(info->hdr, info->sechdrs, mod);
 }
 
+/* Is this module of this name done loading?  No locks held. */
+static bool finished_loading(const char *name)
+{
+	struct module *mod;
+	bool ret;
+
+	mutex_lock(&module_mutex);
+	mod = find_module(name);
+	ret = !mod || mod->state != MODULE_STATE_COMING;
+	mutex_unlock(&module_mutex);
+
+	return ret;
+}
+
 /* Allocate and load the module: note that size of section 0 is always
    zero, and we rely on this for optional sections. */
 static struct module *load_module(void __user *umod,
@@ -2852,7 +2866,7 @@ static struct module *load_module(void _
 				  const char __user *uargs)
 {
 	struct load_info info = { NULL, };
-	struct module *mod;
+	struct module *mod, *old;
 	long err;
 
 	pr_debug("load_module: umod=%p, len=%lu, uargs=%p\n",
@@ -2918,8 +2932,18 @@ static struct module *load_module(void _
 	 * function to insert in a way safe to concurrent readers.
 	 * The mutex protects against concurrent writers.
 	 */
+again:
 	mutex_lock(&module_mutex);
-	if (find_module(mod->name)) {
+	if ((old = find_module(mod->name)) != NULL) {
+		if (old->state == MODULE_STATE_COMING) {
+			/* Wait in case it fails to load. */
+			mutex_unlock(&module_mutex);
+			err = wait_event_interruptible(module_wq,
+					       finished_loading(mod->name));
+			if (err)
+				goto free_arch_cleanup;
+			goto again;
+		}
 		err = -EEXIST;
 		goto unlock;
 	}

Re: [PATCH 2/2] module: wait when loading a module which is currently initializing.

[Lucas De Marchi]

Hi Rusty,

On Fri, Sep 14, 2012 at 4:11 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> The original module-init-tools module loader used a fnctl lock on the
> .ko file to avoid attempts to simultaneously load a module.
> Unfortunately, you can't get an exclusive fcntl lock on a read-only
> fd, making this not work for read-only mounted filesystems.
> module-init-tools has a hacky sleep-and-loop for this now.
>
> It's not that hard to wait in the kernel, and only return -EEXIST once
> the first module has finished loading (or continue loading the module
> if the first one failed to initialize for some reason).  It's also
> consistent with what we do for dependent modules which are still loading.
>
> Suggested-by: Lucas De Marchi <lucas.demarchi@profusion.mobi>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  kernel/module.c |   28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2845,6 +2845,20 @@ static int post_relocation(struct module
>         return module_finalize(info->hdr, info->sechdrs, mod);
>  }
>
> +/* Is this module of this name done loading?  No locks held. */
> +static bool finished_loading(const char *name)
> +{
> +       struct module *mod;
> +       bool ret;
> +
> +       mutex_lock(&module_mutex);
> +       mod = find_module(name);
> +       ret = !mod || mod->state != MODULE_STATE_COMING;
> +       mutex_unlock(&module_mutex);
> +
> +       return ret;
> +}

Much cleaner than we had before :-)

> +
>  /* Allocate and load the module: note that size of section 0 is always
>     zero, and we rely on this for optional sections. */
>  static struct module *load_module(void __user *umod,
> @@ -2852,7 +2866,7 @@ static struct module *load_module(void _
>                                   const char __user *uargs)
>  {
>         struct load_info info = { NULL, };
> -       struct module *mod;
> +       struct module *mod, *old;
>         long err;
>
>         pr_debug("load_module: umod=%p, len=%lu, uargs=%p\n",
> @@ -2918,8 +2932,18 @@ static struct module *load_module(void _
>          * function to insert in a way safe to concurrent readers.
>          * The mutex protects against concurrent writers.
>          */
> +again:
>         mutex_lock(&module_mutex);
> -       if (find_module(mod->name)) {
> +       if ((old = find_module(mod->name)) != NULL) {
> +               if (old->state == MODULE_STATE_COMING) {
> +                       /* Wait in case it fails to load. */
> +                       mutex_unlock(&module_mutex);
> +                       err = wait_event_interruptible(module_wq,
> +                                              finished_loading(mod->name));
> +                       if (err)
> +                               goto free_arch_cleanup;
> +                       goto again;

I wonder if we should indeed retry in case the module failed to load
or if we should just skip straight to returning the error code. We
don't have the return code for the failed load, but maybe we can
fabricate one here.

Thoughts?

cheers,
Lucas De Marchi

Re: [PATCH 2/2] module: wait when loading a module which is currently initializing.

[Rusty Russell]

Lucas De Marchi <lucas.demarchi@profusion.mobi> writes:
>> -       if (find_module(mod->name)) {
>> +       if ((old = find_module(mod->name)) != NULL) {
>> +               if (old->state == MODULE_STATE_COMING) {
>> +                       /* Wait in case it fails to load. */
>> +                       mutex_unlock(&module_mutex);
>> +                       err = wait_event_interruptible(module_wq,
>> +                                              finished_loading(mod->name));
>> +                       if (err)
>> +                               goto free_arch_cleanup;
>> +                       goto again;
>
> I wonder if we should indeed retry in case the module failed to load
> or if we should just skip straight to returning the error code. We
> don't have the return code for the failed load, but maybe we can
> fabricate one here.
>
> Thoughts?

Could have different cmdline parameters, or other randomness like
out-of-memory.  I think this is safest.

Cheers,
Rusty.

Re: [PATCH 2/2] module: wait when loading a module which is currently initializing.

[Lucas De Marchi]

On Mon, Sep 17, 2012 at 1:36 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Lucas De Marchi <lucas.demarchi@profusion.mobi> writes:
>>> -       if (find_module(mod->name)) {
>>> +       if ((old = find_module(mod->name)) != NULL) {
>>> +               if (old->state == MODULE_STATE_COMING) {
>>> +                       /* Wait in case it fails to load. */
>>> +                       mutex_unlock(&module_mutex);
>>> +                       err = wait_event_interruptible(module_wq,
>>> +                                              finished_loading(mod->name));
>>> +                       if (err)
>>> +                               goto free_arch_cleanup;
>>> +                       goto again;
>>
>> I wonder if we should indeed retry in case the module failed to load
>> or if we should just skip straight to returning the error code. We
>> don't have the return code for the failed load, but maybe we can
>> fabricate one here.
>>
>> Thoughts?
>
> Could have different cmdline parameters, or other randomness like
> out-of-memory.  I think this is safest.

makes sense. Ack.

Lucas De Marchi

module: test code for waiting.

[Rusty Russell]

From: Rusty Russell <rusty@rustcorp.com.au>
Subject: module: dummy module to test loading race.
---
 kernel/Makefile   |    1 +
 kernel/test-mod.c |   21 +++++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/kernel/Makefile b/kernel/Makefile
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -131,3 +131,4 @@ quiet_cmd_timeconst  = TIMEC   $@
 targets += timeconst.h
 $(obj)/timeconst.h: $(src)/timeconst.pl FORCE
 	$(call if_changed,timeconst)
+obj-m += test-mod.o
diff --git a/kernel/test-mod.c b/kernel/test-mod.c
new file mode 100644
--- /dev/null
+++ b/kernel/test-mod.c
@@ -0,0 +1,21 @@
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/delay.h>
+
+static bool fail;
+module_param(fail, bool, 0644);
+
+int init(void)
+{
+	printk("Module %p init start...\n", THIS_MODULE);
+	ssleep(10);
+	printk("...%p init %s\n", THIS_MODULE, fail ? "fail" : "succeed");
+	return fail ? -EINVAL : 0;
+}
+
+void fini(void)
+{
+}
+
+module_init(init);
+module_exit(fini);

Re: [PATCH 1/2] module: fix symbol waiting when module fails before init

[Lucas De Marchi]

On Fri, Sep 14, 2012 at 4:09 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> We use resolve_symbol_wait(), which blocks if the module containing
> the symbol is still loading.  However:
>
> 1) The module_wq we use is only woken after calling the modules' init
>    function, but there are other failure paths after the module is
>    placed in the linked list where we need to do the same thing.
>
> 2) wake_up() only wakes one waiter, and our waitqueue is shared by all
>    modules, so we need to wake them all.
>
> 3) wake_up_all() doesn't imply a memory barrier: I feel happier calling
>    it after we've grabbed and dropped the module_mutex, not just after
>    the state assignment.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  kernel/module.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2959,7 +2959,7 @@ static struct module *load_module(void _
>         /* Unlink carefully: kallsyms could be walking list. */
>         list_del_rcu(&mod->list);
>         module_bug_cleanup(mod);
> -
> +       wake_up_all(&module_wq);
>   ddebug:
>         dynamic_debug_remove(info.debug);
>   unlock:
> @@ -3034,7 +3034,7 @@ SYSCALL_DEFINE3(init_module, void __user
>                 blocking_notifier_call_chain(&module_notify_list,
>                                              MODULE_STATE_GOING, mod);
>                 free_module(mod);
> -               wake_up(&module_wq);
> +               wake_up_all(&module_wq);
>                 return ret;
>         }
>         if (ret > 0) {
> @@ -3046,9 +3046,8 @@ SYSCALL_DEFINE3(init_module, void __user
>                 dump_stack();
>         }
>
> -       /* Now it's a first class citizen!  Wake up anyone waiting for it. */
> +       /* Now it's a first class citizen! */
>         mod->state = MODULE_STATE_LIVE;
> -       wake_up(&module_wq);
>         blocking_notifier_call_chain(&module_notify_list,
>                                      MODULE_STATE_LIVE, mod);
>
> @@ -3071,6 +3070,7 @@ SYSCALL_DEFINE3(init_module, void __user
>         mod->init_ro_size = 0;
>         mod->init_text_size = 0;
>         mutex_unlock(&module_mutex);
> +       wake_up_all(&module_wq);
>
>         return 0;
>  }

Ack.

cheers,
Lucas De Marchi