[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20170106215354.GB8640@packer-debian-8-amd64.digitalocean.com>
Date: Fri, 6 Jan 2017 16:53:54 -0500
From: Jessica Yu <jeyu@...hat.com>
To: "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc: Rusty Russell <rusty@...tcorp.com.au>,
Filipe Manana <fdmanana@...e.com>,
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
rgoldwyn@...e.com, hare <hare@...e.com>,
Jonathan Corbet <corbet@....net>,
Linus Torvalds <torvalds@...ux-foundation.org>,
linux-kselftest@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>,
Dan Williams <dan.j.williams@...el.com>,
Aaron Tomlin <atomlin@...hat.com>, rwright@....com,
Heinrich Schuchardt <xypron.glpk@....de>,
Michal Marek <mmarek@...e.com>, martin.wilck@...e.com,
Jeff Mahoney <jeffm@...e.com>, Ingo Molnar <mingo@...hat.com>,
Petr Mladek <pmladek@...e.com>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Guenter Roeck <linux@...ck-us.net>,
"Eric W. Biederman" <ebiederm@...ssion.com>, shuah@...nel.org,
DSterba@...e.com, Kees Cook <keescook@...omium.org>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Arnaldo Carvalho de Melo <acme@...hat.com>,
Miroslav Benes <mbenes@...e.cz>, NeilBrown <neilb@...e.com>,
"linux-kernel@...r.kernel.o rg" <linux-kernel@...r.kernel.org>,
David Miller <davem@...emloft.net>,
Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>,
Julia Lawall <julia.lawall@...6.fr>
Subject: Re: kmod: add a sanity check on module loading
+++ Luis R. Rodriguez [06/01/17 21:36 +0100]:
>On Tue, Jan 03, 2017 at 10:34:53AM +1030, Rusty Russell wrote:
>> "Luis R. Rodriguez" <mcgrof@...nel.org> writes:
>> > Right, out of ~350 request_module() calls (not included try requests)
>> > only ~46 check the return value. Hence a validation check, and come to
>> > think of it, *this* was the issue that originally had me believing
>> > that in some places we might end up in a null deref --if those open
>> > coded request_module() calls assume the driver is loaded there could
>> > be many places where a NULL is inevitable.
>>
>> Yes, assuming success == module loade is simply a bug. I wrote
>> try_then_request_module() to attempt to encapsulate the correct logic
>> into a single place; maybe we need other helpers to cover (most of?) the
>> remaining cases?
>
>I see...
>
>OK so indeed we have a few possible changes to kernel given the above:
>
>a) Add SmPL rule to nag about incorrect uses of request_module() which
> never check for the return value, and fix 86% of calls (304 call sites)
> which are buggy
>
>b) Add a new API call, perhaps request_module_assert() which would
> BUG_ON() if the requested module didn't load, and change the callers
> which do not check for the return value to this.
It is probably not a good idea to panic/BUG() because a requested
module didn't load. IMO callers should already be accounting for the
fact that request_module() doesn't provide these guarantees. I haven't
looked yet to see if the majority of these callers actually do the the
responsible thing, though.
>Make request_module() do the assert and changing all proper callers of
>request_module() to a new API call which *does* let you check for the
>return value is another option but tasteless.
>
>b) seems to be what you allude to, and while it may seem also of bad taste,
>in practice it may be hard to get callers to properly check for the return
>value. I actually just favor a) even though its more work.
>
>> > Granted, I agree they
>> > should be fixed, we could add a grammar rule to start nagging at
>> > driver developers for started, but it does beg the question also of
>> > what a tightly knit validation for modprobe might look like, and hence
>> > this patch and now the completed not-yet-posted alias work.
>>
>> I really think aliases-in-kernel is too heavy a hammer, but a warning
>> when modprobe "succeeds" and the module still isn't found would be
>> a Good Thing.
>
>OK -- such a warning can really only happen if we had alias support though.
>So one option is to add this and alias parsing support as a debug option.
Hm, I see what you're saying..
To clarify the problem (if anyone was confused, as I was..): we can
verify a module is loaded by using find_module_all() and looking at
its state. However, find_module_all() operates on real module names,
and we can't verify a module has successfully loaded if all we have is
the name of the alias (eg, "fs-*" aliases in get_fs_type), because we
have no alias->real_module_name mappings in the kernel.
However, in Rusty's sample get_fs_type WARN() code, we indirectly
validated request_module()'s work by verifying that the
file_system_type has actually registered, which is what should happen
if a filesystem module successfully loads. So in this case, the caller
(get_fs_type) indirectly checks if the service it requested is now
available, which is what I *thought* callers were supposed to do in
the first place (and we didn't need the help of aliases to do that).
I think the main question we have to answer is, should the burden of
validation be on the callers, or on request_module? I am currently
leaning towards the former, but I'm still thinking.
>> > Would it be worthy as a kconfig kmod debugging aide for now? I can
>> > follow up with a semantic patch to nag about checking the return value
>> > of request_module(), and we can have 0-day then also complain about
>> > new invalid uses.
>>
>> Yeah, a warning about this would be win for sure.
>
>OK will work on such SmPL patch into the next patch series for this patch set.
>
>> BTW, I wrote the original "check-for-module-before-loading" in
>> module-init-tools, but I'm starting to wonder if it was a premature
>> optimization. Have you thought about simply removing it and always
>> trying to load the module? If it doesn't slow things down, perhaps
>> simplicity FTW?
>
>I've given this some thought as I tried to blow up request_module() with
>the new kmod stress test driver and given the small changes I made -- I'm of the
>mind set it should be based on numbers: if a change improves the time it takes
>to load modules while also not regressing all the other test cases then we
>should go with it. The only issue is we don't yet have enough test cases
>to cover the typical distribution setup: load tons of modules, and only
>sometimes try to load a few of the same modules.
>
>The early module-init-tools check seems fair gain to me given a bounce back to
>the kernel and back to userspace should incur a bit more work than just checking
>for a few files on the filesystem. As I noted though, I can't prove this for most
>cases for now, but its a hunch.
>
>So I'd advocate leaving the "check-for-module-before-loading" on kmod for now.
>
> Luis
Powered by blists - more mailing lists