+++ 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@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
