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

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

[Alexey Dobriyan]

One head-scratching session could be noticeably shorter with this patch...

Signed-off-by: Alexey Dobriyan <adobriyan@sw.ru>
---

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

--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2171,6 +2171,12 @@ sys_init_module(void __user *umod,
 		wake_up(&module_wq);
 		return ret;
 	}
+	if (ret > 0) {
+		printk(KERN_WARNING "%s: '%s'->init suspiciously returned %d\n"
+		       KERN_WARNING "%s: loading module anyway...\n",
+		       __func__, mod->name, ret,
+		       __func__);
+	}
 
 	/* Now it's a first class citizen! */
 	mutex_lock(&module_mutex);

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

[Andrew Morton]

On Mon, 4 Feb 2008 18:42:15 +0300
Alexey Dobriyan <adobriyan@sw.ru> wrote:

> One head-scratching session could be noticeably shorter with this patch...
> 

Sorry, this is not an adequate description of why you think this patch
should be merged.

> ---
> 
>  kernel/module.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2171,6 +2171,12 @@ sys_init_module(void __user *umod,
>  		wake_up(&module_wq);
>  		return ret;
>  	}
> +	if (ret > 0) {
> +		printk(KERN_WARNING "%s: '%s'->init suspiciously returned %d\n"
> +		       KERN_WARNING "%s: loading module anyway...\n",
> +		       __func__, mod->name, ret,
> +		       __func__);
> +	}
>  
>  	/* Now it's a first class citizen! */
>  	mutex_lock(&module_mutex);

So we add a debug statement to detect a module init function which returns
positive non-zero values, which module init functions are not supposed to
do.

Fair enough.  But a) the printk could state that more clearly and b) there
should be a comment in the code so that a developer (at whom this patch is
targetted) can go in and find out exactly what he did wrong.

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

[Rusty Russell]

On Tuesday 05 February 2008 02:42:15 Alexey Dobriyan wrote:
> One head-scratching session could be noticeably shorter with this patch...
>
> Signed-off-by: Alexey Dobriyan <adobriyan@sw.ru>

If we want to prevent > 0 returns, let's just BUG_ON().

Thanks,
Rusty.

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

[Andrew Morton]

On Tue, 5 Feb 2008 14:43:31 +1100 Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Tuesday 05 February 2008 02:42:15 Alexey Dobriyan wrote:
> > One head-scratching session could be noticeably shorter with this patch...
> >
> > Signed-off-by: Alexey Dobriyan <adobriyan@sw.ru>
> 
> If we want to prevent > 0 returns, let's just BUG_ON().
> 

That risks killing previously-working setups.  WARN_ON is sufficient.

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

[Rusty Russell]

On Tuesday 05 February 2008 14:53:18 Andrew Morton wrote:
> On Tue, 5 Feb 2008 14:43:31 +1100 Rusty Russell <rusty@rustcorp.com.au> 
wrote:
> > On Tuesday 05 February 2008 02:42:15 Alexey Dobriyan wrote:
> > > One head-scratching session could be noticeably shorter with this
> > > patch...
> > >
> > > Signed-off-by: Alexey Dobriyan <adobriyan@sw.ru>
> >
> > If we want to prevent > 0 returns, let's just BUG_ON().
>
> That risks killing previously-working setups.  WARN_ON is sufficient.

I disagree.  WARN_ON is useful for developers, but they can handle BUG_ON, 
too.

If we were in freeze, I'd say WARN_ON.

Even better, audit them all, then BUG_ON.  Alexey?
Rusty.

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

[Andrew Morton]

On Tue, 5 Feb 2008 17:08:37 +1100 Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Tuesday 05 February 2008 14:53:18 Andrew Morton wrote:
> > On Tue, 5 Feb 2008 14:43:31 +1100 Rusty Russell <rusty@rustcorp.com.au> 
> wrote:
> > > On Tuesday 05 February 2008 02:42:15 Alexey Dobriyan wrote:
> > > > One head-scratching session could be noticeably shorter with this
> > > > patch...
> > > >
> > > > Signed-off-by: Alexey Dobriyan <adobriyan@sw.ru>
> > >
> > > If we want to prevent > 0 returns, let's just BUG_ON().
> >
> > That risks killing previously-working setups.  WARN_ON is sufficient.
> 
> I disagree.  WARN_ON is useful for developers, but they can handle BUG_ON, 
> too.

For developers, BUG_ON has zero benefit relative to WARN_ON.

For non-developers, BUG_ON has large disadvantages relative to WARN_ON.

It's a no-brainer.

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

[Rusty Russell]

On Tuesday 05 February 2008 17:24:57 Andrew Morton wrote:
> On Tue, 5 Feb 2008 17:08:37 +1100 Rusty Russell <rusty@rustcorp.com.au> 
wrote:
> > On Tuesday 05 February 2008 14:53:18 Andrew Morton wrote:
> > > That risks killing previously-working setups.  WARN_ON is sufficient.
> >
> > I disagree.  WARN_ON is useful for developers, but they can handle
> > BUG_ON, too.
>
> For developers, BUG_ON has zero benefit relative to WARN_ON.
>
> For non-developers, BUG_ON has large disadvantages relative to WARN_ON.
>
> It's a no-brainer.

For non-developers, WARN_ON is a noop.

For developers, WARN_ON is often a noop.

BUG_ON() will make us fix it in return for short-term pain.  WARN_ON() wont, 
in return for less pain.  It's mildly better than nothing, but not worth the 
patch.

Rusty.

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

[Andrew Morton]

On Wed, 6 Feb 2008 09:48:10 +1100
Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Tuesday 05 February 2008 17:24:57 Andrew Morton wrote:
> > On Tue, 5 Feb 2008 17:08:37 +1100 Rusty Russell <rusty@rustcorp.com.au> 
> wrote:
> > > On Tuesday 05 February 2008 14:53:18 Andrew Morton wrote:
> > > > That risks killing previously-working setups.  WARN_ON is sufficient.
> > >
> > > I disagree.  WARN_ON is useful for developers, but they can handle
> > > BUG_ON, too.
> >
> > For developers, BUG_ON has zero benefit relative to WARN_ON.
> >
> > For non-developers, BUG_ON has large disadvantages relative to WARN_ON.
> >
> > 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.

> For developers, WARN_ON is often a noop.

And from developers.

> 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!

>  WARN_ON() wont, 
> in return for less pain.  It's mildly better than nothing, but not worth the 
> patch.

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

[Rusty Russell]

On Wednesday 06 February 2008 10:37:52 Andrew Morton wrote:
> On Wed, 6 Feb 2008 09:48:10 +1100
>
> Rusty Russell <rusty@rustcorp.com.au> 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.

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.  But it is still an admission of ignorance.

Cheers,
Rusty.

[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@rustcorp.com.au> 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@gmail.com>
---

 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);

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

[Rusty Russell]

On Monday 11 February 2008 09:09:06 Alexey Dobriyan wrote:
> On Wed, Feb 06, 2008 at 05:55:26PM +1100, Rusty Russell wrote:
> > 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.

I'm not convinced that people are introducing bugs that fast :)

> > 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.

Sure, I'd start by writing some filters for all the easy cases.  It'd probably 
only take a day to do them all.

The thing is, every time I do an audit like this, I find all kinds of things 
to fix; it's not actually a useless exercise.

> > 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.

Sure, I've put it in my tree for the moment.

Thanks,
Rusty.