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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMOw1v6Qy1wHhRcPQkyhDe7LrGaM_qRdhufr8YtHnfp5pTPFAA@mail.gmail.com>
Date:	Fri, 14 Sep 2012 13:37:10 -0300
From:	Lucas De Marchi <lucas.demarchi@...fusion.mobi>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Jon Masters <jonathan@...masters.org>,
	linux-modules <linux-modules@...r.kernel.org>,
	Lucas De Marchi <lucas.de.marchi@...il.com>
Subject: Re: [PATCH 2/2] module: wait when loading a module which is currently initializing.

Hi Rusty,

On Fri, Sep 14, 2012 at 4:11 AM, Rusty Russell <rusty@...tcorp.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@...fusion.mobi>
> Signed-off-by: Rusty Russell <rusty@...tcorp.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/