All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jessica Yu <jeyu@redhat.com>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>,
	Filipe Manana <fdmanana@suse.com>,
	"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	rgoldwyn@suse.com, hare <hare@suse.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	linux-kselftest@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Dan Williams <dan.j.williams@intel.com>,
	Aaron Tomlin <atomlin@redhat.com>,
	rwright@hpe.com, Heinrich Schuchardt <xypron.glpk@gmx.de>,
	Michal Marek <mmarek@suse.com>,
	martin.wilck@suse.com, Jeff Mahoney <jeffm@suse.com>,
	Ingo Molnar <mingo@redhat.com>, Petr Mladek <pmladek@suse.com>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Guenter Roeck <linux@roeck-us.net>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	shuah@kernel.org, DSterba@suse.com,
	Kees Cook <keescook@chromium.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Arnaldo Carvalho de Melo <acme@redhat.com>,
	Miroslav Benes <mbenes@suse.cz>, NeilBrown <neilb@suse.com>,
	"linux-kernel@vger.kernel.o rg" <linux-kernel@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>,
	Julia Lawall <julia.lawall@lip6.fr>
Subject: Re: kmod: add a sanity check on module loading
Date: Fri, 6 Jan 2017 16:53:54 -0500	[thread overview]
Message-ID: <20170106215354.GB8640@packer-debian-8-amd64.digitalocean.com> (raw)
In-Reply-To: <20170106203637.GV13946@wotan.suse.de>

+++ 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@kernel.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

  reply	other threads:[~2017-01-06 21:55 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-08 18:47 [RFC 00/10] kmod: stress test driver, few fixes and enhancements Luis R. Rodriguez
2016-12-08 18:47 ` [RFC 01/10] kmod: add test driver to stress test the module loader Luis R. Rodriguez
2016-12-08 20:24   ` Kees Cook
2016-12-13 21:10     ` Luis R. Rodriguez
2016-12-16  7:41       ` Luis R. Rodriguez
2016-12-08 19:48 ` [RFC 02/10] module: fix memory leak on early load_module() failures Luis R. Rodriguez
2016-12-08 20:30   ` Kees Cook
2016-12-08 21:10     ` Luis R. Rodriguez
2016-12-08 21:17       ` Kees Cook
2016-12-09 17:06   ` Miroslav Benes
2016-12-16  8:51     ` Luis R. Rodriguez
2016-12-15 18:46   ` Aaron Tomlin
2016-12-08 19:48 ` [RFC 03/10] kmod: add dynamic max concurrent thread count Luis R. Rodriguez
2016-12-08 20:28   ` Kees Cook
2016-12-08 21:00     ` Luis R. Rodriguez
2016-12-14 15:38   ` Petr Mladek
2016-12-16  8:39     ` Luis R. Rodriguez
2017-01-10 19:24       ` Luis R. Rodriguez
2016-12-08 19:48 ` [RFC 04/10] kmod: provide wrappers for kmod_concurrent inc/dec Luis R. Rodriguez
2016-12-08 20:29   ` Kees Cook
2016-12-08 21:08     ` Luis R. Rodriguez
2016-12-15 12:46       ` Petr Mladek
2016-12-16  8:05         ` Luis R. Rodriguez
2016-12-22  4:48           ` Jessica Yu
2017-01-06 20:54             ` Luis R. Rodriguez
2017-01-10 18:57           ` [RFC 04/10] " Luis R. Rodriguez
2017-01-11 20:08             ` Luis R. Rodriguez
2017-05-16 18:02               ` Luis R. Rodriguez
2017-05-18  2:37                 ` Luis R. Rodriguez
2016-12-22  5:07   ` Jessica Yu
2017-01-10 20:28     ` Luis R. Rodriguez
2016-12-08 19:48 ` [RFC 05/10] kmod: return -EBUSY if modprobe limit is reached Luis R. Rodriguez
2016-12-08 19:48 ` [RFC 06/10] kmod: provide sanity check on kmod_concurrent access Luis R. Rodriguez
2016-12-14 16:08   ` Petr Mladek
2016-12-14 17:12     ` Luis R. Rodriguez
2016-12-15 12:57   ` Petr Mladek
2017-01-10 20:00     ` Luis R. Rodriguez
2016-12-08 19:49 ` [RFC 07/10] kmod: use simplified rate limit printk Luis R. Rodriguez
2016-12-14 16:23   ` Petr Mladek
2016-12-14 16:41     ` Joe Perches
2016-12-16  8:44     ` Luis R. Rodriguez
2016-12-08 19:49 ` [RFC 08/10] sysctl: add support for unsigned int properly Luis R. Rodriguez
2016-12-08 19:49 ` [RFC 09/10] kmod: add helpers for getting kmod count and limit Luis R. Rodriguez
2016-12-15 16:56   ` Petr Mladek
2016-12-16  7:57     ` Luis R. Rodriguez
2017-01-11 18:27       ` Luis R. Rodriguez
2016-12-08 19:49 ` [RFC 10/10] kmod: add a sanity check on module loading Luis R. Rodriguez
2016-12-09 20:03   ` Martin Wilck
2016-12-09 20:56     ` Linus Torvalds
2016-12-15 18:08       ` Luis R. Rodriguez
2016-12-15  0:27   ` Rusty Russell
2016-12-16  8:31     ` Luis R. Rodriguez
2016-12-17  3:54       ` Rusty Russell
     [not found]         ` <CAB=NE6VvuA9a6hf6yoopGfUxVJQM5HyV5bNzUdsEtUV0UhbG-g@mail.gmail.com>
2016-12-20  0:53           ` Rusty Russell
2016-12-20 18:52             ` Luis R. Rodriguez
2016-12-21  2:21               ` Rusty Russell
2016-12-21 13:08                 ` Luis R. Rodriguez
2017-01-03  0:04                   ` Rusty Russell
2017-01-06 20:36                     ` Luis R. Rodriguez
2017-01-06 21:53                       ` Jessica Yu [this message]
2017-01-09 20:27                         ` Luis R. Rodriguez
     [not found]                       ` <87bmvgax51.fsf@rustcorp.com.au>
2017-01-09 19:56                         ` [RFC 10/10] " Luis R. Rodriguez
2017-01-06 21:03                     ` Jessica Yu
2017-01-04  2:47   ` Jessica Yu
2017-01-11 19:10 ` [RFC 00/10] kmod: stress test driver, few fixes and enhancements Luis R. Rodriguez

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170106215354.GB8640@packer-debian-8-amd64.digitalocean.com \
    --to=jeyu@redhat.com \
    --cc=DSterba@suse.com \
    --cc=acme@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=atomlin@redhat.com \
    --cc=corbet@lwn.net \
    --cc=dan.j.williams@intel.com \
    --cc=davem@davemloft.net \
    --cc=dmitry.torokhov@gmail.com \
    --cc=ebiederm@xmission.com \
    --cc=fdmanana@suse.com \
    --cc=hare@suse.com \
    --cc=jeffm@suse.com \
    --cc=jpoimboe@redhat.com \
    --cc=julia.lawall@lip6.fr \
    --cc=keescook@chromium.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=martin.wilck@suse.com \
    --cc=mbenes@suse.cz \
    --cc=mcgrof@kernel.org \
    --cc=mingo@redhat.com \
    --cc=mmarek@suse.com \
    --cc=neilb@suse.com \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=pmladek@suse.com \
    --cc=rgoldwyn@suse.com \
    --cc=rusty@rustcorp.com.au \
    --cc=rwright@hpe.com \
    --cc=shuah@kernel.org \
    --cc=subashab@codeaurora.org \
    --cc=torvalds@linux-foundation.org \
    --cc=xypron.glpk@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.