[PATCH] init: Do not warn on non-zero initcall return

[Steven Rostedt]

Commit f91eb62f71 "init: scream bloody murder if interrupts are enabled
too early" added three new warnings. The first two seemed reasonable,
but the third included a warning when an initcall returned non-zero.
Although, the third WARN() does include an imbalanced preempt disabled,
or irqs disable, it shouldn't warn if it only had an initcall that just
returns non-zero.

Link: http://lkml.kernel.org/r/2013050113...@phenom.dumpdata.com

Reported-by: Konrad Rzeszutek Wilk <konra...@oracle.com>
Signed-off-by: Steven Rostedt <ros...@goodmis.org>

diff --git a/init/main.c b/init/main.c
index bea1287..f0106bf 100644
--- a/init/main.c
+++ b/init/main.c
@@ -678,6 +678,7 @@ int __init_or_module do_one_initcall(initcall_t fn)
{
int count = preempt_count();
int ret;
+ bool warn = false;

if (initcall_debug)
ret = do_one_initcall_debug(fn);
@@ -692,12 +693,17 @@ int __init_or_module do_one_initcall(initcall_t fn)
if (preempt_count() != count) {
strlcat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
preempt_count() = count;
+ warn = true;
}
if (irqs_disabled()) {
strlcat(msgbuf, "disabled interrupts ", sizeof(msgbuf));
local_irq_enable();
+ warn = true;
+ }
+ if (msgbuf[0]) {
+ pr_err("initcall %pF returned with %s\n", fn, msgbuf);
+ WARN_ON(warn);
}
- WARN(msgbuf[0], "initcall %pF returned with %s\n", fn, msgbuf);

return ret;
}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Linus Torvalds]

On Wed, May 1, 2013 at 8:02 AM, Steven Rostedt <ros...@goodmis.org> wrote:
> Commit f91eb62f71 "init: scream bloody murder if interrupts are enabled
> too early" added three new warnings. The first two seemed reasonable,
> but the third included a warning when an initcall returned non-zero.
> Although, the third WARN() does include an imbalanced preempt disabled,
> or irqs disable, it shouldn't warn if it only had an initcall that just
> returns non-zero.

Ugh. Sorry, but this patch just looks stupid.

It seems that the right thing to do is to just remove the whole crappy

if (ret && ret != -ENODEV && initcall_debug)
sprintf(msgbuf, "error code %d ", ret);

thing entirely, since it's moronic to add that error code printout
anyway, since if initcall_debug is set, we already do a much *better*
job earlier with the whole

pr_debug("initcall %pF returned %d after %lld usecs\n",
fn, ret, duration);

printout in do_one_initcall_debug(). That will then fix the WARN()
issue automatically.

Hmm?

Linus

[Steven Rostedt]

On Wed, 2013-05-01 at 10:11 -0700, Linus Torvalds wrote:
> On Wed, May 1, 2013 at 8:02 AM, Steven Rostedt <ros...@goodmis.org> wrote:
> > Commit f91eb62f71 "init: scream bloody murder if interrupts are enabled
> > too early" added three new warnings. The first two seemed reasonable,
> > but the third included a warning when an initcall returned non-zero.
> > Although, the third WARN() does include an imbalanced preempt disabled,
> > or irqs disable, it shouldn't warn if it only had an initcall that just
> > returns non-zero.
>
> Ugh. Sorry, but this patch just looks stupid.
>
> It seems that the right thing to do is to just remove the whole crappy
>
> if (ret && ret != -ENODEV && initcall_debug)
> sprintf(msgbuf, "error code %d ", ret);
>
> thing entirely, since it's moronic to add that error code printout
> anyway, since if initcall_debug is set, we already do a much *better*
> job earlier with the whole
>
> pr_debug("initcall %pF returned %d after %lld usecs\n",
> fn, ret, duration);
>
> printout in do_one_initcall_debug(). That will then fix the WARN()
> issue automatically.

Heh, I didn't even notice that it only prints if initcall_debug was
enabled.

OK, I'll just remove that part of the code.

-- Steve

[Steven Rostedt]

Commit f91eb62f71 "init: scream bloody murder if interrupts are enabled
too early" added three new warnings. The first two seemed reasonable,
but the third included a warning when an initcall returned non-zero.
Although, the third WARN() does include an imbalanced preempt disabled,
or irqs disable, it shouldn't warn if it only had an initcall that just
returns non-zero.

In fact, according to Linus, it shouldn't print at all. As it only
prints with initcall_debug set, and that already shows enough
information to fix things.

Link: http://lkml.kernel.org/r/CA+55aFzaBC5SFi7=F2mfm+KWY5qTsBmOq...@mail.gmail.com

Suggested-by: Linus Torvalds <torv...@linux-foundation.org>

Reported-by: Konrad Rzeszutek Wilk <konra...@oracle.com>
Signed-off-by: Steven Rostedt <ros...@goodmis.org>

diff --git a/init/main.c b/init/main.c

index bea1287..ceed17a 100644
--- a/init/main.c
+++ b/init/main.c
@@ -686,11 +686,8 @@ int __init_or_module do_one_initcall(initcall_t fn)

msgbuf[0] = 0;

- if (ret && ret != -ENODEV && initcall_debug)
- sprintf(msgbuf, "error code %d ", ret);
-
if (preempt_count() != count) {
- strlcat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
+ sprintf(msgbuf, "preemption imbalance ");
preempt_count() = count;
}
if (irqs_disabled()) {

[Geert Uytterhoeven]

snprintf(), please?

JFYI, the v3.9 version already used up all of msgbuf[] in the
worst-case scenario,
and it did have the strlcat() as a parachute:

$ echo -n "error code 1234567890 preemption imbalance disabled
interrupts " | wc -c
63
$

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[Steven Rostedt]

Why? The msgbuf is 64 bytes, this is the first occurrence and
"preemption imbalance " is much less than 64 bytes.

>
> JFYI, the v3.9 version already used up all of msgbuf[] in the
> worst-case scenario,
> and it did have the strlcat() as a parachute:

Right, the other users had strlcat as they were not the first user and
add onto the string.

-- Steve

>
> $ echo -n "error code 1234567890 preemption imbalance disabled
> interrupts " | wc -c
> 63
> $

[Geert Uytterhoeven]

On Thu, May 2, 2013 at 5:17 PM, Steven Rostedt <ros...@goodmis.org> wrote:
> On Thu, 2013-05-02 at 09:43 +0200, Geert Uytterhoeven wrote:
>> On Wed, May 1, 2013 at 7:35 PM, Steven Rostedt <ros...@goodmis.org> wrote:
>> > --- a/init/main.c
>> > +++ b/init/main.c
>> > @@ -686,11 +686,8 @@ int __init_or_module do_one_initcall(initcall_t fn)
>> >
>> > msgbuf[0] = 0;
>> >
>> > - if (ret && ret != -ENODEV && initcall_debug)
>> > - sprintf(msgbuf, "error code %d ", ret);
>> > -
>> > if (preempt_count() != count) {
>> > - strlcat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
>> > + sprintf(msgbuf, "preemption imbalance ");
>>
>> snprintf(), please?
>
> Why? The msgbuf is 64 bytes, this is the first occurrence and
> "preemption imbalance " is much less than 64 bytes.

The day after tomorrow, someone will modify the code, and cause a buffer
overflow.

I'm actually surprised (v)sprintf() is not tagged __deprecated.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[Steven Rostedt]

On Thu, 2013-05-02 at 20:33 +0200, Geert Uytterhoeven wrote:
> On Thu, May 2, 2013 at 5:17 PM, Steven Rostedt <ros...@goodmis.org> wrote:
> > On Thu, 2013-05-02 at 09:43 +0200, Geert Uytterhoeven wrote:
> >> On Wed, May 1, 2013 at 7:35 PM, Steven Rostedt <ros...@goodmis.org> wrote:
> >> > --- a/init/main.c
> >> > +++ b/init/main.c
> >> > @@ -686,11 +686,8 @@ int __init_or_module do_one_initcall(initcall_t fn)
> >> >
> >> > msgbuf[0] = 0;
> >> >
> >> > - if (ret && ret != -ENODEV && initcall_debug)
> >> > - sprintf(msgbuf, "error code %d ", ret);
> >> > -
> >> > if (preempt_count() != count) {
> >> > - strlcat(msgbuf, "preemption imbalance ", sizeof(msgbuf));
> >> > + sprintf(msgbuf, "preemption imbalance ");
> >>
> >> snprintf(), please?
> >
> > Why? The msgbuf is 64 bytes, this is the first occurrence and
> > "preemption imbalance " is much less than 64 bytes.
>
> The day after tomorrow, someone will modify the code, and cause a buffer
> overflow.
>
> I'm actually surprised (v)sprintf() is not tagged __deprecated.
>

I actually did think for a second in adding that. Not sure why I didn't.
Probably just because the original code didn't do that.

But I'll let that clean up come another day by someone else.

-- Steve