Re: kmod: add a sanity check on module loading

[Luis R. Rodriguez]

On Fri, Jan 06, 2017 at 04:53:54PM -0500, Jessica Yu wrote:
> +++ 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"  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.

It seems proper form is hard to vet for, and the return value actually
doesn't really give us much useful information.

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

Yup!

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

Validation check on the caller makes sense *but* what makes this a bit hard
is as I have found, request_module() *call* can fail for some reasons
other than the module not being available on the system -- races, and inherent
design decisions (kmod concurrent). In my patch series I address kmod concurrent
limit to be more graceful, the clutch I mentioned is another addition
to help make failures be less aggressive.

Because some issues can creep up with request_module() -- checking its return
value seems desirable -- but as Rusty notes its currently only seen as
an optimization to check for the return value. Its not really clear what
the best path forward is. Here are a bit of my current thoughts:

  o The debug check Rusty suggested seems fair for upstream get_fs_type() in
retropsect.

  o Although callers should validate a module was loaded and that should
in theory suffice to cover most API failures 

Re: kmod: add a sanity check on module loading

[Luis R. Rodriguez]

On Fri, Jan 06, 2017 at 04:53:54PM -0500, Jessica Yu wrote:
> +++ 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"  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.

It seems proper form is hard to vet for, and the return value actually
doesn't really give us much useful information.

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

Yup!

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

Validation check on the caller makes sense *but* what makes this a bit hard
is as I have found, request_module() *call* can fail for some reasons
other than the module not being available on the system -- races, and inherent
design decisions (kmod concurrent). In my patch series I address kmod concurrent
limit to be more graceful, the clutch I mentioned is another addition
to help make failures be less aggressive.

Because some issues can creep up with request_module() -- checking its return
value seems desirable -- but as Rusty notes its currently only seen as
an optimization to check for the return value. Its not really clear what
the best path forward is. Here are a bit of my current thoughts:

  o The debug check Rusty suggested seems fair for upstream get_fs_type() in
retropsect.

  o Although callers should validate a module was loaded and that should
in theory suffice to cover most API failures on request_module(),

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: kmod: add a sanity check on module loading

[Jessica Yu]

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

Re: kmod: add a sanity check on module loading

[Jessica Yu]

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

Re: kmod: add a sanity check on module loading

[Jessica Yu]

+++ Rusty Russell [03/01/17 10:34 +1030]:

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


I was under the impression that aliases were a userspace concern. i.e., we let
kmod tools take care of alias resolution and bookkeeping. I'm getting the
feeling we're bending over backwards here to accommodate buggy/untrustworthy
userspace (modprobe). If I understand correctly, we're performing this
validation work - we're proposing to make the kernel alias-aware - because we
can't even trust modprobe's return value, and the proposal is to double check
this work ourselves in-kernel.

But I thought that request_module() wasn't written to provide these "module is
now live and loaded" guarantees in the first place. This seems to be documented
in kernel/kmod.c - "Callers must check that the service they requested is now
available not blindly invoke it." Isn't it the caller's responsibility to
(indirectly) validate request_module's work, to check that the service they 
want is
now there? If a caller doesn't do this, then this is a bug on their side. If it
is crucial for get_fs_type() to not fail, then perhaps we should be tightening
get_fs_type() instead, be that WARNing if the requested filesystem is still not
there (as suggested earlier), or maybe even trying the request again.


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: kmod: add a sanity check on module loading

[Jessica Yu]

+++ Rusty Russell [03/01/17 10:34 +1030]:

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


I was under the impression that aliases were a userspace concern. i.e., we let
kmod tools take care of alias resolution and bookkeeping. I'm getting the
feeling we're bending over backwards here to accommodate buggy/untrustworthy
userspace (modprobe). If I understand correctly, we're performing this
validation work - we're proposing to make the kernel alias-aware - because we
can't even trust modprobe's return value, and the proposal is to double check
this work ourselves in-kernel.

But I thought that request_module() wasn't written to provide these "module is
now live and loaded" guarantees in the first place. This seems to be documented
in kernel/kmod.c - "Callers must check that the service they requested is now
available not blindly invoke it." Isn't it the caller's responsibility to
(indirectly) validate request_module's work, to check that the service they 
want is
now there? If a caller doesn't do this, then this is a bug on their side. If it
is crucial for get_fs_type() to not fail, then perhaps we should be tightening
get_fs_type() instead, be that WARNing if the requested filesystem is still not
there (as suggested earlier), or maybe even trying the request again.


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, 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: kmod: add a sanity check on module loading

[Jessica Yu]

+++ Luis R. Rodriguez [08/12/16 11:49 -0800]:

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.

Additionally kmod <= v19 was broken -- it returns 0 to modprobe calls,
assuming the kernel module is built-in, where really we have a race as
the module starts forming. kmod <= v19 has incorrect userspace heuristics,
a userspace kmod fix is available for it:

http://git.kernel.org/cgit/utils/kernel/kmod/kmod.git/commit/libkmod/libkmod-module.c?id=fd44a98ae2eb5eb32161088954ab21e58e19dfc4

This changes kmod to address both:

o Provides the alias optimization for get_fs_type() so modules already
  loaded do not get re-requested.

o Provides a sanity test to verify modprobe's work

This is important given how any get_fs_type() users assert success
means we're ready to go, and tests with the new test_kmod stress driver
reveal that request_module() and get_fs_type() might fail for a few
other reasons. You don't need old kmod to fail on request_module() or
get_fs_type(), with the right system setup, these calls *can* fail
today.

Although this does get us in the business of keeping alias maps in
kernel, the the work to support and maintain this is trivial.
Aditionally, since it may be important get_fs_type() should not fail on
certain systems, this tightens things up a bit more.

The TL;DR:

kmod <= v19 will return 0 on modprobe calls if you are built-in,
however its heuristics for checking if you are built-in were broken.

It assumed that having the directory /sys/module/module-name
but not having the file /sys/module/module-name/initstate
is sufficient to assume a module is built-in.

The kernel loads the inittstate attribute *after* it creates the
directory. This is an issue when modprobe returns 0 for kernel calls
which assumes a return of 0 on request_module() can give you the
right to assert the module is loaded and live.

We cannot trust returns of modprobe as 0 in the kernel, we need to
verify that modules are live if modprobe return 0 but only if modules
*are* modules. The kernel heuristic we use to determine if a module is
built-in is that if modprobe returns 0 we know we must be built-in or
a module, but if we are a module clearly we must have a lingering kmod
dangling on our linked list. If there is no modules there we are *somewhat*
certain the module must be built in.

This is not enough though... we cannot easily work around this since the
kernel can use aliases to userspace for modules calls. For instance
fs/namespace.c uses fs-modulename for filesystesms on get_fs_type(), so
these need to be taken into consideration as well.

Using kmod <= 19 will give you a NULL get_fs_type() return even though
the module was loaded... That is a corner case, there are other failures
for request_module() though -- the other failures are not easy to
reproduce though but fortunately we have a stress test driver to help
with that now. Use the following tests:

# tools/testing/selftests/kmod/kmod.sh -t 0008
# tools/testing/selftests/kmod/kmod.sh -t 0009

You can more easily see this error if you have kmod <= v19 installed.

You will need to install kmod <= v19, be sure to install its modprobe
into /sbin/ as by default the 'make install' target does not replace
your own.

This test helps cure test_kmod cases 0008 0009 so enable them.

Reported-by: Martin Wilck 
Reported-by: Randy Wright 
Signed-off-by: Luis R. Rodriguez 


Back from travel today, apologies for the delay. Will be able to give
this a proper look this week.

Jessica

Re: kmod: add a sanity check on module loading

[Jessica Yu]

+++ Luis R. Rodriguez [08/12/16 11:49 -0800]:

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.

Additionally kmod <= v19 was broken -- it returns 0 to modprobe calls,
assuming the kernel module is built-in, where really we have a race as
the module starts forming. kmod <= v19 has incorrect userspace heuristics,
a userspace kmod fix is available for it:

http://git.kernel.org/cgit/utils/kernel/kmod/kmod.git/commit/libkmod/libkmod-module.c?id=fd44a98ae2eb5eb32161088954ab21e58e19dfc4

This changes kmod to address both:

o Provides the alias optimization for get_fs_type() so modules already
  loaded do not get re-requested.

o Provides a sanity test to verify modprobe's work

This is important given how any get_fs_type() users assert success
means we're ready to go, and tests with the new test_kmod stress driver
reveal that request_module() and get_fs_type() might fail for a few
other reasons. You don't need old kmod to fail on request_module() or
get_fs_type(), with the right system setup, these calls *can* fail
today.

Although this does get us in the business of keeping alias maps in
kernel, the the work to support and maintain this is trivial.
Aditionally, since it may be important get_fs_type() should not fail on
certain systems, this tightens things up a bit more.

The TL;DR:

kmod <= v19 will return 0 on modprobe calls if you are built-in,
however its heuristics for checking if you are built-in were broken.

It assumed that having the directory /sys/module/module-name
but not having the file /sys/module/module-name/initstate
is sufficient to assume a module is built-in.

The kernel loads the inittstate attribute *after* it creates the
directory. This is an issue when modprobe returns 0 for kernel calls
which assumes a return of 0 on request_module() can give you the
right to assert the module is loaded and live.

We cannot trust returns of modprobe as 0 in the kernel, we need to
verify that modules are live if modprobe return 0 but only if modules
*are* modules. The kernel heuristic we use to determine if a module is
built-in is that if modprobe returns 0 we know we must be built-in or
a module, but if we are a module clearly we must have a lingering kmod
dangling on our linked list. If there is no modules there we are *somewhat*
certain the module must be built in.

This is not enough though... we cannot easily work around this since the
kernel can use aliases to userspace for modules calls. For instance
fs/namespace.c uses fs-modulename for filesystesms on get_fs_type(), so
these need to be taken into consideration as well.

Using kmod <= 19 will give you a NULL get_fs_type() return even though
the module was loaded... That is a corner case, there are other failures
for request_module() though -- the other failures are not easy to
reproduce though but fortunately we have a stress test driver to help
with that now. Use the following tests:

# tools/testing/selftests/kmod/kmod.sh -t 0008
# tools/testing/selftests/kmod/kmod.sh -t 0009

You can more easily see this error if you have kmod <= v19 installed.

You will need to install kmod <= v19, be sure to install its modprobe
into /sbin/ as by default the 'make install' target does not replace
your own.

This test helps cure test_kmod cases 0008 0009 so enable them.

Reported-by: Martin Wilck 
Reported-by: Randy Wright 
Signed-off-by: Luis R. Rodriguez 


Back from travel today, apologies for the delay. Will be able to give
this a proper look this week.

Jessica

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)

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

[Luis R. Rodriguez]

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.

Additionally kmod <= v19 was broken -- it returns 0 to modprobe calls,
assuming the kernel module is built-in, where really we have a race as
the module starts forming. kmod <= v19 has incorrect userspace heuristics,
a userspace kmod fix is available for it:

http://git.kernel.org/cgit/utils/kernel/kmod/kmod.git/commit/libkmod/libkmod-module.c?id=fd44a98ae2eb5eb32161088954ab21e58e19dfc4

This changes kmod to address both:

 o Provides the alias optimization for get_fs_type() so modules already
   loaded do not get re-requested.

 o Provides a sanity test to verify modprobe's work

This is important given how any get_fs_type() users assert success
means we're ready to go, and tests with the new test_kmod stress driver
reveal that request_module() and get_fs_type() might fail for a few
other reasons. You don't need old kmod to fail on request_module() or
get_fs_type(), with the right system setup, these calls *can* fail
today.

Although this does get us in the business of keeping alias maps in
kernel, the the work to support and maintain this is trivial.
Aditionally, since it may be important get_fs_type() should not fail on
certain systems, this tightens things up a bit more.

The TL;DR:

kmod <= v19 will return 0 on modprobe calls if you are built-in,
however its heuristics for checking if you are built-in were broken.

It assumed that having the directory /sys/module/module-name
but not having the file /sys/module/module-name/initstate
is sufficient to assume a module is built-in.

The kernel loads the inittstate attribute *after* it creates the
directory. This is an issue when modprobe returns 0 for kernel calls
which assumes a return of 0 on request_module() can give you the
right to assert the module is loaded and live.

We cannot trust returns of modprobe as 0 in the kernel, we need to
verify that modules are live if modprobe return 0 but only if modules
*are* modules. The kernel heuristic we use to determine if a module is
built-in is that if modprobe returns 0 we know we must be built-in or
a module, but if we are a module clearly we must have a lingering kmod
dangling on our linked list. If there is no modules there we are *somewhat*
certain the module must be built in.

This is not enough though... we cannot easily work around this since the
kernel can use aliases to userspace for modules calls. For instance
fs/namespace.c uses fs-modulename for filesystesms on get_fs_type(), so
these need to be taken into consideration as well.

Using kmod <= 19 will give you a NULL get_fs_type() return even though
the module was loaded... That is a corner case, there are other failures
for request_module() though -- the other failures are not easy to
reproduce though but fortunately we have a stress test driver to help
with that now. Use the following tests:

 # tools/testing/selftests/kmod/kmod.sh -t 0008
 # tools/testing/selftests/kmod/kmod.sh -t 0009

You can more easily see this error if you have kmod <= v19 installed.

You will need to install kmod <= v19, be sure to install its modprobe
into /sbin/ as by default the 'make install' target does not replace
your own.

This test helps cure test_kmod cases 0008 0009 so enable them.

Reported-by: Martin Wilck 
Reported-by: Randy Wright 
Signed-off-by: Luis R. Rodriguez 
---
 kernel/kmod.c| 73 
 kernel/module.c  | 11 --
 tools/testing/selftests/kmod/kmod.sh |  9 ++---
 3 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index a0f449f77ed7..6bf0feab41d1 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -61,6 +61,11 @@ static DECLARE_RWSEM(umhelper_sem);
 
 #ifdef CONFIG_MODULES
 
+bool finished_loading(const char *name);
+int module_wait_until_finished(const char *name);
+struct module *find_module_all(const char *name, size_t len,
+  bool even_unformed);
+
 /*
modprobe_path is set via /proc/sys.
 */
@@ -158,6 +163,72 @@ int get_kmod_umh_count(void)
return atomic_read(_concurrent);
 }
 
+static bool kmod_exists(char *name)
+{
+   struct module *mod;
+
+   mutex_lock(_mutex);
+   mod = find_module_all(name, strlen(name), true);
+   mutex_unlock(_mutex);
+
+   if (mod)
+   return true;
+
+   return false;
+}
+
+/*
+ * The assumption is this must be a module, it could still not be live though
+ * since kmod <= 19 returns 0 even if it was not ready yet.  Allow for force
+ * wait check in case you are stuck on old userspace.
+ */
+static int wait_for_kmod(char *name)
+{
+   int ret = 0;
+
+   if (!finished_loading(name))
+   ret = 

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

[Luis R. Rodriguez]

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.

Additionally kmod <= v19 was broken -- it returns 0 to modprobe calls,
assuming the kernel module is built-in, where really we have a race as
the module starts forming. kmod <= v19 has incorrect userspace heuristics,
a userspace kmod fix is available for it:

http://git.kernel.org/cgit/utils/kernel/kmod/kmod.git/commit/libkmod/libkmod-module.c?id=fd44a98ae2eb5eb32161088954ab21e58e19dfc4

This changes kmod to address both:

 o Provides the alias optimization for get_fs_type() so modules already
   loaded do not get re-requested.

 o Provides a sanity test to verify modprobe's work

This is important given how any get_fs_type() users assert success
means we're ready to go, and tests with the new test_kmod stress driver
reveal that request_module() and get_fs_type() might fail for a few
other reasons. You don't need old kmod to fail on request_module() or
get_fs_type(), with the right system setup, these calls *can* fail
today.

Although this does get us in the business of keeping alias maps in
kernel, the the work to support and maintain this is trivial.
Aditionally, since it may be important get_fs_type() should not fail on
certain systems, this tightens things up a bit more.

The TL;DR:

kmod <= v19 will return 0 on modprobe calls if you are built-in,
however its heuristics for checking if you are built-in were broken.

It assumed that having the directory /sys/module/module-name
but not having the file /sys/module/module-name/initstate
is sufficient to assume a module is built-in.

The kernel loads the inittstate attribute *after* it creates the
directory. This is an issue when modprobe returns 0 for kernel calls
which assumes a return of 0 on request_module() can give you the
right to assert the module is loaded and live.

We cannot trust returns of modprobe as 0 in the kernel, we need to
verify that modules are live if modprobe return 0 but only if modules
*are* modules. The kernel heuristic we use to determine if a module is
built-in is that if modprobe returns 0 we know we must be built-in or
a module, but if we are a module clearly we must have a lingering kmod
dangling on our linked list. If there is no modules there we are *somewhat*
certain the module must be built in.

This is not enough though... we cannot easily work around this since the
kernel can use aliases to userspace for modules calls. For instance
fs/namespace.c uses fs-modulename for filesystesms on get_fs_type(), so
these need to be taken into consideration as well.

Using kmod <= 19 will give you a NULL get_fs_type() return even though
the module was loaded... That is a corner case, there are other failures
for request_module() though -- the other failures are not easy to
reproduce though but fortunately we have a stress test driver to help
with that now. Use the following tests:

 # tools/testing/selftests/kmod/kmod.sh -t 0008
 # tools/testing/selftests/kmod/kmod.sh -t 0009

You can more easily see this error if you have kmod <= v19 installed.

You will need to install kmod <= v19, be sure to install its modprobe
into /sbin/ as by default the 'make install' target does not replace
your own.

This test helps cure test_kmod cases 0008 0009 so enable them.

Reported-by: Martin Wilck 
Reported-by: Randy Wright 
Signed-off-by: Luis R. Rodriguez 
---
 kernel/kmod.c| 73 
 kernel/module.c  | 11 --
 tools/testing/selftests/kmod/kmod.sh |  9 ++---
 3 files changed, 85 insertions(+), 8 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index a0f449f77ed7..6bf0feab41d1 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -61,6 +61,11 @@ static DECLARE_RWSEM(umhelper_sem);
 
 #ifdef CONFIG_MODULES
 
+bool finished_loading(const char *name);
+int module_wait_until_finished(const char *name);
+struct module *find_module_all(const char *name, size_t len,
+  bool even_unformed);
+
 /*
modprobe_path is set via /proc/sys.
 */
@@ -158,6 +163,72 @@ int get_kmod_umh_count(void)
return atomic_read(_concurrent);
 }
 
+static bool kmod_exists(char *name)
+{
+   struct module *mod;
+
+   mutex_lock(_mutex);
+   mod = find_module_all(name, strlen(name), true);
+   mutex_unlock(_mutex);
+
+   if (mod)
+   return true;
+
+   return false;
+}
+
+/*
+ * The assumption is this must be a module, it could still not be live though
+ * since kmod <= 19 returns 0 even if it was not ready yet.  Allow for force
+ * wait check in case you are stuck on old userspace.
+ */
+static int wait_for_kmod(char *name)
+{
+   int ret = 0;
+
+   if (!finished_loading(name))
+   ret = module_wait_until_finished(name);
+
+   return ret;
+}
+
+/*
+ * kmod <= 19 will