[patch] module: potential deadlock in error path

[patch] module: potential deadlock in error path

[Dan Carpenter]

We take the lock twice if we hit this goto.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/kernel/module.c b/kernel/module.c
index d25e359..2eefa7d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3239,8 +3239,10 @@ again:
 	mutex_lock(&module_mutex);
 	/* Find duplicate symbols (must be called under lock). */
 	err = verify_export_symbols(mod);
-	if (err < 0)
+	if (err < 0) {
+		mutex_unlock(&module_mutex);
 		goto ddebug_cleanup;
+	}
 
 	/* This relies on module_mutex for list integrity. */
 	module_bug_finalize(info->hdr, info->sechdrs, mod);

Re: [patch] module: potential deadlock in error path

[Rusty Russell]

Dan Carpenter <dan.carpenter@oracle.com> writes:
> We take the lock twice if we hit this goto.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Damn, just pushed that to Linus: should have read mail first.

I've added this, thanks.

The following changes since commit a7f2a366f62319dfebf8d4dfe8b211f631c78457:

  ima: fallback to MODULE_SIG_ENFORCE for existing kernel module syscall (2012-12-24 09:35:48 -0500)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux.git tags/fixes-for-linus

for you to fetch changes up to 8f82d479da05bd7df8cca3a5fbc0273f73b601c0:

  module: potential deadlock in error path (2013-01-21 11:29:33 +1030)

----------------------------------------------------------------
Various minor fixes, but a slightly more complex one to fix the per-cpu overload
problem introduced recently by kvm id changes.

(Now with an error path fix from Dan, thanks)

----------------------------------------------------------------
Alexander Graf (1):
      virtio-blk: Don't free ida when disk is in use

Dan Carpenter (1):
      module: potential deadlock in error path

Rusty Russell (2):
      module: add new state MODULE_STATE_UNFORMED.
      module: put modules in list much earlier.

Sasha Levin (1):
      module: prevent warning when finit_module a 0 sized file

 drivers/block/virtio_blk.c  |    7 +-
 include/linux/module.h      |   10 +--
 kernel/debug/kdb/kdb_main.c |    2 +
 kernel/module.c             |  156 ++++++++++++++++++++++++++++++-------------
 lib/bug.c                   |    1 +
 5 files changed, 124 insertions(+), 52 deletions(-)

Re: [patch] module: potential deadlock in error path

[Linus Torvalds]

On Sun, Jan 20, 2013 at 5:20 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Dan Carpenter <dan.carpenter@oracle.com> writes:
>> We take the lock twice if we hit this goto.
>>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Damn, just pushed that to Linus: should have read mail first.
>
> I've added this, thanks.

I'm not pulling this. It seems stupid.

Why isn't the fix just this (whitespace-damaged, cut-and-pasted)
one-liner instead? I may be blind, but as far as I cal tell, there's
exactly one single place we do that "giti ddebug_cleanup", and it
wants to unlock the mutex, so we should just move the unlock down one
line instead.

Hmm? Is there some hidden magic going on that I can't see?

                  Linus

---
diff --git a/kernel/module.c b/kernel/module.c
index d25e359279ae..eab08274ec9b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3274,8 +3274,8 @@ again:
        /* module_bug_cleanup needs module_mutex protection */
        mutex_lock(&module_mutex);
        module_bug_cleanup(mod);
-       mutex_unlock(&module_mutex);
  ddebug_cleanup:
+       mutex_unlock(&module_mutex);
        dynamic_debug_remove(info->debug);
        synchronize_sched();
        kfree(mod->args);

Re: [patch] module: potential deadlock in error path

[Rusty Russell]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sun, Jan 20, 2013 at 5:20 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Dan Carpenter <dan.carpenter@oracle.com> writes:
>>> We take the lock twice if we hit this goto.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> Damn, just pushed that to Linus: should have read mail first.
>>
>> I've added this, thanks.
>
> I'm not pulling this. It seems stupid.
>
> Why isn't the fix just this (whitespace-damaged, cut-and-pasted)
> one-liner instead? I may be blind, but as far as I cal tell, there's
> exactly one single place we do that "giti ddebug_cleanup", and it
> wants to unlock the mutex, so we should just move the unlock down one
> line instead.
>
> Hmm? Is there some hidden magic going on that I can't see?

TBH, I find your change marginally less clear.

You've now conflated two completely different lock paths into a single
unlock.  mutex_bug_cleanup() should really lock internally, but doesn't
so we wrap it.  And that mutex_unlock of yours has nothing to do with
cleaning up ddebug, so the labels misnamed, at best.

> diff --git a/kernel/module.c b/kernel/module.c
> index d25e359279ae..eab08274ec9b 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3274,8 +3274,8 @@ again:
>         /* module_bug_cleanup needs module_mutex protection */
>         mutex_lock(&module_mutex);
>         module_bug_cleanup(mod);
> -       mutex_unlock(&module_mutex);
>   ddebug_cleanup:
> +       mutex_unlock(&module_mutex);
>         dynamic_debug_remove(info->debug);
>         synchronize_sched();
>         kfree(mod->args);

Not that it matters much: this is going to change for next merge window.
See below for freshly-minted patch (compiled, untested).

Nice to make module_bug_cleanup() lock internally but it's in bug.c,
and I've avoided making the module mutex non-static due to a history of
abuse...

Thanks,
Rusty.

module: clean up load_module a little more.

1fb9341ac34825aa40354e74d9a2c69df7d2c304 made our locking in
load_module more complicated: we grab the mutex once to insert the
module in the list, then again to upgrade it once it's formed.

Since the locking is self-contained, it's neater to do this in
separate functions.

diff --git a/kernel/module.c b/kernel/module.c
index 2b1d517..c0bc9b9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3145,12 +3145,72 @@ static int may_init_module(void)
 	return 0;
 }
 
+/*
+ * We try to place it in the list now to make sure it's unique before
+ * we dedicate too many resources.  In particular, temporary percpu
+ * memory exhaustion.
+ */
+static int add_unformed_module(struct module *mod)
+{
+	int err;
+	struct module *old;
+
+	mod->state = MODULE_STATE_UNFORMED;
+
+again:
+	mutex_lock(&module_mutex);
+	if ((old = find_module_all(mod->name, true)) != NULL) {
+		if (old->state == MODULE_STATE_COMING
+		    || old->state == MODULE_STATE_UNFORMED) {
+			/* Wait in case it fails to load. */
+			mutex_unlock(&module_mutex);
+			err = wait_event_interruptible(module_wq,
+					       finished_loading(mod->name));
+			if (err)
+				goto out_unlocked;
+			goto again;
+		}
+		err = -EEXIST;
+		goto out;
+	}
+	list_add_rcu(&mod->list, &modules);
+	err = 0;
+
+out:
+	mutex_unlock(&module_mutex);
+out_unlocked:
+	return err;
+}
+
+static int complete_formation(struct module *mod, struct load_info *info)
+{
+	int err;
+
+	mutex_lock(&module_mutex);
+
+	/* Find duplicate symbols (must be called under lock). */
+	err = verify_export_symbols(mod);
+	if (err < 0)
+		goto out;
+
+	/* This relies on module_mutex for list integrity. */
+	module_bug_finalize(info->hdr, info->sechdrs, mod);
+
+	/* Mark state as coming so strong_try_module_get() ignores us,
+	 * but kallsyms etc. can see us. */
+	mod->state = MODULE_STATE_COMING;
+
+out:
+	mutex_unlock(&module_mutex);
+	return err;
+}
+
 /* Allocate and load the module: note that size of section 0 is always
    zero, and we rely on this for optional sections. */
 static int load_module(struct load_info *info, const char __user *uargs,
 		       int flags)
 {
-	struct module *mod, *old;
+	struct module *mod;
 	long err;
 
 	err = module_sig_check(info);
@@ -3168,31 +3228,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
 		goto free_copy;
 	}
 
-	/*
-	 * We try to place it in the list now to make sure it's unique
-	 * before we dedicate too many resources.  In particular,
-	 * temporary percpu memory exhaustion.
-	 */
-	mod->state = MODULE_STATE_UNFORMED;
-again:
-	mutex_lock(&module_mutex);
-	if ((old = find_module_all(mod->name, true)) != NULL) {
-		if (old->state == MODULE_STATE_COMING
-		    || old->state == MODULE_STATE_UNFORMED) {
-			/* 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_module;
-			goto again;
-		}
-		err = -EEXIST;
-		mutex_unlock(&module_mutex);
+	/* Reserve our place in the list. */
+	err = add_unformed_module(mod);
+	if (err)
 		goto free_module;
-	}
-	list_add_rcu(&mod->list, &modules);
-	mutex_unlock(&module_mutex);
 
 #ifdef CONFIG_MODULE_SIG
 	mod->sig_ok = info->sig_ok;
@@ -3245,22 +3284,10 @@ again:
 
 	dynamic_debug_setup(info->debug, info->num_debug);
 
-	mutex_lock(&module_mutex);
-	/* Find duplicate symbols (must be called under lock). */
-	err = verify_export_symbols(mod);
-	if (err < 0) {
-		mutex_unlock(&module_mutex);
+	/* Finally it's fully formed, ready to start executing. */
+	err = complete_formation(mod, info);
+	if (err)
 		goto ddebug_cleanup;
-	}
-
-	/* This relies on module_mutex for list integrity. */
-	module_bug_finalize(info->hdr, info->sechdrs, mod);
-
-	/* Mark state as coming so strong_try_module_get() ignores us,
-	 * but kallsyms etc. can see us. */
-	mod->state = MODULE_STATE_COMING;
-
-	mutex_unlock(&module_mutex);
 
 	/* Module is ready to execute: parsing args may do that. */
 	err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,

Re: [patch] module: potential deadlock in error path

[Linus Torvalds]

On Sun, Jan 20, 2013 at 7:52 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>
> You've now conflated two completely different lock paths into a single
> unlock.

We have that elsewhere too. And it's what we used to have before too.

So the simple fact is that commit 1fb9341ac348 just introduced this
bug, and moving the goto target around is the obvious fix for it, and
makes it match the old code that was simply incorrectly modified.

The suggested patch instead has *some* cleanup inside the
if-statement, and some at the goto target. That makes no sense to
humans, and just makes it harder for the compiler to generate better
code.

> mutex_bug_cleanup() should really lock internally, but doesn't
> so we wrap it.  And that mutex_unlock of yours has nothing to do with
> cleaning up ddebug, so the labels misnamed, at best.

Bah, humbug. It's called "ddebug_cleanup" because it's called after
the  debug setup, so it needs to clean up the state set up by that.
The fact that it needs to unlock is secondary, and is simply because
the lock is taken at that point, so needs to be released. The naming
is not wonderful, but it's not hugely illogical, and again, that's
what it used to (except "ddebug" has been renamed to
"ddebug_cleanup"). You could rename it if you want to (we used to have
a target called "unlock" at that point), but that's *still* no excuse
for just creating code that does cleanup in two totally unrelated
places.

> Not that it matters much: this is going to change for next merge window.

Now, agreed, that looks better, although I suspect you could have
taken the "split that ugly function up" further still.

                   Linus

Re: [patch] module: potential deadlock in error path

[Dan Carpenter]

Rusty, you have this right?  I like Linus's version.  Could you give
me a Reported-by tag?

regards,
dan carpenter