Re: [PATCH] Magic SysRq loglevel fix.

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

> > There was a time when I thought that was a good idea.
> >
> > Go ahead, laugh.

Nah, I don't want to laugh at it.  More like cry at it.
It's set me back from other work by too many hours already.

> > Anyway, that sort of unresolved state is bad, and is the source of all
> > of this song and dance. I think the right answer is to force handlers to
> > open their own calls to printk, and to keep whats going on with the
> > console_loglevel and printk buffer nice and clean.
> >
> > The cost is that messages like this:
> >
> > SysRq : Loglevel switched to X
> >
> > will have to become more like this:
> >
> > SysRq : Loglevel
> > Loglevel switched to X

Yes, that's ugly and shouldn't be done.

I made an OK state machine (previous and next states) of it.
Your patch moves restoring console_loglevel to a place
that makes sense.

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.

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.

~Randy