* [PATCH] sysrq: Restore original console_loglevel when sysrq disabled
@ 2019-01-11 12:45 Petr Mladek
2019-01-11 13:07 ` Sergey Senozhatsky
0 siblings, 1 reply; 6+ messages in thread
From: Petr Mladek @ 2019-01-11 12:45 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby
Cc: Sergey Senozhatsky, Steven Rostedt, Sergey Senozhatsky,
linux-kernel, Petr Mladek
The sysrq header line is printed with an increased loglevel
to provide users some positive feedback.
The original loglevel is not restored when the sysrq operation
is disabled. This bug was introduced in 2.6.12 (pre-git-history)
by the commit ("Allow admin to enable only some of the Magic-Sysrq
functions").
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
drivers/tty/sysrq.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 1f03078ec352..2e4e184cfaa6 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -553,6 +553,7 @@ void __handle_sysrq(int key, bool check_mask)
op_p->handler(key);
} else {
pr_cont("This sysrq operation is disabled.\n");
+ console_loglevel = orig_log_level;
}
} else {
pr_cont("HELP : ");
--
2.13.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] sysrq: Restore original console_loglevel when sysrq disabled
2019-01-11 12:45 [PATCH] sysrq: Restore original console_loglevel when sysrq disabled Petr Mladek
@ 2019-01-11 13:07 ` Sergey Senozhatsky
2019-01-11 15:32 ` Petr Mladek
2019-01-11 16:24 ` Steven Rostedt
0 siblings, 2 replies; 6+ messages in thread
From: Sergey Senozhatsky @ 2019-01-11 13:07 UTC (permalink / raw)
To: Petr Mladek
Cc: Greg Kroah-Hartman, Jiri Slaby, Sergey Senozhatsky,
Steven Rostedt, Sergey Senozhatsky, linux-kernel
On (01/11/19 13:45), Petr Mladek wrote:
> The sysrq header line is printed with an increased loglevel
> to provide users some positive feedback.
>
> The original loglevel is not restored when the sysrq operation
> is disabled. This bug was introduced in 2.6.12 (pre-git-history)
> by the commit ("Allow admin to enable only some of the Magic-Sysrq
> functions").
Good find, and the patch looks OK to me. A small comment below.
FWIW,
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
A side note (nitpick, etc.); it's Friday night in here, I'm enjoying
my beer; so maybe I'm wrong about the whole thing.
> @@ -553,6 +553,7 @@ void __handle_sysrq(int key, bool check_mask)
> op_p->handler(key);
> } else {
> pr_cont("This sysrq operation is disabled.\n");
> + console_loglevel = orig_log_level;
> }
This looks a bit racy.
When we do
printk("FOO\n");
console_loglevel = XYZ;
We don't have any real guarantees that printk("FOO\n") will print
anything straight ahead. It is possible that console_sem is already
locked and the owner is preempted, so by the time the console_sem
owner picks up that FOO\n messages, console_loglevel is back to
orig_log_level and suppress_message_printing() will just tell us
to skip the message.
Do we need pr_cont() there? Maybe we can just have a normal pr_err()
which would always tell that "key" sysrq is disabled? (we also
would need to change the error message a bit).
-ss
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sysrq: Restore original console_loglevel when sysrq disabled
2019-01-11 13:07 ` Sergey Senozhatsky
@ 2019-01-11 15:32 ` Petr Mladek
2019-01-14 5:36 ` Sergey Senozhatsky
2019-01-11 16:24 ` Steven Rostedt
1 sibling, 1 reply; 6+ messages in thread
From: Petr Mladek @ 2019-01-11 15:32 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Greg Kroah-Hartman, Jiri Slaby, Steven Rostedt,
Sergey Senozhatsky, linux-kernel
On Fri 2019-01-11 22:07:29, Sergey Senozhatsky wrote:
> On (01/11/19 13:45), Petr Mladek wrote:
> > The sysrq header line is printed with an increased loglevel
> > to provide users some positive feedback.
> >
> > The original loglevel is not restored when the sysrq operation
> > is disabled. This bug was introduced in 2.6.12 (pre-git-history)
> > by the commit ("Allow admin to enable only some of the Magic-Sysrq
> > functions").
>
>
> Good find, and the patch looks OK to me. A small comment below.
> FWIW,
> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Thanks.
> A side note (nitpick, etc.); it's Friday night in here, I'm enjoying
> my beer; so maybe I'm wrong about the whole thing.
>
>
> > @@ -553,6 +553,7 @@ void __handle_sysrq(int key, bool check_mask)
> > op_p->handler(key);
> > } else {
> > pr_cont("This sysrq operation is disabled.\n");
> > + console_loglevel = orig_log_level;
> > }
>
> This looks a bit racy.
>
> When we do
>
> printk("FOO\n");
> console_loglevel = XYZ;
>
> We don't have any real guarantees that printk("FOO\n") will print
> anything straight ahead. It is possible that console_sem is already
> locked and the owner is preempted, so by the time the console_sem
> owner picks up that FOO\n messages, console_loglevel is back to
> orig_log_level and suppress_message_printing() will just tell us
> to skip the message.
>
> Do we need pr_cont() there? Maybe we can just have a normal pr_err()
> which would always tell that "key" sysrq is disabled? (we also
> would need to change the error message a bit).
The same problem is with the sysrq header line. It uses the trick
with console_loglevel by intention. We want to show it but
it is not really an error message, see the commit
fb144adc517d9ebe8fd ("sysrq: add commentary on why we use
the console loglevel over using KERN_EMERG").
Best Regards,
Petr
PS: I am going to resend this patch as part of a patchset.
I was working on one more fix. It looked independent
and questionable. I wanted to send it as RFC separately
but there is a clash in the end...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sysrq: Restore original console_loglevel when sysrq disabled
2019-01-11 13:07 ` Sergey Senozhatsky
2019-01-11 15:32 ` Petr Mladek
@ 2019-01-11 16:24 ` Steven Rostedt
1 sibling, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2019-01-11 16:24 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, Greg Kroah-Hartman, Jiri Slaby, Sergey Senozhatsky,
linux-kernel
On Fri, 11 Jan 2019 22:07:29 +0900
Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:
> On (01/11/19 13:45), Petr Mladek wrote:
> > The sysrq header line is printed with an increased loglevel
> > to provide users some positive feedback.
> >
> > The original loglevel is not restored when the sysrq operation
> > is disabled. This bug was introduced in 2.6.12 (pre-git-history)
> > by the commit ("Allow admin to enable only some of the Magic-Sysrq
> > functions").
>
>
> Good find, and the patch looks OK to me. A small comment below.
> FWIW,
> Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
>
>
> ---
>
> A side note (nitpick, etc.); it's Friday night in here, I'm enjoying
> my beer; so maybe I'm wrong about the whole thing.
>
>
> > @@ -553,6 +553,7 @@ void __handle_sysrq(int key, bool check_mask)
> > op_p->handler(key);
> > } else {
> > pr_cont("This sysrq operation is disabled.\n");
> > + console_loglevel = orig_log_level;
> > }
>
> This looks a bit racy.
>
> When we do
>
> printk("FOO\n");
> console_loglevel = XYZ;
>
> We don't have any real guarantees that printk("FOO\n") will print
> anything straight ahead. It is possible that console_sem is already
> locked and the owner is preempted, so by the time the console_sem
> owner picks up that FOO\n messages, console_loglevel is back to
> orig_log_level and suppress_message_printing() will just tell us
> to skip the message.
>
> Do we need pr_cont() there? Maybe we can just have a normal pr_err()
> which would always tell that "key" sysrq is disabled? (we also
> would need to change the error message a bit).
>
All this is for another patch and another discussion. This current
patch keeps with what is there and fixes a missing reset of
console_loglevel.
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
-- Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sysrq: Restore original console_loglevel when sysrq disabled
2019-01-11 15:32 ` Petr Mladek
@ 2019-01-14 5:36 ` Sergey Senozhatsky
2019-01-14 14:37 ` Petr Mladek
0 siblings, 1 reply; 6+ messages in thread
From: Sergey Senozhatsky @ 2019-01-14 5:36 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Greg Kroah-Hartman, Jiri Slaby,
Steven Rostedt, Sergey Senozhatsky, linux-kernel
On (01/11/19 16:32), Petr Mladek wrote:
> The same problem is with the sysrq header line. It uses the trick
> with console_loglevel by intention. We want to show it but
> it is not really an error message
May be.
I usually see it as an "error".
My case:
systemd sets sysrq on every boot to /lib/sysctl.d/50-default.conf
kernel.sysrq value, which I usually set to 1. But after every systemd
package update I have to edit 50-default.conf again, because somebody
concluded that overwriting /lib/sysctl.d/50-default.conf during package
update was the right thing to do. So, occasionally, when I need to do
sysrq all I get is "This sysrq operation is disabled" error. So I swear
a lot, reboot the box, change the sysrq mask and try to reproduce the
problem. /* I became familiar with "sysrq_always_enabled=1" just
recently. */
"This sysrq operation is disabled" is always bad news and is always not
what I want to see.
-ss
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sysrq: Restore original console_loglevel when sysrq disabled
2019-01-14 5:36 ` Sergey Senozhatsky
@ 2019-01-14 14:37 ` Petr Mladek
0 siblings, 0 replies; 6+ messages in thread
From: Petr Mladek @ 2019-01-14 14:37 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Sergey Senozhatsky, Greg Kroah-Hartman, Jiri Slaby,
Steven Rostedt, linux-kernel
On Mon 2019-01-14 14:36:42, Sergey Senozhatsky wrote:
> On (01/11/19 16:32), Petr Mladek wrote:
> > The same problem is with the sysrq header line. It uses the trick
> > with console_loglevel by intention. We want to show it but
> > it is not really an error message
>
> May be.
>
> I usually see it as an "error".
>
>
> My case:
> systemd sets sysrq on every boot to /lib/sysctl.d/50-default.conf
> kernel.sysrq value, which I usually set to 1. But after every systemd
> package update I have to edit 50-default.conf again, because somebody
> concluded that overwriting /lib/sysctl.d/50-default.conf during package
> update was the right thing to do. So, occasionally, when I need to do
> sysrq all I get is "This sysrq operation is disabled" error. So I swear
> a lot, reboot the box, change the sysrq mask and try to reproduce the
> problem. /* I became familiar with "sysrq_always_enabled=1" just
> recently. */
>
> "This sysrq operation is disabled" is always bad news and is always not
> what I want to see.
It is a matter of taste and I do not have strong opinion about it.
Anyway, changing the string should be a separate patch.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-14 14:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11 12:45 [PATCH] sysrq: Restore original console_loglevel when sysrq disabled Petr Mladek
2019-01-11 13:07 ` Sergey Senozhatsky
2019-01-11 15:32 ` Petr Mladek
2019-01-14 5:36 ` Sergey Senozhatsky
2019-01-14 14:37 ` Petr Mladek
2019-01-11 16:24 ` Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).