Re: [PATCH] kmod: make request_module() return an error when autoloading is disabled

[Eric Biggers]

On Wed, Mar 11, 2020 at 06:31:30AM +0000, Luis Chamberlain wrote:
> On Tue, Mar 10, 2020 at 10:26:20PM -0700, Eric Biggers wrote:
> > On Wed, Mar 11, 2020 at 04:32:21AM +0000, Luis Chamberlain wrote:
> > > On Tue, Mar 10, 2020 at 03:37:31PM -0700, Eric Biggers wrote:
> > > > From: Eric Biggers <ebiggers@xxxxxxxxxx>
> > > > 
> > > > It's long been possible to disable kernel module autoloading completely
> > > > by setting /proc/sys/kernel/modprobe to the empty string.  This can be
> > > > preferable
> > > 
> > > preferable but ... not documented. Or was this documented or recommended
> > > somewhere?
> > > 
> > > > to setting it to a nonexistent file since it avoids the
> > > > overhead of an attempted execve(), avoids potential deadlocks, and
> > > > avoids the call to security_kernel_module_request() and thus on
> > > > SELinux-based systems eliminates the need to write SELinux rules to
> > > > dontaudit module_request.
> > 
> > Not that I know of, though I didn't look too hard.  proc(5) mentions
> > /proc/sys/kernel/modprobe but doesn't mention the empty string case.
> > 
> > In any case, it's been supported for a long time, and it's useful for the
> > reasons I mentioned.
> 
> Sure. I think then its important to document it as such then, or perhaps
> make a kconfig option which sets this to empty and document it on the
> kconfig entry.

I'll send a man-pages patch to document it in proc(5).

Most users, including the one I have in mind, should just be able to run
'echo > /proc/sys/kernel/modprobe' early in the boot process.  So I don't think
the need for a kconfig option to control the default value has been clearly
demonstrated yet.  You're certainly welcome to send a patch for it if you
believe it would be useful, though.

> > So I don't see
> > why it would be controversial?  I already went through all callers of
> > request_module() that check its return value, and they all appear to work better
> > with -ENOENT, since they assume that 0 means the module was loaded.
> 
> Thanks for doing that, but I note that getting 0 is not assurance
> either. The de-facto best practive for the request_module() call is to
> do your own in place verifier.
> 
> > Incorrectly returning 0 typically causes unnecessary work (checking again
> > whether the module's functionality is available) or misleading log messages.
> 
> Yes but returning 0 cannot be relied upon today for assuming the module
> is loaded. *If* we revisit that decision and want the kernel to do a
> generic verifier, then yes, we can get rid of all the caller specific
> verfifiers, but not today.
> 

Sure, I understand all that; I think we're actually on the same page.  Even if
we make the return value of request_module() completely correct, nothing stops
another process from loading or unloading the module immediately afterwards.

However, callers do sometimes use the return value opportunisticly, like to log
an appropriate message or to return early if module loading failed.  Those seem
like relatively appropriate uses.  The thing which you really can't do, which I
didn't see anyone doing, is use 'ret == 0' as a signal to go ahead and run code
that will crash if the module has been unloaded already.

- Eric