LKML Archive on lore.kernel.org
 help / Atom feed
* [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	[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, back to index

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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox