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

[Luis R. Rodriguez]

On Tue, Jan 10, 2017 at 05:17:22AM +1030, Rusty Russell wrote:
> "Luis R. Rodriguez"  writes:
> > On Tue, Jan 03, 2017 at 10:34:53AM +1030, Rusty Russell wrote:
> >> "Luis R. Rodriguez"  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
> 
> Well, checking the return value is merely an optimization.  The bug
> is not re-checking for registrations and *assuming existence*.

An optimization, I see.. I was going with using the return value from
request_module() -- clearly that is not proper form. Might as well make this
void ?

> I glanced through the first 100, and they're fine.  You are supposed to
> do "request_module()" then "re-check if it's there", and that seems to
> the pattern.

OK I then understand now why you added try_then_request_module() and hinted
to more similar forms. If try_then_request_module() was capturing the
required effort properly then I will note that my grammar rule now finds
one invalid use on drivers/media/usb/as102/as102_drv.c, although its
use seems invalid though the module is loaded for firmware loading
purposes and it seems that is optional at that point in time as such
it does not seem invalid. Not sure if its worth adding a separate API
call for this to annotate its fine to ignore the return value.

One thing I am sure of at this point though is that the loose required
checks for proper form makes it pretty hard to validate the callers.

> > 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.
> >
> > 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.
> 
> No, I meant to look for patterns to see if we could create helpers.  But
> I've revised that, since I don't actually see any problems.
> 
> In fact, you've yet to identify a single problem user.

Indeed, its actually hard to verify proper "form" for this API given
how different each caller verifies the requested module is present or
loaded.

> >> 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.
> 
> Just benchmark boot time.  That's a pretty good test.

Alright.

  Luis

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

[Luis R. Rodriguez]

On Tue, Jan 10, 2017 at 05:17:22AM +1030, Rusty Russell wrote:
> "Luis R. Rodriguez"  writes:
> > On Tue, Jan 03, 2017 at 10:34:53AM +1030, Rusty Russell wrote:
> >> "Luis R. Rodriguez"  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
> 
> Well, checking the return value is merely an optimization.  The bug
> is not re-checking for registrations and *assuming existence*.

An optimization, I see.. I was going with using the return value from
request_module() -- clearly that is not proper form. Might as well make this
void ?

> I glanced through the first 100, and they're fine.  You are supposed to
> do "request_module()" then "re-check if it's there", and that seems to
> the pattern.

OK I then understand now why you added try_then_request_module() and hinted
to more similar forms. If try_then_request_module() was capturing the
required effort properly then I will note that my grammar rule now finds
one invalid use on drivers/media/usb/as102/as102_drv.c, although its
use seems invalid though the module is loaded for firmware loading
purposes and it seems that is optional at that point in time as such
it does not seem invalid. Not sure if its worth adding a separate API
call for this to annotate its fine to ignore the return value.

One thing I am sure of at this point though is that the loose required
checks for proper form makes it pretty hard to validate the callers.

> > 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.
> >
> > 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.
> 
> No, I meant to look for patterns to see if we could create helpers.  But
> I've revised that, since I don't actually see any problems.
> 
> In fact, you've yet to identify a single problem user.

Indeed, its actually hard to verify proper "form" for this API given
how different each caller verifies the requested module is present or
loaded.

> >> 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.
> 
> Just benchmark boot time.  That's a pretty good test.

Alright.

  Luis

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

[Luis R. Rodriguez]

On Tue, Jan 03, 2017 at 10:34:53AM +1030, Rusty Russell wrote:
> "Luis R. Rodriguez"  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.

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.

> > 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

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

[Luis R. Rodriguez]

On Tue, Jan 03, 2017 at 10:34:53AM +1030, Rusty Russell wrote:
> "Luis R. Rodriguez"  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.

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.

> > 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

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

[Rusty Russell]

"Luis R. Rodriguez"  writes:
>> Maybe a similar hack for try_then_request_module(), but many places seem
>> to open-code request_module() so it's not as trivial...

Hi Luis, Jessica (who is the main module maintainer now),

Back from break, sorry about delay.

> 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?

> 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.

> 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.

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?

Thanks,
Rusty.

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

[Rusty Russell]

"Luis R. Rodriguez"  writes:
>> Maybe a similar hack for try_then_request_module(), but many places seem
>> to open-code request_module() so it's not as trivial...

Hi Luis, Jessica (who is the main module maintainer now),

Back from break, sorry about delay.

> 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?

> 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.

> 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.

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?

Thanks,
Rusty.

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

[Luis R. Rodriguez]

On Tue, Dec 20, 2016 at 8:21 PM, Rusty Russell  wrote:
> "Luis R. Rodriguez"  writes:
>> OK -- if userspace messes up again it may be a bit hard to prove
>> unless we have a validation debug thing in place, would such a thing
>> in debug form be reasonable ?
>
> That makes perfect sense.  Untested hack:
>
> diff --git a/fs/filesystems.c b/fs/filesystems.c
> index c5618db110be..e5c90e80c7d3 100644
> --- a/fs/filesystems.c
> +++ b/fs/filesystems.c
> @@ -275,9 +275,10 @@ struct file_system_type *get_fs_type(const char *name)
> int len = dot ? dot - name : strlen(name);
>
> fs = __get_fs_type(name, len);
> -   if (!fs && (request_module("fs-%.*s", len, name) == 0))
> +   if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
> fs = __get_fs_type(name, len);
> -
> +   WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still 
> no fs?\n", len, name);
> +   }
> if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
> put_filesystem(fs);
> fs = NULL;

This is precisely a type of debug patch we had added first to verify "WTF".

> Maybe a similar hack for try_then_request_module(), but many places seem
> to open-code request_module() so it's not as trivial...

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. 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.

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.

  Luis

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

[Luis R. Rodriguez]

On Tue, Dec 20, 2016 at 8:21 PM, Rusty Russell  wrote:
> "Luis R. Rodriguez"  writes:
>> OK -- if userspace messes up again it may be a bit hard to prove
>> unless we have a validation debug thing in place, would such a thing
>> in debug form be reasonable ?
>
> That makes perfect sense.  Untested hack:
>
> diff --git a/fs/filesystems.c b/fs/filesystems.c
> index c5618db110be..e5c90e80c7d3 100644
> --- a/fs/filesystems.c
> +++ b/fs/filesystems.c
> @@ -275,9 +275,10 @@ struct file_system_type *get_fs_type(const char *name)
> int len = dot ? dot - name : strlen(name);
>
> fs = __get_fs_type(name, len);
> -   if (!fs && (request_module("fs-%.*s", len, name) == 0))
> +   if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
> fs = __get_fs_type(name, len);
> -
> +   WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still 
> no fs?\n", len, name);
> +   }
> if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
> put_filesystem(fs);
> fs = NULL;

This is precisely a type of debug patch we had added first to verify "WTF".

> Maybe a similar hack for try_then_request_module(), but many places seem
> to open-code request_module() so it's not as trivial...

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. 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.

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.

  Luis

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

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> OK -- if userspace messes up again it may be a bit hard to prove
> unless we have a validation debug thing in place, would such a thing
> in debug form be reasonable ?

That makes perfect sense.  Untested hack:

diff --git a/fs/filesystems.c b/fs/filesystems.c
index c5618db110be..e5c90e80c7d3 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -275,9 +275,10 @@ struct file_system_type *get_fs_type(const char *name)
int len = dot ? dot - name : strlen(name);
 
fs = __get_fs_type(name, len);
-   if (!fs && (request_module("fs-%.*s", len, name) == 0))
+   if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
fs = __get_fs_type(name, len);
-
+   WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no 
fs?\n", len, name);
+   }
if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
put_filesystem(fs);
fs = NULL;

Maybe a similar hack for try_then_request_module(), but many places seem
to open-code request_module() so it's not as trivial...

Cheers,
Rusty.

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

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> OK -- if userspace messes up again it may be a bit hard to prove
> unless we have a validation debug thing in place, would such a thing
> in debug form be reasonable ?

That makes perfect sense.  Untested hack:

diff --git a/fs/filesystems.c b/fs/filesystems.c
index c5618db110be..e5c90e80c7d3 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -275,9 +275,10 @@ struct file_system_type *get_fs_type(const char *name)
int len = dot ? dot - name : strlen(name);
 
fs = __get_fs_type(name, len);
-   if (!fs && (request_module("fs-%.*s", len, name) == 0))
+   if (!fs && (request_module("fs-%.*s", len, name) == 0)) {
fs = __get_fs_type(name, len);
-
+   WARN_ONCE(!fs, "request_module fs-%.*s succeeded, but still no 
fs?\n", len, name);
+   }
if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
put_filesystem(fs);
fs = NULL;

Maybe a similar hack for try_then_request_module(), but many places seem
to open-code request_module() so it's not as trivial...

Cheers,
Rusty.

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

[Luis R. Rodriguez]

On Mon, Dec 19, 2016 at 6:53 PM, Rusty Russell  wrote:
> Where does this NULL-deref is the module isn't correctly loaded?

No you are right, sorry -- I had confused a failure to mount over null
deref, my mistake.

>> *Iff* we want a sanity check to verify kmod's umh is not lying to us we
>> need to verify after 0 was returned that it was not lying to us. Since kmod
>> accepts aliases but find_modules_all() only works on the real module name a
>> validation check cannot happen when all you have are aliases.
>
> request_module() should block until resolution, but that's fundamentally
> a userspace problem.  Let's not paper over it in kernelspace.

OK -- if userspace messes up again it may be a bit hard to prove
unless we have a validation debug thing in place, would such a thing
in debug form be reasonable ?

>> Yes; the kallsyms code does this on Oops.  Not really a big issue in
>> practice, but a nice fix.
>>
>> Ok, will bundle into my queue.
>
> Please submit to Jessica for her module queue, as it's orthogonal
> AFAICT.

Will do.

>> I will note though that I still think there's a bug in this code --
>> upon a failure other "spinning" requests can fail, I believe this may
>> be due to not having another state or informing pending modules too
>> early of a failure but I haven't been able to prove this conjecture
>> yet.
>
> That's possible, but I can't see it from quickly re-checking the code.
>
> The module should be fully usable at this point; the module's init has
> been called successfully, so in the case of __get_fs_type() it should
> now succeed.  The module cleans up its init section, but that should be
> independent.
>
> If there is a race, it's likely to be when some other caller wakes the
> queue.  Moving the wakeup as soon as possible should make it easier to
> trigger:
>
> diff --git a/kernel/module.c b/kernel/module.c
> index f57dd63186e6..78bd89d41a22 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3397,6 +3397,7 @@ static noinline int do_init_module(struct module *mod)
>
> /* Now it's a first class citizen! */
> mod->state = MODULE_STATE_LIVE;
> +   wake_up_all(_wq);
> blocking_notifier_call_chain(_notify_list,
>  MODULE_STATE_LIVE, mod);
>
> @@ -3445,7 +3446,6 @@ static noinline int do_init_module(struct module *mod)
>  */
> call_rcu_sched(>rcu, do_free_init);
> mutex_unlock(_mutex);
> -   wake_up_all(_wq);
>
> return 0;
>

Will give this a shot, thanks!

  Luis

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

[Luis R. Rodriguez]

On Mon, Dec 19, 2016 at 6:53 PM, Rusty Russell  wrote:
> Where does this NULL-deref is the module isn't correctly loaded?

No you are right, sorry -- I had confused a failure to mount over null
deref, my mistake.

>> *Iff* we want a sanity check to verify kmod's umh is not lying to us we
>> need to verify after 0 was returned that it was not lying to us. Since kmod
>> accepts aliases but find_modules_all() only works on the real module name a
>> validation check cannot happen when all you have are aliases.
>
> request_module() should block until resolution, but that's fundamentally
> a userspace problem.  Let's not paper over it in kernelspace.

OK -- if userspace messes up again it may be a bit hard to prove
unless we have a validation debug thing in place, would such a thing
in debug form be reasonable ?

>> Yes; the kallsyms code does this on Oops.  Not really a big issue in
>> practice, but a nice fix.
>>
>> Ok, will bundle into my queue.
>
> Please submit to Jessica for her module queue, as it's orthogonal
> AFAICT.

Will do.

>> I will note though that I still think there's a bug in this code --
>> upon a failure other "spinning" requests can fail, I believe this may
>> be due to not having another state or informing pending modules too
>> early of a failure but I haven't been able to prove this conjecture
>> yet.
>
> That's possible, but I can't see it from quickly re-checking the code.
>
> The module should be fully usable at this point; the module's init has
> been called successfully, so in the case of __get_fs_type() it should
> now succeed.  The module cleans up its init section, but that should be
> independent.
>
> If there is a race, it's likely to be when some other caller wakes the
> queue.  Moving the wakeup as soon as possible should make it easier to
> trigger:
>
> diff --git a/kernel/module.c b/kernel/module.c
> index f57dd63186e6..78bd89d41a22 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3397,6 +3397,7 @@ static noinline int do_init_module(struct module *mod)
>
> /* Now it's a first class citizen! */
> mod->state = MODULE_STATE_LIVE;
> +   wake_up_all(_wq);
> blocking_notifier_call_chain(_notify_list,
>  MODULE_STATE_LIVE, mod);
>
> @@ -3445,7 +3446,6 @@ static noinline int do_init_module(struct module *mod)
>  */
> call_rcu_sched(>rcu, do_free_init);
> mutex_unlock(_mutex);
> -   wake_up_all(_wq);
>
> return 0;
>

Will give this a shot, thanks!

  Luis

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

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> On Dec 16, 2016 9:54 PM, "Rusty Russell"  wrote:
> > AFAICT the mistake here is that kmod is returning "done, OK" when the
> > module it is trying to load is already loading (but not finished
> > loading).  That's the root problem; it's an attempt at optimization by
> > kmod which goes awry.
>
> This is true! To be precise though the truth of the matter is that kmod'd
> respective usermode helper: modprobe can be buggy and may lie to us. It may
> allow request_module() to return 0 but since we don't validate it, any
> assumption we make can be deadly. In the case of get_fs_type() its a null
> dereference.

Wait, what??  I can't see that in get_fs_type, which hasn't changed
since 2013.  If a caller is assuming get_fs_type() doesn't return NULL,
they're broken and need fixing of course:

struct file_system_type *get_fs_type(const char *name)
{
struct file_system_type *fs;
const char *dot = strchr(name, '.');
int len = dot ? dot - name : strlen(name);

fs = __get_fs_type(name, len);
if (!fs && (request_module("fs-%.*s", len, name) == 0))
fs = __get_fs_type(name, len);

if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
put_filesystem(fs);
fs = NULL;
}
return fs;
}

Where does this NULL-deref is the module isn't correctly loaded?

> *Iff* we want a sanity check to verify kmod's umh is not lying to us we
> need to verify after 0 was returned that it was not lying to us. Since kmod
> accepts aliases but find_modules_all() only works on the real module name a
> validation check cannot happen when all you have are aliases.

request_module() should block until resolution, but that's fundamentally
a userspace problem.  Let's not paper over it in kernelspace.

> *Iff* we are sure we don't want a validation (or another earlier
> optimization to avoid calling out to modrobe if the alias requested is
> already present, which does the time shaving I mentioned on the tests) then
> naturally no request_module() calls returning 0 can assert information
> about the requested module. I think we might need to change more code if we
> accept we cannot trust request_module() calls, or we accept userspace
> telling the kernel something may mean we sometimes crash. This later
> predicament seems rather odd so hence the patch.
>
> Perhaps in some cases validation of work from a umh is not critical in
> kernel but for request_module() I can tell you that today get_fs_type code
> currently asserts the module found can never be NULL.

OK, what am I missing in the code above?  

> > Looking at the code in the kernel, we *already* get this right: block if
> > a module is still loading anyway.  Once it succeeds we return -EBUSY if
> >
> > it fails we'll proceed to try to load it again.
> >
> > I don't understand what you're trying to fix with adding aliases
> > in-kernel?
>
> Two fold now:
>
> a) validation on request_module() work when an alias is used

But why?

> b) since kmod accepts aliaes, if we get aliases support, it means we could
> *also* preemptively avoid calling out to userspace for modules already
> present.

No, because once we have a module we don't request it: requesting is the
fallback case.

> >> FWIW a few things did occur to me:
> >>
> >> a) list_add_rcu() is used so new modules get added first
> >
> > Only after we're sure that there are no duplicates.
> >
> >
> OK! This is a very critical assertion. I should be able to add a debug
> WARN_ON() should two modules be on the modules list for the same module
> then ?

Yes, names must be unique.

>> b) find_module_all() returns the last module which was added as it
> traverses
>>the module list
>
>> BTW should find_module_all() use rcu to traverse?
>
> Yes; the kallsyms code does this on Oops.  Not really a big issue in
> practice, but a nice fix.
>
> Ok, will bundle into my queue.

Please submit to Jessica for her module queue, as it's orthogonal
AFAICT.

> I will note though that I still think there's a bug in this code --
> upon a failure other "spinning" requests can fail, I believe this may
> be due to not having another state or informing pending modules too
> early of a failure but I haven't been able to prove this conjecture
> yet.

That's possible, but I can't see it from quickly re-checking the code.

The module should be fully usable at this point; the module's init has
been called successfully, so in the case of __get_fs_type() it should
now succeed.  The module cleans up its init section, but that should be
independent.

If there is a race, it's likely to be when some other caller wakes the
queue.  Moving the wakeup as soon as possible should make it easier to
trigger:

diff --git a/kernel/module.c b/kernel/module.c
index f57dd63186e6..78bd89d41a22 100644
--- 

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

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> On Dec 16, 2016 9:54 PM, "Rusty Russell"  wrote:
> > AFAICT the mistake here is that kmod is returning "done, OK" when the
> > module it is trying to load is already loading (but not finished
> > loading).  That's the root problem; it's an attempt at optimization by
> > kmod which goes awry.
>
> This is true! To be precise though the truth of the matter is that kmod'd
> respective usermode helper: modprobe can be buggy and may lie to us. It may
> allow request_module() to return 0 but since we don't validate it, any
> assumption we make can be deadly. In the case of get_fs_type() its a null
> dereference.

Wait, what??  I can't see that in get_fs_type, which hasn't changed
since 2013.  If a caller is assuming get_fs_type() doesn't return NULL,
they're broken and need fixing of course:

struct file_system_type *get_fs_type(const char *name)
{
struct file_system_type *fs;
const char *dot = strchr(name, '.');
int len = dot ? dot - name : strlen(name);

fs = __get_fs_type(name, len);
if (!fs && (request_module("fs-%.*s", len, name) == 0))
fs = __get_fs_type(name, len);

if (dot && fs && !(fs->fs_flags & FS_HAS_SUBTYPE)) {
put_filesystem(fs);
fs = NULL;
}
return fs;
}

Where does this NULL-deref is the module isn't correctly loaded?

> *Iff* we want a sanity check to verify kmod's umh is not lying to us we
> need to verify after 0 was returned that it was not lying to us. Since kmod
> accepts aliases but find_modules_all() only works on the real module name a
> validation check cannot happen when all you have are aliases.

request_module() should block until resolution, but that's fundamentally
a userspace problem.  Let's not paper over it in kernelspace.

> *Iff* we are sure we don't want a validation (or another earlier
> optimization to avoid calling out to modrobe if the alias requested is
> already present, which does the time shaving I mentioned on the tests) then
> naturally no request_module() calls returning 0 can assert information
> about the requested module. I think we might need to change more code if we
> accept we cannot trust request_module() calls, or we accept userspace
> telling the kernel something may mean we sometimes crash. This later
> predicament seems rather odd so hence the patch.
>
> Perhaps in some cases validation of work from a umh is not critical in
> kernel but for request_module() I can tell you that today get_fs_type code
> currently asserts the module found can never be NULL.

OK, what am I missing in the code above?  

> > Looking at the code in the kernel, we *already* get this right: block if
> > a module is still loading anyway.  Once it succeeds we return -EBUSY if
> >
> > it fails we'll proceed to try to load it again.
> >
> > I don't understand what you're trying to fix with adding aliases
> > in-kernel?
>
> Two fold now:
>
> a) validation on request_module() work when an alias is used

But why?

> b) since kmod accepts aliaes, if we get aliases support, it means we could
> *also* preemptively avoid calling out to userspace for modules already
> present.

No, because once we have a module we don't request it: requesting is the
fallback case.

> >> FWIW a few things did occur to me:
> >>
> >> a) list_add_rcu() is used so new modules get added first
> >
> > Only after we're sure that there are no duplicates.
> >
> >
> OK! This is a very critical assertion. I should be able to add a debug
> WARN_ON() should two modules be on the modules list for the same module
> then ?

Yes, names must be unique.

>> b) find_module_all() returns the last module which was added as it
> traverses
>>the module list
>
>> BTW should find_module_all() use rcu to traverse?
>
> Yes; the kallsyms code does this on Oops.  Not really a big issue in
> practice, but a nice fix.
>
> Ok, will bundle into my queue.

Please submit to Jessica for her module queue, as it's orthogonal
AFAICT.

> I will note though that I still think there's a bug in this code --
> upon a failure other "spinning" requests can fail, I believe this may
> be due to not having another state or informing pending modules too
> early of a failure but I haven't been able to prove this conjecture
> yet.

That's possible, but I can't see it from quickly re-checking the code.

The module should be fully usable at this point; the module's init has
been called successfully, so in the case of __get_fs_type() it should
now succeed.  The module cleans up its init section, but that should be
independent.

If there is a race, it's likely to be when some other caller wakes the
queue.  Moving the wakeup as soon as possible should make it easier to
trigger:

diff --git a/kernel/module.c b/kernel/module.c
index f57dd63186e6..78bd89d41a22 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ 

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

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> On Thu, Dec 15, 2016 at 10:57:42AM +1030, Rusty Russell wrote:
>> "Luis R. Rodriguez"  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.

AFAICT the mistake here is that kmod is returning "done, OK" when the
module it is trying to load is already loading (but not finished
loading).  That's the root problem; it's an attempt at optimization by
kmod which goes awry.

Looking at the code in the kernel, we *already* get this right: block if
a module is still loading anyway.  Once it succeeds we return -EBUSY; if
it fails we'll proceed to try to load it again.

I don't understand what you're trying to fix with adding aliases
in-kernel?

> FWIW a few things did occur to me:
>
> a) list_add_rcu() is used so new modules get added first

Only after we're sure that there are no duplicates.

> b) find_module_all() returns the last module which was added as it traverses
>the module list

> BTW should find_module_all() use rcu to traverse?

Yes; the kallsyms code does this on Oops.  Not really a big issue in
practice, but a nice fix.

Thanks,
Rusty.

>
> --- 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, , list) {
> + list_for_each_entry_rcu(mod, , 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(>list, );
> + list_add_tail_rcu(>list, );
>   mod_tree_insert(mod);
>   err = 0;
>  

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

[Rusty Russell]

"Luis R. Rodriguez"  writes:
> On Thu, Dec 15, 2016 at 10:57:42AM +1030, Rusty Russell wrote:
>> "Luis R. Rodriguez"  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.

AFAICT the mistake here is that kmod is returning "done, OK" when the
module it is trying to load is already loading (but not finished
loading).  That's the root problem; it's an attempt at optimization by
kmod which goes awry.

Looking at the code in the kernel, we *already* get this right: block if
a module is still loading anyway.  Once it succeeds we return -EBUSY; if
it fails we'll proceed to try to load it again.

I don't understand what you're trying to fix with adding aliases
in-kernel?

> FWIW a few things did occur to me:
>
> a) list_add_rcu() is used so new modules get added first

Only after we're sure that there are no duplicates.

> b) find_module_all() returns the last module which was added as it traverses
>the module list

> BTW should find_module_all() use rcu to traverse?

Yes; the kallsyms code does this on Oops.  Not really a big issue in
practice, but a nice fix.

Thanks,
Rusty.

>
> --- 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, , list) {
> + list_for_each_entry_rcu(mod, , 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(>list, );
> + list_add_tail_rcu(>list, );
>   mod_tree_insert(mod);
>   err = 0;
>  

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

[Luis R. Rodriguez]

On Thu, Dec 15, 2016 at 10:57:42AM +1030, Rusty Russell wrote:
> "Luis R. Rodriguez"  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, , list) {
+   list_for_each_entry_rcu(mod, , 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(>list, );
+   list_add_tail_rcu(>list, );
mod_tree_insert(mod);
err = 0;
 

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

[Luis R. Rodriguez]

On Thu, Dec 15, 2016 at 10:57:42AM +1030, Rusty Russell wrote:
> "Luis R. Rodriguez"  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, , list) {
+   list_for_each_entry_rcu(mod, , 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(>list, );
+   list_add_tail_rcu(>list, );
mod_tree_insert(mod);
err = 0;
 

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

[Luis R. Rodriguez]

On Fri, Dec 09, 2016 at 12:56:21PM -0800, Linus Torvalds wrote:
> On Fri, Dec 9, 2016 at 12:03 PM, Martin Wilck  wrote:
> > On Thu, 2016-12-08 at 11:49 -0800, Luis R. Rodriguez wrote:
> >>
> >> Although this does get us in the business of keeping alias maps in
> >> kernel, the the work to support and maintain this is trivial.
> >
> > You've implemented a special treatment for request_module("fs-$X")in
> > finished_kmod_load(), but there are many more aliases defined (and
> > used) in the kernel. Do you plan to implement special code for "char-
> > major-$X", "crypto-$X", "binfmt-$X" etc. later?
> 
> Yeah, no, that is just complete garbage.
> 
> Those module aliases already exist in the module info section. We just
> don't parse the alias tags in the kernel.
> 
> So the real fix is to make find_module_all() just do that.

Ah yes, that is much sexier, this is now done and it works nicely, thanks
for the suggestion.

> Doing random ad-hoc "let's prefix with 'fs-xyz'" games are completely
> unacceptable. That's just pure shit. Stop this idiocy.

Look at that fin DNA in action :)

  Luis

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

[Luis R. Rodriguez]

On Fri, Dec 09, 2016 at 12:56:21PM -0800, Linus Torvalds wrote:
> On Fri, Dec 9, 2016 at 12:03 PM, Martin Wilck  wrote:
> > On Thu, 2016-12-08 at 11:49 -0800, Luis R. Rodriguez wrote:
> >>
> >> Although this does get us in the business of keeping alias maps in
> >> kernel, the the work to support and maintain this is trivial.
> >
> > You've implemented a special treatment for request_module("fs-$X")in
> > finished_kmod_load(), but there are many more aliases defined (and
> > used) in the kernel. Do you plan to implement special code for "char-
> > major-$X", "crypto-$X", "binfmt-$X" etc. later?
> 
> Yeah, no, that is just complete garbage.
> 
> Those module aliases already exist in the module info section. We just
> don't parse the alias tags in the kernel.
> 
> So the real fix is to make find_module_all() just do that.

Ah yes, that is much sexier, this is now done and it works nicely, thanks
for the suggestion.

> Doing random ad-hoc "let's prefix with 'fs-xyz'" games are completely
> unacceptable. That's just pure shit. Stop this idiocy.

Look at that fin DNA in action :)

  Luis

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

[Rusty Russell]

"Luis R. Rodriguez"  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.  Otherwise it has to do some weird spinning
thing in userspace anyway.

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.

Cheers,
Rusty.

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

[Rusty Russell]

"Luis R. Rodriguez"  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.  Otherwise it has to do some weird spinning
thing in userspace anyway.

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.

Cheers,
Rusty.

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

[Linus Torvalds]

On Fri, Dec 9, 2016 at 12:03 PM, Martin Wilck  wrote:
> On Thu, 2016-12-08 at 11:49 -0800, Luis R. Rodriguez wrote:
>>
>> Although this does get us in the business of keeping alias maps in
>> kernel, the the work to support and maintain this is trivial.
>
> You've implemented a special treatment for request_module("fs-$X")in
> finished_kmod_load(), but there are many more aliases defined (and
> used) in the kernel. Do you plan to implement special code for "char-
> major-$X", "crypto-$X", "binfmt-$X" etc. later?

Yeah, no, that is just complete garbage.

Those module aliases already exist in the module info section. We just
don't parse the alias tags in the kernel.

So the real fix is to make find_module_all() just do that.

Doing random ad-hoc "let's prefix with 'fs-xyz'" games are completely
unacceptable. That's just pure shit. Stop this idiocy.

Linus

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

[Linus Torvalds]

On Fri, Dec 9, 2016 at 12:03 PM, Martin Wilck  wrote:
> On Thu, 2016-12-08 at 11:49 -0800, Luis R. Rodriguez wrote:
>>
>> Although this does get us in the business of keeping alias maps in
>> kernel, the the work to support and maintain this is trivial.
>
> You've implemented a special treatment for request_module("fs-$X")in
> finished_kmod_load(), but there are many more aliases defined (and
> used) in the kernel. Do you plan to implement special code for "char-
> major-$X", "crypto-$X", "binfmt-$X" etc. later?

Yeah, no, that is just complete garbage.

Those module aliases already exist in the module info section. We just
don't parse the alias tags in the kernel.

So the real fix is to make find_module_all() just do that.

Doing random ad-hoc "let's prefix with 'fs-xyz'" games are completely
unacceptable. That's just pure shit. Stop this idiocy.

Linus

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

[Martin Wilck]

On Thu, 2016-12-08 at 11:49 -0800, Luis R. Rodriguez wrote:
> 
> Although this does get us in the business of keeping alias maps in
> kernel, the the work to support and maintain this is trivial.

You've implemented a special treatment for request_module("fs-$X")in
finished_kmod_load(), but there are many more aliases defined (and
used) in the kernel. Do you plan to implement special code for "char-
major-$X", "crypto-$X", "binfmt-$X" etc. later? 

Regards
Martin

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

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

[Martin Wilck]

On Thu, 2016-12-08 at 11:49 -0800, Luis R. Rodriguez wrote:
> 
> Although this does get us in the business of keeping alias maps in
> kernel, the the work to support and maintain this is trivial.

You've implemented a special treatment for request_module("fs-$X")in
finished_kmod_load(), but there are many more aliases defined (and
used) in the kernel. Do you plan to implement special code for "char-
major-$X", "crypto-$X", "binfmt-$X" etc. later? 

Regards
Martin

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)