Module Observations

Module Observations

[Srivatsa Vaddagiri]

Hi,
	I was going thr' module code and made some observations:

1. sys_init_module drops the module_mutex semaphore
   before calling mod->init() function and later
   reacquires it. After reacquiring, it marks
   the module state as MODULE_STATE_LIVE.

   In the window when mod->init() function is running,
   isn't it possible that sys_delete_module (running
   on some other CPU and trying to remove the _same_ module)
   acquires the module_mutex sem and marks the module
   state as MODULE_STATE_GOING?

   Shouldn't sys_init_module check for
   that possibility when it reacquires the semaphore after
   calling mod->init function?

--- module.c.org        Fri Jan  2 18:37:54 2004
+++ module.c    Fri Jan  2 18:38:57 2004
@@ -1750,7 +1750,8 @@

        /* Now it's a first class citizen! */
        down(&module_mutex);
-       mod->state = MODULE_STATE_LIVE;
+       if (likely(mod->state != MODULE_STATE_GOING))
+               mod->state = MODULE_STATE_LIVE;
        /* Drop initial reference. */
        module_put(mod);
        module_free(mod, mod->module_init);


  This off-course means that you are trying to insmod and rmmod 
  the same module simultaneously from different CPUs and hence
  may not be practical.

2. try_module_get() and module_put()

	try_module_get increments the local cpu's ref count for the module 
   and module_put decrements it.

   Is it required that the caller call both these functions from the same CPU?
   Otherwise, the total refcount for the module will be non-zero!



-- 


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

Re: Module Observations

[Rusty Russell]

On Fri, 2 Jan 2004 18:55:09 +0530
Srivatsa Vaddagiri <vatsa@in.ibm.com> wrote:

> Hi,
> 	I was going thr' module code and made some observations:
> 
> 1. sys_init_module drops the module_mutex semaphore
>    before calling mod->init() function and later
>    reacquires it. After reacquiring, it marks
>    the module state as MODULE_STATE_LIVE.
> 
>    In the window when mod->init() function is running,
>    isn't it possible that sys_delete_module (running
>    on some other CPU and trying to remove the _same_ module)
>    acquires the module_mutex sem and marks the module
>    state as MODULE_STATE_GOING?
> 
>    Shouldn't sys_init_module check for
>    that possibility when it reacquires the semaphore after
>    calling mod->init function?

Good catch.  The module removal should refuse to remove it without --force.

I opened this hole when I dropped the sem around mod->init() (because
some modules load other modules in their init routine).

Andrew, please apply patch below.

> 2. try_module_get() and module_put()
> 
> 	try_module_get increments the local cpu's ref count for the module 
>    and module_put decrements it.
> 
>    Is it required that the caller call both these functions from the same CPU?
>    Otherwise, the total refcount for the module will be non-zero!

No, it's OK.  We only care about the *total* being zero.

Thanks,
Rusty.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

Name: Prevent Removal of Module During Init
Author: Rusty Russell
Status: Trivial

D: Vatsa spotted this: you can remove a module while it's being
D: initialized, and that will be bad.  Hole was opened when I dropped
D: the sem around the init routine (which can probe for other
D: modules).

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22325-linux-2.6.1-rc1/kernel/module.c .22325-linux-2.6.1-rc1.updated/kernel/module.c
--- .22325-linux-2.6.1-rc1/kernel/module.c	2003-11-24 15:42:33.000000000 +1100
+++ .22325-linux-2.6.1-rc1.updated/kernel/module.c	2004-01-03 15:59:54.000000000 +1100
@@ -687,8 +687,8 @@ sys_delete_module(const char __user *nam
 		goto out;
 	}
 
-	/* Already dying? */
-	if (mod->state == MODULE_STATE_GOING) {
+	/* Doing init or already dying? */
+	if (mod->state != MODULE_STATE_LIVE) {
 		/* FIXME: if (force), slam module count and wake up
                    waiter --RR */
 		DEBUGP("%s already dying\n", mod->name);

Re: Module Observations

[Rusty Russell]

On Fri, 2 Jan 2004 18:55:09 +0530
Srivatsa Vaddagiri <vatsa@in.ibm.com> wrote:

> Hi,
> 	I was going thr' module code and made some observations:
> 
> 1. sys_init_module drops the module_mutex semaphore
>    before calling mod->init() function and later
>    reacquires it. After reacquiring, it marks
>    the module state as MODULE_STATE_LIVE.
> 
>    In the window when mod->init() function is running,
>    isn't it possible that sys_delete_module (running
>    on some other CPU and trying to remove the _same_ module)
>    acquires the module_mutex sem and marks the module
>    state as MODULE_STATE_GOING?
> 
>    Shouldn't sys_init_module check for
>    that possibility when it reacquires the semaphore after
>    calling mod->init function?

Good catch.  The module removal should refuse to remove it without --force.

I opened this hole when I dropped the sem around mod->init() (because
some modules load other modules in their init routine).

Andrew, please apply patch below.

> 2. try_module_get() and module_put()
> 
> 	try_module_get increments the local cpu's ref count for the module 
>    and module_put decrements it.
> 
>    Is it required that the caller call both these functions from the same CPU?
>    Otherwise, the total refcount for the module will be non-zero!

No, it's OK.  We only care about the *total* being zero.

Thanks,
Rusty.
-- 
   there are those who do and those who hang on and you don't see too
   many doers quoting their contemporaries.  -- Larry McVoy

Name: Prevent Removal of Module During Init
Author: Rusty Russell
Status: Trivial

D: Vatsa spotted this: you can remove a module while it's being
D: initialized, and that will be bad.  Hole was opened when I dropped
D: the sem around the init routine (which can probe for other
D: modules).

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .22325-linux-2.6.1-rc1/kernel/module.c .22325-linux-2.6.1-rc1.updated/kernel/module.c
--- .22325-linux-2.6.1-rc1/kernel/module.c	2003-11-24 15:42:33.000000000 +1100
+++ .22325-linux-2.6.1-rc1.updated/kernel/module.c	2004-01-03 15:59:54.000000000 +1100
@@ -687,8 +687,8 @@ sys_delete_module(const char __user *nam
 		goto out;
 	}
 
-	/* Already dying? */
-	if (mod->state == MODULE_STATE_GOING) {
+	/* Doing init or already dying? */
+	if (mod->state != MODULE_STATE_LIVE) {
 		/* FIXME: if (force), slam module count and wake up
                    waiter --RR */
 		DEBUGP("%s already dying\n", mod->name);