Re: [PATCH] Magic SysRq loglevel fix.

[Crutcher Dunnavant]

++ 21/09/01 18:25 -0700 - Randy.Dunlap:
> Crutcher Dunnavant wrote:
> > 
> > Attached is a fix for this part of the sysrq code.
> > 
> > ++ 21/09/01 17:08 -0400 - Crutcher Dunnavant:
> > > ++ 19/09/01 08:56 -0700 - Randy.Dunlap:
> > > > It always sets console_loglevel and then restores
> > > > console_loglevel from orig_log_level, so Alt+SysRq+#
> > > > handling is severely broken.
> > > >
> > > > If someone (Crutcher ?) wants to patch it, that's fine.
> > > > If I patched it, I would just add a
> > > >   next_loglevel = -1;
> > > > at the beginning of __handle_sysrq_nolock() and then
> > > > let the loglevel handler(s) set next_loglevel.
> > > > If next_loglevel != -1 at the end of __handle_sysrq_nolock(),
> > > > set console_loglevel to next_loglevel.
> > >
> > > I'm looking real close at this right now, and there are a couple of
> > > problems, and a simple, but ugly solution.
> > >
> > > The entire reason that console_loglevel is touched _after_ the call to
> > > the second level handler is actually for the loglevel handler's
> > > printout. I was trying to minimize change in the display, but horked it.
> > >
> > > Here is the problem.
> > >
> > > SysRq events use action messages which get printed by the top level
> > > handler before calling the second level handler, the call line is:
> > >
> > >         orig_log_level = console_loglevel;
> > >         console_loglevel = 7;
> > >         printk(KERN_INFO "SysRq : ");
> > >
> > >         op_p = __sysrq_get_key_op(key);
> > >       ...
> > >         printk ("%s", op_p->action_msg);
> > >         op_p->handler(key, pt_regs, kbd, tty);
> > >       ...
> > >         console_loglevel = orig_log_level;
> > >
> > >
> > > The killer here is the fact that the action message format string does
> > > not carry a newline, allowing people to register strings which leave the
> > > printk state open. The loglevel handler then fills in the loglevel, and
> > > closes the printk state.
> 
> /me switches to a decent keyboard to test with.
> 
> No, the killer is that console_loglevel is restored from
> orig_log_level after having been modified in the loglevel
> handler.

But the _reason_ is that the action message does not carry a new line.

> Bottom line:  I don't care which code goes into the sysrq.c,
> but I hope that you (and others) learn to do some basic testing
> before unleashing it.  I don't expect all Linux kernel code
> to be thoroughly tested before it is added to a kernel,
> especially for areas like VM and file systems.
> But some basic level of testing should have been done on it,
> and I can't tell that it was done.

A whole lot of basic testing was done. The locking and the tables and
the registration all work correctly. I screwed up the loglevel stuff,
and it took 6 months for anyone to notice. Some people think that the
registration functions should be valid to call even when
CONFIG_MAGIC_SYSRQ is not defined, but this is a style thing.

Waving your hands and saying "Why wasn't this tested!!!" over and over
is annoying, and doesn't accomplish anything.

> There is still room for some more/small improvements here.  Nothing
> earth-shattering.  For example, go_sync() and do_emergency_sync()
> don't need to save console_loglevel or set it to 7 (they have both
> already been done in __handle_sysrq_nolock()).
> My patch eliminated this cruft.

No. go_sync() and do_emergency_sync() should NOT make assumptions about
the nature of console_loglevel, they should behave as they have been.
The commingling of loglevel settings was wrong in the first place.

-- 
Crutcher        <[e-mail]>
GCS d--- s+:>+:- a-- C++++$ UL++++$ L+++$>++++ !E PS+++ PE Y+ PGP+>++++
    R-(+++) !tv(+++) b+(++++) G+ e>++++ h+>++ r* y+>*$