[PATCH v2] Whine about suspicious return values from module's ->init() hook

[Alexey Dobriyan]

On Wed, Feb 06, 2008 at 05:55:26PM +1100, Rusty Russell wrote:
> On Wednesday 06 February 2008 10:37:52 Andrew Morton wrote:
> > On Wed, 6 Feb 2008 09:48:10 +1100
> >
> > Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
> > > > It's a no-brainer.
> > >
> > > For non-developers, WARN_ON is a noop.
> >
> > Oh..  Rusty.  The mailing list and bugzilla are *full* of WARN_ON reports
> > from testers.  Your statement is empirically wrong.
> 
> My apologies.  I had extrapolated from my own behaviour: I don't notice 
> WARN_ON unless something else goes wrong to make me look in the logs.
> 
> > > BUG_ON() will make us fix it in return for short-term pain.
> >
> > Pain to our users and testers.  People upon whom we are very dependent and
> > to whom we are hugely indebted.  People who I have to spend a lot of time
> > defending from the likes of you!
> 
> I think you misunderstand.  I proposed that we audit all the code before such 
> a change.  We shouldn't do *anything* until we can estimate the impact this 
> change will have.

With such rate of changes, good luck doing that.

Good example is sysctl checks. There was initial influx of reports and
they were fixed and there were no sysctl reports recently.

> Our users deserve better than "I don't know if this will break anything so I 
> used WARN_ON".  They deserve "we have confidence that this change won't break 
> any existing code".
> 
> Now, if an audit is impractical or unreliable, we are better off with a 
> WARN_ON.

It's impractical as in it's extremely boring to read every modules init
function and propagate return values in mind.

> But it is still an admission of ignorance.

I love BUG_ON and BUILD_BUG_ON very much but on such scale you can't
just throw them in.

Here goes version 2 with improved changelog. Let's put in -mm and see
what happens, then put it in mainline and see what happens.


[PATCH v2] Whine about suspicious return values from module's ->init() hook

Return value convention of module's init functions is 0/-E. Sometimes, e.g.
during forward-porting mistakes happen and buggy module created, where result
of comparison "workqueue != NULL" is propagated all the way up to
sys_init_module. What happens is that some other module created workqueue in
question, our module created it again and module was successfully loaded.

Or it could be some other bug.

Let's make such mistakes much more visible. In retrospective, such messages
would noticeably shorten some of my head-scratching sessions.

Note, that dump_stack() is just a way to get attention from user.
Sample message:

sys_init_module: 'foo'->init suspiciously returned 1, it should follow 0/-E convention
sys_init_module: loading module anyway...
Pid: 4223, comm: modprobe Not tainted 2.6.24-25f666300625d894ebe04bac2b4b3aadb907c861 #5

Call Trace:
 [<ffffffff80254b05>] sys_init_module+0xe5/0x1d0
 [<ffffffff8020b39b>] system_call_after_swapgs+0x7b/0x80

Signed-off-by: Alexey Dobriyan <adobriyan@xxxxxxxxx>
---

 kernel/module.c |    8 ++++++++
 1 file changed, 8 insertions(+)

--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2174,6 +2174,14 @@ sys_init_module(void __user *umod,
 		wake_up(&module_wq);
 		return ret;
 	}
+	if (ret > 0) {
+		printk(KERN_WARNING "%s: '%s'->init suspiciously returned %d, "
+				    "it should follow 0/-E convention\n"
+		       KERN_WARNING "%s: loading module anyway...\n",
+		       __func__, mod->name, ret,
+		       __func__);
+		dump_stack();
+	}
 
 	/* Now it's a first class citizen! */
 	mutex_lock(&module_mutex);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/