[PATCH] fixup loop blkdev, add module_get

[PATCH] fixup loop blkdev, add module_get

[Jeff Garzik]

Sometimes, we are absolutely certain that we have at least one module
reference "locked open" for us.  Loop is an example of such a case:  the
set-fd and clear-fd struct block_device_operations ioctls already have a
module reference from simply the block device being opened.

Therefore, we can just unconditionally increment the module refcount.
I added module_get to do this.

Implementing try_module_get in terms of module_get is left as an
exercise for the maintainer :)  I am too lazy to investigate whether it
is ok for try_module_get to call module_is_live outside of the
get_cpu/put_cpu pair, which is a prereq for try_module_get using
module_get.

Patch against latest Linus BK tree follows...  this also eliminates the
"deprecated" warnings for the loop device.



===== drivers/block/loop.c 1.78 vs edited =====
--- 1.78/drivers/block/loop.c	Thu Dec  5 10:52:06 2002
+++ edited/drivers/block/loop.c	Sat Jan 11 22:45:54 2003
@@ -642,7 +642,7 @@
 	int		lo_flags = 0;
 	int		error;
 
-	MOD_INC_USE_COUNT;
+	module_get(THIS_MODULE);
 
 	error = -EBUSY;
 	if (lo->lo_state != Lo_unbound)
@@ -742,7 +742,7 @@
  out_putf:
 	fput(file);
  out:
-	MOD_DEC_USE_COUNT;
+	module_put(THIS_MODULE);
 	return error;
 }
 
@@ -814,7 +814,7 @@
 	filp->f_dentry->d_inode->i_mapping->gfp_mask = gfp;
 	lo->lo_state = Lo_unbound;
 	fput(filp);
-	MOD_DEC_USE_COUNT;
+	module_put(THIS_MODULE);
 	return 0;
 }
 
===== include/linux/module.h 1.38 vs edited =====
--- 1.38/include/linux/module.h	Wed Jan  8 08:19:44 2003
+++ edited/include/linux/module.h	Sat Jan 11 22:43:20 2003
@@ -249,6 +249,20 @@
 #define local_dec(x) atomic_dec(x)
 #endif
 
+/* bump a module's refcount.
+ *
+ * called only when we are absolutely certain that
+ * module != NULL, and the locking / call chain
+ * guarantees that the module won't disappear out
+ * from underneath us.
+ */
+static inline void module_get(struct module *module)
+{
+	unsigned int cpu = get_cpu();
+	local_inc(&module->ref[cpu].count);
+	put_cpu();
+}
+
 static inline int try_module_get(struct module *module)
 {
 	int ret = 1;
@@ -277,6 +291,7 @@
 }
 
 #else /*!CONFIG_MODULE_UNLOAD*/
+static inline void module_get(struct module *module) {}
 static inline int try_module_get(struct module *module)
 {
 	return !module || module_is_live(module);

Re: [PATCH] fixup loop blkdev, add module_get

[Rusty Russell]

In message <20030112035620.GA25648@gtf.org> you write:
> Sometimes, we are absolutely certain that we have at least one module
> reference "locked open" for us.  Loop is an example of such a case:  the
> set-fd and clear-fd struct block_device_operations ioctls already have a
> module reference from simply the block device being opened.
> 
> Therefore, we can just unconditionally increment the module refcount.
> I added module_get to do this.

Hi Jeff,

	We may yet want such a primitive, but I've been resisting it
for the moment.

	Firstly, because it's a very specialized and rare case which
lends itself to being abused, and secondly because if I "rmmod --wait"
the module, then such operations which try to hold the module in place
*should* fail.  Not doing so is impolite, at least.

	Sure, it's a fairly strange corner case, but that argument
cuts both ways.

I prefer this patch (-EBUSY is a little unusual, but as clear as any).

Thoughts?
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5-bk/drivers/block/loop.c working-2.5-bk-loop/drivers/block/loop.c
--- linux-2.5-bk/drivers/block/loop.c	2003-01-02 12:45:18.000000000 +1100
+++ working-2.5-bk-loop/drivers/block/loop.c	2003-01-13 11:49:21.000000000 +1100
@@ -642,7 +642,9 @@ static int loop_set_fd(struct loop_devic
 	int		lo_flags = 0;
 	int		error;
 
-	MOD_INC_USE_COUNT;
+	if (!try_module_get(THIS_MODULE))
+		/* I'm going away: pretend I'm not here. */
+		return -EBUSY;
 
 	error = -EBUSY;
 	if (lo->lo_state != Lo_unbound)
@@ -742,7 +744,7 @@ static int loop_set_fd(struct loop_devic
  out_putf:
 	fput(file);
  out:
-	MOD_DEC_USE_COUNT;
+	module_put(THIS_MODULE);
 	return error;
 }
 
@@ -814,7 +816,9 @@ static int loop_clr_fd(struct loop_devic
 	filp->f_dentry->d_inode->i_mapping->gfp_mask = gfp;
 	lo->lo_state = Lo_unbound;
 	fput(filp);
-	MOD_DEC_USE_COUNT;
+
+	/* This won't drop to zero, since we're inside an ioctl. */
+	module_put(THIS_MODULE);
 	return 0;
 }
 

Re: [PATCH] fixup loop blkdev, add module_get

[Jeff Garzik]

On Mon, Jan 13, 2003 at 11:55:47AM +1100, Rusty Russell wrote:
> In message <20030112035620.GA25648@gtf.org> you write:
> > Sometimes, we are absolutely certain that we have at least one module
> > reference "locked open" for us.  Loop is an example of such a case:  the
> > set-fd and clear-fd struct block_device_operations ioctls already have a
> > module reference from simply the block device being opened.
> > 
> > Therefore, we can just unconditionally increment the module refcount.
> > I added module_get to do this.
> 
> Hi Jeff,
> 
> 	We may yet want such a primitive, but I've been resisting it
> for the moment.
> 
> 	Firstly, because it's a very specialized and rare case which
> lends itself to being abused, and secondly because if I "rmmod --wait"
> the module, then such operations which try to hold the module in place
> *should* fail.  Not doing so is impolite, at least.

Eh...  You are trying to chase infinity with 'rmmod --wait'.

More below...


> diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5-bk/drivers/block/loop.c working-2.5-bk-loop/drivers/block/loop.c
> --- linux-2.5-bk/drivers/block/loop.c	2003-01-02 12:45:18.000000000 +1100
> +++ working-2.5-bk-loop/drivers/block/loop.c	2003-01-13 11:49:21.000000000 +1100
> @@ -642,7 +642,9 @@ static int loop_set_fd(struct loop_devic
>  	int		lo_flags = 0;
>  	int		error;
>  
> -	MOD_INC_USE_COUNT;
> +	if (!try_module_get(THIS_MODULE))
> +		/* I'm going away: pretend I'm not here. */
> +		return -EBUSY;
>  
>  	error = -EBUSY;
>  	if (lo->lo_state != Lo_unbound)

I disagree:

1) we do not prevent root from shooting themselves in the foot,

2) moreover we do not prevent them from doing something that may be
perfectly reasonable,

3) and this kind of code just adds error handling for no reason, when
_not_ handling the error keeps the code more clean.

In general this is just caring way too much about an obscure corner
case.  Is the increased complexity of error handling when we _know_ the
refcnt is locked for worth it?

Note that Linus turned off the 'deprecated' warning because MOD.*COUNT
users are just too frequent, still.

	Jeff

Re: [PATCH] fixup loop blkdev, add module_get

[Rusty Russell]

In message <20030113020325.GA18756@gtf.org> you write:
> On Mon, Jan 13, 2003 at 11:55:47AM +1100, Rusty Russell wrote:
> > In message <20030112035620.GA25648@gtf.org> you write:
> > > Sometimes, we are absolutely certain that we have at least one module
> > > reference "locked open" for us.  Loop is an example of such a case:  the
> > > set-fd and clear-fd struct block_device_operations ioctls already have a
> > > module reference from simply the block device being opened.
> > > 
> > > Therefore, we can just unconditionally increment the module refcount.
> > > I added module_get to do this.
> > 
> > Hi Jeff,
> > 
> > 	We may yet want such a primitive, but I've been resisting it
> > for the moment.
> > 
> > 	Firstly, because it's a very specialized and rare case which
> > lends itself to being abused, and secondly because if I "rmmod --wait"
> > the module, then such operations which try to hold the module in place
> > *should* fail.  Not doing so is impolite, at least.
> 
> Eh...  You are trying to chase infinity with 'rmmod --wait'.

No, you are trying to remove something and you want to chase down and
kill the users, scripts, whatever.  It guarantees that no new users
will access the module.

> I disagree:
> 
> 1) we do not prevent root from shooting themselves in the foot,

I don't understand this point.

> 2) moreover we do not prevent them from doing something that may be
> perfectly reasonable,

Nor this one, which seems to bethe same.

> 3) and this kind of code just adds error handling for no reason, when
> _not_ handling the error keeps the code more clean.

No, the reason is simple: the admin has said they want the damn module
removed.  They've *told* you what they want.  Why do you want to
disobey them?  8)

> In general this is just caring way too much about an obscure corner
> case.  Is the increased complexity of error handling when we _know_ the
> refcnt is locked for worth it?

Is the increased complexity of another primitive for "you know you
have a refcount" worth it? 8)

If there were 10 of these cases, sure, a __try_module_get() makes
sense: IMHO this is one of those areas on which intelligent people can
disagree, I think.

> Note that Linus turned off the 'deprecated' warning because MOD.*COUNT
> users are just too frequent, still.

Note that I didn't put the damn thing in there 8)

Hope he turned them back into macros, so the __unsafe runtime warning
doesn't report "module.h".

Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: [PATCH] fixup loop blkdev, add module_get

[Roman Zippel]

Hi,

Rusty Russell wrote:

> > 1) we do not prevent root from shooting themselves in the foot,
> 
> I don't understand this point.

Something to remember here: When we kill processes with SIGKILL, do we
risk kernel stability? E.g. do we turn TASK_UNINTERRUPTIBLE into
TASK_INTERRUPTIBLE, do we allow to kill init or do we leave resources
behind?

> > 3) and this kind of code just adds error handling for no reason, when
> > _not_ handling the error keeps the code more clean.
> 
> No, the reason is simple: the admin has said they want the damn module
> removed.  They've *told* you what they want.  Why do you want to
> disobey them?  8)

How do you want to make this mechanism halfway safe to use? Did you
check that all modules can still be deconfigured after you put them into
the going state via the wait option?
Both the wait and the force option are extremely dangerous options. You
are trying to implement something behind the modules back, what can only
go wrong as soon as the module becomes slightly more complex. If you
want to force the removal of a module, you should least attempt to keep
it safe and this is not possible without the help of the module. A
global don't-use switch is a worse idea than the global module count, if
it's forced onto the modules.
I'm really curious when we get the first bug reports from people, who
"only" unloaded a module. :(

bye, Roman