linux-kernel - Re: [RFC 10/10] kmod: add a sanity check on module loading

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 for Android: free password hash cracker in your pocket

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

Date:   Fri, 16 Dec 2016 09:31:23 +0100
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Rusty Russell <rusty@...tcorp.com.au>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>, shuah@...nel.org,
        jeyu@...hat.com, ebiederm@...ssion.com, dmitry.torokhov@...il.com,
        acme@...hat.com, corbet@....net, martin.wilck@...e.com,
        mmarek@...e.com, pmladek@...e.com, hare@...e.com, rwright@....com,
        jeffm@...e.com, DSterba@...e.com, fdmanana@...e.com,
        neilb@...e.com, linux@...ck-us.net, rgoldwyn@...e.com,
        subashab@...eaurora.org, xypron.glpk@....de, keescook@...omium.org,
        atomlin@...hat.com, mbenes@...e.cz, paulmck@...ux.vnet.ibm.com,
        dan.j.williams@...el.com, jpoimboe@...hat.com, davem@...emloft.net,
        mingo@...hat.com, akpm@...ux-foundation.org,
        torvalds@...ux-foundation.org, linux-kselftest@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC 10/10] kmod: add a sanity check on module loading

On Thu, Dec 15, 2016 at 10:57:42AM +1030, Rusty Russell wrote:
> "Luis R. Rodriguez" <mcgrof@...nel.org> writes:
> > kmod has an optimization in place whereby if a some kernel code
> > uses request_module() on a module already loaded we never bother
> > userspace as the module already is loaded. This is not true for
> > get_fs_type() though as it uses aliases.
> 
> Well, the obvious thing to do here is block kmod if we're currently
> loading the same module.

OK thanks, I've now added this, it sure helps. Test cases 0008 and 0009 require
hammering on the test over and over to see a failure on vanilla kernels,
an upper bound I found was about 150 times each test. Running test 0008
150 times with this enhancement you mentioned shaves off ~4 seconds.
For test 0009 it shaves off ~16 seconds, but as I note below the alias support
was needed as well.

> Otherwise it has to do some weird spinning
> thing in userspace anyway.

Right, but note that the get_fs_type() tests would still fail given
module.c was not alias-aware yet. I have the patches to add support
for the aliases now though and this is part of what helped shave
off time from the tests.

> We already have module_wq for this, we just need a bit more code to
> share the return value; and there's a weird corner case there where we
> have "modprobe foo param=invalid" then "modprobe foo param=valid" and we
> fail both with -EINVAL, but it's probably not worth fixing.

Hm OK. Although the set of patches I have fix and optimize now some
of these corner cases one issue that I still didn't quite yet figure
out was that a failure propagates secondary failures. That is,
say a module fails and you have loaded 4 request for the same module,
if the first request failed the last 3 *could* also fail. You can
trigger and see this with the latest script:

http://drvbp1.linux-foundation.org/~mcgrof/2016/12/16/kmod.sh

The latest version of the test_kmod driver:

http://drvbp1.linux-foundation.org/~mcgrof/2016/12/16/test_kmod.patch

./kmod.sh -t 0008
./kmod.sh -t 0009

When either of these fail you'll on dmesg that either a few NULL or
errors were found. It may not be worth fixing this race... given
that after apply all of my patches I no longer see this at all,
but I'm pretty sure a test case can be created to replicate more
easily.

FWIW a few things did occur to me:

a) list_add_rcu() is used so new modules get added first
b) find_module_all() returns the last module which was added as it traverses
   the module list

Because of a) and b) if two modules for the same driver can be on
the list at the same time then we'll get very likely a module which
is unformed or going than a live module. Changing module addition
to use list_add_tail_rcu() should mean we typically get the first
module added to the list for the module name I think, but other
than that I could not think clearly of the root case to allowing
multiple errors.

BTW should find_module_all() use rcu to traverse?

--- a/kernel/module.c
+++ b/kernel/module.c
@@ -594,7 +594,7 @@ static struct module *find_module_all(const char *name, size_t len,

 	module_assert_mutex_or_preempt();

-	list_for_each_entry(mod, &modules, list) {
+	list_for_each_entry_rcu(mod, &modules, list) {
 		if (!even_unformed && mod->state == MODULE_STATE_UNFORMED)
 			continue;
 		if (strlen(mod->name) == len && !memcmp(mod->name, name, len))
@@ -3532,7 +3532,7 @@ static int add_unformed_module(struct module *mod)
 		goto out;
 	}
 	mod_update_bounds(mod);
-	list_add_rcu(&mod->list, &modules);
+	list_add_tail_rcu(&mod->list, &modules);
 	mod_tree_insert(mod);
 	err = 0;