[RFC] race in request_module()

[RFC] race in request_module()

[Alexander Viro]

	Looks like request_module() has quite a few problems:

* there is no way to distinguish between failing modprobe and successful
  one followed by rmmod -a (e.g. called by cron).  For one thing, we
  don't pass exit value of modprobe to caller of request_module().

* even if we would pass it, obvious attempt to cope with rmmod -a races
  fails.  I.e. something like

	while (object doesn't exist) {
		if (request_module(module_name) < 0)
			break;
	}

  would screw up for something like

mount -t floppy <whatever>

  since we would happily load floppy.o and look for fs type called "floppy".
  And keep doing that forever, since floppy.o doesn't define any fs.

* we could try to protect against rmmod -a by changing semantics of module
  syscalls and modprobe(8).  Namely, let modprobe called by request_module()
  pin the module(s) down and make request_module() (actually its caller)
  decrement refcounts.  That would solve the problem, but we get another one:
  how to find all modules pulled in by modprobe(8) and its children.
  Notice that argument of request_module() doesn't help at all - it can have
  nothing to name of module we load (block-major-2 -> floppy) and we could have
  other modules grabbed by the same modprobe.

* we might try to pull the following trick: in sys_create_module() follow
  ->parent until we step on request_module()-spawned task.  Then put the new
  module on a list for that instance of request_module().  That would solve
  the problem, but I'm not too happy about such solution - IMO it's ugly.
  However, I don't see anything else...

Comments?

  

Re: [RFC] race in request_module()

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 2828 bytes --]

Isn't the real problem here that we've got a "rogue" running around
removing things that we might be about to use?

Yes, I think that request_module() should indicate to the caller if
something "suitable" was found.  But I think having rmmod -a running around
sweeping things randomly is bad.

Perhaps what we need is a way to tell _how_long_ago_ the count on a module
last changed.  Thus, rmmod -a could decide to only remove modules that were
last used more than an hour ago, or somesuch.  Push the policy question into
userspace.

Matt

On Mon, Apr 22, 2002 at 08:49:40PM -0400, Alexander Viro wrote:
> 	Looks like request_module() has quite a few problems:
> 
> * there is no way to distinguish between failing modprobe and successful
>   one followed by rmmod -a (e.g. called by cron).  For one thing, we
>   don't pass exit value of modprobe to caller of request_module().
> 
> * even if we would pass it, obvious attempt to cope with rmmod -a races
>   fails.  I.e. something like
> 
> 	while (object doesn't exist) {
> 		if (request_module(module_name) < 0)
> 			break;
> 	}
> 
>   would screw up for something like
> 
> mount -t floppy <whatever>
> 
>   since we would happily load floppy.o and look for fs type called "floppy".
>   And keep doing that forever, since floppy.o doesn't define any fs.
> 
> * we could try to protect against rmmod -a by changing semantics of module
>   syscalls and modprobe(8).  Namely, let modprobe called by request_module()
>   pin the module(s) down and make request_module() (actually its caller)
>   decrement refcounts.  That would solve the problem, but we get another one:
>   how to find all modules pulled in by modprobe(8) and its children.
>   Notice that argument of request_module() doesn't help at all - it can have
>   nothing to name of module we load (block-major-2 -> floppy) and we could have
>   other modules grabbed by the same modprobe.
> 
> * we might try to pull the following trick: in sys_create_module() follow
>   ->parent until we step on request_module()-spawned task.  Then put the new
>   module on a list for that instance of request_module().  That would solve
>   the problem, but I'm not too happy about such solution - IMO it's ugly.
>   However, I don't see anything else...
> 
> Comments?
> 
>   
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

YOU SEE!!?? It's like being born with only one nipple!
					-- Erwin
User Friendly, 10/19/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [RFC] race in request_module()

[Alexander Viro]

On Mon, 22 Apr 2002, Matthew Dharm wrote:

> Isn't the real problem here that we've got a "rogue" running around
> removing things that we might be about to use?
> 
> Yes, I think that request_module() should indicate to the caller if
> something "suitable" was found.  But I think having rmmod -a running around
> sweeping things randomly is bad.
> 
> Perhaps what we need is a way to tell _how_long_ago_ the count on a module
> last changed.  Thus, rmmod -a could decide to only remove modules that were
> last used more than an hour ago, or somesuch.  Push the policy question into
> userspace.

Still doesn't solve the problem.  And BTW, there are userland races of
similar kind - foo.o depends on bar.o, modprobe loads bar.o, goes to look
for foo.o and gets bar.o removed from under it.

The thing being, relying on time doesn't help - e.g. we might have modules
on automounted volume and delays may be really long if the thing happens
at time when load is high.

Re: [RFC] race in request_module()

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

The question then becomes one of how do I distinguish a race condition from
a legitimate load/unload cycle?

I'm not certain that I see a way.  Unless we mark modules as "not used
yet", until the first piece of code in them is used.  But that feels like
an ugly hack, and seems likely to be problematic for some unusual
scenarios.

It looks like we might need another state for modules to be in.  Or simply
discourage people from auto-unloading "unused" modules.

I've seen this on usb-storage also, where the module count is maintained by
the SCSI layers -- it's only non-zero when someone/something is actively
using a device, which means that it will tend to get unloaded by an rmmod
-a if we're between CD burns, for example.  And, when the module is
unloaded, all sorts of state information is lost.  rmmod -a is my enemy in
this case.

Isn't the problem here just the misuse of rmmod -a?  Perhaps we should
attach a warning to the documentation to indicate the possible badness that
can happen.

Matt

On Mon, Apr 22, 2002 at 09:05:56PM -0400, Alexander Viro wrote:
> 
> 
> On Mon, 22 Apr 2002, Matthew Dharm wrote:
> 
> > Isn't the real problem here that we've got a "rogue" running around
> > removing things that we might be about to use?
> > 
> > Yes, I think that request_module() should indicate to the caller if
> > something "suitable" was found.  But I think having rmmod -a running around
> > sweeping things randomly is bad.
> > 
> > Perhaps what we need is a way to tell _how_long_ago_ the count on a module
> > last changed.  Thus, rmmod -a could decide to only remove modules that were
> > last used more than an hour ago, or somesuch.  Push the policy question into
> > userspace.
> 
> Still doesn't solve the problem.  And BTW, there are userland races of
> similar kind - foo.o depends on bar.o, modprobe loads bar.o, goes to look
> for foo.o and gets bar.o removed from under it.
> 
> The thing being, relying on time doesn't help - e.g. we might have modules
> on automounted volume and delays may be really long if the thing happens
> at time when load is high.

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

I'm just trying to think of a way to say "up yours" without getting fired.
					-- Stef
User Friendly, 10/8/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [RFC] race in request_module()

[Alexander Viro]

On Mon, 22 Apr 2002, Matthew Dharm wrote:

> The question then becomes one of how do I distinguish a race condition from
> a legitimate load/unload cycle?

> Isn't the problem here just the misuse of rmmod -a?  Perhaps we should
> attach a warning to the documentation to indicate the possible badness that
> can happen.

Not really.  _Any_ use of rmmod -a (i.e. unload stuff not in use) can
trigger that.

As for legitimate load/unload cycle... in this situation thing should be
considered busy.  It's that simple - what happens here is that we are asking
for module because we want to make it busy as soon as we get it.  Race
window is between the sys_create_module() from modprobe and try_inc_use_count()
in whatever wants it in the kernel or sys_init_module() for module depending
on it.

IOW, unload is _not_ legitimate here.

Re: [RFC] race in request_module()

[Keith Owens]

On Mon, 22 Apr 2002 20:49:40 -0400 (EDT), 
Alexander Viro <viro@math.psu.edu> wrote:
>	Looks like request_module() has quite a few problems:
>
>* there is no way to distinguish between failing modprobe and successful
>  one followed by rmmod -a (e.g. called by cron).  For one thing, we
>  don't pass exit value of modprobe to caller of request_module().

There is no such thing as a failing modprobe.  It either works and the
module is loaded or modprobe does not work and the module is not
loaded.  This excludes the case where a module oops during init, but
that is not what you are worried about.

When a module is loaded, it is marked !MOD_USED_ONCE.  An explicit
rmmod will get rid of the module but rmmod -a will not.  rmmod -a will
not remove a module unless __MOD_INC_USE_COUNT has been issued on the
module at least once, or the module is loaded to satisfy unresolved
symbols from another module.

Rusty and I have a completely new design for module loading and
unloading in 2.5, we believe it is race free.  I do not have time to
work on the new design until I have got kbuild 2.5 into the kernel.

Re: [RFC] race in request_module()

[Alexander Viro]

On Tue, 23 Apr 2002, Keith Owens wrote:

> There is no such thing as a failing modprobe.  It either works and the
> module is loaded or modprobe does not work and the module is not
> loaded.  This excludes the case where a module oops during init, but
> that is not what you are worried about.
> 
> When a module is loaded, it is marked !MOD_USED_ONCE.  An explicit
> rmmod will get rid of the module but rmmod -a will not.  rmmod -a will
> not remove a module unless __MOD_INC_USE_COUNT has been issued on the
> module at least once, or the module is loaded to satisfy unresolved
> symbols from another module.


Which is still racy - open()/close() bringing stuff from the same module
during the window in question and there we go.

IOW, echo </dev/foo will merrily set MOD_USED_ONCE.

Re: [RFC] race in request_module()

[Keith Owens]

On Mon, 22 Apr 2002 23:35:51 -0400 (EDT), 
Alexander Viro <viro@math.psu.edu> wrote:
>On Tue, 23 Apr 2002, Keith Owens wrote:
>> When a module is loaded, it is marked !MOD_USED_ONCE.  An explicit
>> rmmod will get rid of the module but rmmod -a will not.  rmmod -a will
>> not remove a module unless __MOD_INC_USE_COUNT has been issued on the
>> module at least once, or the module is loaded to satisfy unresolved
>> symbols from another module.
>
>
>Which is still racy - open()/close() bringing stuff from the same module
>during the window in question and there we go.
>
>IOW, echo </dev/foo will merrily set MOD_USED_ONCE.

Where is the race?

  open /dev/foo
    request_module(foo)
      load foo, mark !MOD_USED_ONCE.
    continue with open, MOD_INC_USE_COUNT(foo), mark MOD_USED_ONCE.
    return to use, module is locked down
  User space closes /dev/foo
    Release foo resources.
      MOD_DEC_USE_COUNT(foo)
        return to user space
  rmmod -a cleans up.  Nothing is using foo, it is removed.	

Re: [RFC] race in request_module()

[Alexander Viro]

On Tue, 23 Apr 2002, Keith Owens wrote:

>   open /dev/foo
>     request_module(foo)
>       load foo, mark !MOD_USED_ONCE.

Another process:
	open /dev/foo
		MOD_INC_USE_COUNT
		mark MOD_USED_ONCE
	close /dev/foo
		MOD_DEC_USE_COUNT
rmmod -a
	kills module

>     continue with open, MOD_INC_USE_COUNT(foo), mark MOD_USED_ONCE.
			  module not loaded

Re: [RFC] race in request_module()

[Keith Owens]

On Tue, 23 Apr 2002 14:09:21 -0400 (EDT), 
Alexander Viro <viro@math.psu.edu> wrote:
>
>
>On Tue, 23 Apr 2002, Keith Owens wrote:
>
>>   open /dev/foo
>>     request_module(foo)
>>       load foo, mark !MOD_USED_ONCE.
>
>Another process:
>	open /dev/foo
>		MOD_INC_USE_COUNT
>		mark MOD_USED_ONCE
>	close /dev/foo
>		MOD_DEC_USE_COUNT
>rmmod -a
>	kills module
>
>>     continue with open, MOD_INC_USE_COUNT(foo), mark MOD_USED_ONCE.
>			  module not loaded

You need two rmmod -a sweeps with no intervening activity on the module
before the module will be autocleaned.  See MOD_VISITED.  Unless the
first process hangs in the kernel for minutes, it will find the module
still present, use the module and cancel the rmmod -a status.

BTW, I do not disagree that module unloading in general is racy,
especially for ancillary data such as exception tables, unwind data,
timers, kernel threads etc.  Which is why Rusty and I plan to rewrite
module loading and unloading in 2.5.  I just do not see a race in the
area you are looking at.

Re: [RFC] race in request_module()

[Rusty Russell]

On Mon, 22 Apr 2002 20:49:40 -0400 (EDT)
Alexander Viro <viro@math.psu.edu> wrote:

> 	Looks like request_module() has quite a few problems:

Um, yes.

> * there is no way to distinguish between failing modprobe and successful
>   one followed by rmmod -a (e.g. called by cron).  For one thing, we
>   don't pass exit value of modprobe to caller of request_module().

But that's kind of the point.  You can *never* do:

	ptr = lookup("foo");
	if (!ptr) {
		if (request_module("mymod-foo") == 0)
			ptr = lookup("foo");
		else
			goto out;
	}

	... Assume ptr is non-null...

> * even if we would pass it, obvious attempt to cope with rmmod -a races
>   fails.  I.e. something like
> 
> 	while (object doesn't exist) {
> 		if (request_module(module_name) < 0)
> 			break;
> 	}
> 
>   would screw up for something like
> 
> mount -t floppy <whatever>
> 
>   since we would happily load floppy.o and look for fs type called "floppy".
>   And keep doing that forever, since floppy.o doesn't define any fs.

Yes, we don't try to cache failures, and you must *never* loop on request_module.

> * we could try to protect against rmmod -a by changing semantics of module
>   syscalls and modprobe(8).  Namely, let modprobe called by request_module()
>   pin the module(s) down and make request_module() (actually its caller)
>   decrement refcounts.  That would solve the problem, but we get another one:
>   how to find all modules pulled in by modprobe(8) and its children.
>   Notice that argument of request_module() doesn't help at all - it can have
>   nothing to name of module we load (block-major-2 -> floppy) and we could have
>   other modules grabbed by the same modprobe.

Yes, and modules pulled in indirectly (see "pre-install")...

> * we might try to pull the following trick: in sys_create_module() follow
>   ->parent until we step on request_module()-spawned task.  Then put the new
>   module on a list for that instance of request_module().  That would solve
>   the problem, but I'm not too happy about such solution - IMO it's ugly.
>   However, I don't see anything else...

I had some code to do this and threw it out.  It assumes alot about the nature
of modprobe (ie. won't get reparented to init).  Having a special inherited
"I am modprobe" flag in the task struct which is inherited across exec & fork,
and is checked in request_module is the "correct" way.  Barf.

> Comments?

<SIGH>.

Wanna get ambitious?  Replace all occurances of:
	ptr = find(xxx);
	if (!ptr && request_module(SOMENAME) == 0)
		ptr = find(xxx);

With a more generic global registration mechanism:
	/* Find in list, try loading module (sleeps) */
	void *find_feature(const char *, int);

	/* Static registration of feature */
	#define FEATURE(name, desc, feature) ...
	/* Dynamic registration of feature */
	int feature(const char *name, int desc, void *feature);
	void unfeature(const char *name, int desc);

Both module loading and the boot code gather and register all the
FEATUREs mentioned in the macro.  modprobe then loads by FEATURE,
not module name.

If we then go down the path that FreeBSD is, then we can have a
context during soft interrupts, allowing us to sleep almost anywhere.
This means we can, for example on receipt of a network packet:

	struct packet_type *ptype;
	ptype = find_feature("net-packet-type", skb->protocol);
	...

ie. As IPX packets come in, we load the ipx module.

This should give us the microkernel we're all waiting for!
Rusty.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

Re: [RFC] race in request_module()

[Pete Zaitcev]

> * there is no way to distinguish between failing modprobe and successful
>   one followed by rmmod -a (e.g. called by cron).  For one thing, we
>   don't pass exit value of modprobe to caller of request_module().

> Comments?

Please do not concern yourself with rmmod -a running from cron.
For one thing, it helpfuly removes your USB keyboard support.
I do not think any distributions install it anymore.

-- Pete