* [PATCH-next 0/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE @ 2020-01-09 21:54 Dmitry Safonov 2020-01-09 21:54 ` [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy Dmitry Safonov 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Safonov @ 2020-01-09 21:54 UTC (permalink / raw) To: linux-kernel Cc: Dmitry Safonov, Dmitry Safonov, Greg Kroah-Hartman, Jiri Slaby, Vasiliy Khoruzhick, linux-serial, Iurii Zaikin, Luis Chamberlain, Kees Cook, linux-fsdevel Magic sysrq has proven for Arista usecases to be useful for debugging issues in field, over serial line when the switch is in such bad state that it can't accept network connections anymore. Unfortunately, having sysrq always enabled doesn't work for some embedded boards that tend to generate garbage on serial line (including BREAKs). Since commit 732dbf3a6104 ("serial: do not accept sysrq characters via serial port"), it's possible to keep sysrq enabled, but over serial line. Add a way to enable sysrq on a uart, where currently it can be constantly either on or off (CONFIG_MAGIC_SYSRQ_SERIAL). While doing so, cleanup __sysrq_enabled and serial_core header file. Sending against -next tree as it's based on removing SUPPORT_SYSRQ ifdeffery [1]. [1]: https://lkml.kernel.org/r/20191213000657.931618-1-dima@arista.com Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Jiri Slaby <jslaby@suse.com> Cc: Vasiliy Khoruzhick <vasilykh@arista.com> Cc: linux-serial@vger.kernel.org Thanks, Dmitry Dmitry Safonov (3): serial_core: Move sysrq functions from header file sysctl/sysrq: Remove __sysrq_enabled copy serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE drivers/tty/serial/serial_core.c | 123 +++++++++++++++++++++++++++++++ drivers/tty/sysrq.c | 7 ++ include/linux/serial_core.h | 86 ++------------------- include/linux/sysrq.h | 1 + kernel/sysctl.c | 41 ++++++----- lib/Kconfig.debug | 8 ++ 6 files changed, 167 insertions(+), 99 deletions(-) -- 2.24.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy 2020-01-09 21:54 [PATCH-next 0/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE Dmitry Safonov @ 2020-01-09 21:54 ` Dmitry Safonov 2020-01-10 3:19 ` Iurii Zaikin 2020-01-10 16:40 ` Greg Kroah-Hartman 0 siblings, 2 replies; 7+ messages in thread From: Dmitry Safonov @ 2020-01-09 21:54 UTC (permalink / raw) To: linux-kernel Cc: Dmitry Safonov, Dmitry Safonov, Greg Kroah-Hartman, Jiri Slaby, Vasiliy Khoruzhick, linux-serial, Iurii Zaikin, Luis Chamberlain, Kees Cook, linux-fsdevel Many embedded boards have a disconnected TTL level serial which can generate some garbage that can lead to spurious false sysrq detects. Currently, sysrq can be either completely disabled for serial console or always disabled (with CONFIG_MAGIC_SYSRQ_SERIAL), since commit 732dbf3a6104 ("serial: do not accept sysrq characters via serial port") At Arista, we have such boards that can generate BREAK and random garbage. While disabling sysrq for serial console would solve the problem with spurious false sysrq triggers, it's also desirable to have a way to enable sysrq back. Having the way to enable sysrq was beneficial to debug lockups with a manual investigation in field and on the other side preventing false sysrq detections. As a preparation to add sysrq_toggle_support() call into uart, remove a private copy of sysrq_enabled from sysctl - it should reflect the actual status of sysrq. Furthermore, the private copy isn't correct already in case sysrq_always_enabled is true. So, remove __sysrq_enabled and use a getter-helper for sysrq enabled status. Cc: Iurii Zaikin <yzaikin@google.com> Cc: Jiri Slaby <jslaby@suse.com> Cc: Luis Chamberlain <mcgrof@kernel.org> Cc: Kees Cook <keescook@chromium.org> Cc: linux-fsdevel@vger.kernel.org Signed-off-by: Dmitry Safonov <dima@arista.com> --- drivers/tty/sysrq.c | 7 +++++++ include/linux/sysrq.h | 1 + kernel/sysctl.c | 41 ++++++++++++++++++++++------------------- 3 files changed, 30 insertions(+), 19 deletions(-) diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c index f724962a5906..ef3e78967146 100644 --- a/drivers/tty/sysrq.c +++ b/drivers/tty/sysrq.c @@ -73,6 +73,13 @@ static bool sysrq_on_mask(int mask) (sysrq_enabled & mask); } +int sysrq_get_mask(void) +{ + if (sysrq_always_enabled) + return 1; + return sysrq_enabled; +} + static int __init sysrq_always_enabled_setup(char *str) { sysrq_always_enabled = true; diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h index 8c71874e8485..4a0b351fa2d3 100644 --- a/include/linux/sysrq.h +++ b/include/linux/sysrq.h @@ -50,6 +50,7 @@ int unregister_sysrq_key(int key, struct sysrq_key_op *op); struct sysrq_key_op *__sysrq_get_key_op(int key); int sysrq_toggle_support(int enable_mask); +int sysrq_get_mask(void); #else diff --git a/kernel/sysctl.c b/kernel/sysctl.c index d396aaaf19a3..6ddb4d7df0e1 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -229,25 +229,8 @@ static int proc_dopipe_max_size(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos); #ifdef CONFIG_MAGIC_SYSRQ -/* Note: sysrq code uses its own private copy */ -static int __sysrq_enabled = CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE; - static int sysrq_sysctl_handler(struct ctl_table *table, int write, - void __user *buffer, size_t *lenp, - loff_t *ppos) -{ - int error; - - error = proc_dointvec(table, write, buffer, lenp, ppos); - if (error) - return error; - - if (write) - sysrq_toggle_support(__sysrq_enabled); - - return 0; -} - + void __user *buffer, size_t *lenp, loff_t *ppos); #endif static struct ctl_table kern_table[]; @@ -747,7 +730,7 @@ static struct ctl_table kern_table[] = { #ifdef CONFIG_MAGIC_SYSRQ { .procname = "sysrq", - .data = &__sysrq_enabled, + .data = NULL, .maxlen = sizeof (int), .mode = 0644, .proc_handler = sysrq_sysctl_handler, @@ -2844,6 +2827,26 @@ static int proc_dostring_coredump(struct ctl_table *table, int write, } #endif +#ifdef CONFIG_MAGIC_SYSRQ +static int sysrq_sysctl_handler(struct ctl_table *table, int write, + void __user *buffer, size_t *lenp, loff_t *ppos) +{ + int tmp, ret; + + tmp = sysrq_get_mask(); + + ret = __do_proc_dointvec(&tmp, table, write, buffer, + lenp, ppos, NULL, NULL); + if (ret || !write) + return ret; + + if (write) + sysrq_toggle_support(tmp); + + return 0; +} +#endif + static int __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos, -- 2.24.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy 2020-01-09 21:54 ` [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy Dmitry Safonov @ 2020-01-10 3:19 ` Iurii Zaikin 2020-01-10 21:48 ` Dmitry Safonov 2020-01-10 16:40 ` Greg Kroah-Hartman 1 sibling, 1 reply; 7+ messages in thread From: Iurii Zaikin @ 2020-01-10 3:19 UTC (permalink / raw) To: Dmitry Safonov Cc: Linux Kernel Mailing List, Dmitry Safonov, Greg Kroah-Hartman, Jiri Slaby, Vasiliy Khoruzhick, linux-serial, Luis Chamberlain, Kees Cook, Linus FS Devel Mailing List > Furthermore, the private copy isn't correct already in case > sysrq_always_enabled is true. So, remove __sysrq_enabled and use a > getter-helper for sysrq enabled status. Since the old way was known to be unreliable, the new way looks like a great candidate for a KUnit test. Off topic: I wonder if Magic Sysrq could be extended with reasonable effort to be triggered by a sequence of keystrokes which would be less likely to be generated by a noisy serial. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy 2020-01-10 3:19 ` Iurii Zaikin @ 2020-01-10 21:48 ` Dmitry Safonov 0 siblings, 0 replies; 7+ messages in thread From: Dmitry Safonov @ 2020-01-10 21:48 UTC (permalink / raw) To: Iurii Zaikin Cc: Linux Kernel Mailing List, Dmitry Safonov, Greg Kroah-Hartman, Jiri Slaby, Vasiliy Khoruzhick, linux-serial, Luis Chamberlain, Kees Cook, Linus FS Devel Mailing List Hi Iurii, On 1/10/20 3:19 AM, Iurii Zaikin wrote: > Off topic: I wonder if Magic Sysrq could be extended with reasonable > effort to be triggered by a sequence of keystrokes which would be less > likely to be generated by a noisy serial. That's what I do in 3/3 patch from the series: https://lkml.kernel.org/r/20200109215444.95995-4-dima@arista.com Or do you mean something else? Thanks, Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy 2020-01-09 21:54 ` [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy Dmitry Safonov 2020-01-10 3:19 ` Iurii Zaikin @ 2020-01-10 16:40 ` Greg Kroah-Hartman 2020-01-10 21:45 ` Dmitry Safonov 1 sibling, 1 reply; 7+ messages in thread From: Greg Kroah-Hartman @ 2020-01-10 16:40 UTC (permalink / raw) To: Dmitry Safonov Cc: linux-kernel, Dmitry Safonov, Jiri Slaby, Vasiliy Khoruzhick, linux-serial, Iurii Zaikin, Luis Chamberlain, Kees Cook, linux-fsdevel On Thu, Jan 09, 2020 at 09:54:43PM +0000, Dmitry Safonov wrote: > Many embedded boards have a disconnected TTL level serial which can > generate some garbage that can lead to spurious false sysrq detects. > > Currently, sysrq can be either completely disabled for serial console > or always disabled (with CONFIG_MAGIC_SYSRQ_SERIAL), since > commit 732dbf3a6104 ("serial: do not accept sysrq characters via serial port") > > At Arista, we have such boards that can generate BREAK and random > garbage. While disabling sysrq for serial console would solve > the problem with spurious false sysrq triggers, it's also desirable > to have a way to enable sysrq back. > > Having the way to enable sysrq was beneficial to debug lockups with > a manual investigation in field and on the other side preventing false > sysrq detections. > > As a preparation to add sysrq_toggle_support() call into uart, > remove a private copy of sysrq_enabled from sysctl - it should reflect > the actual status of sysrq. > > Furthermore, the private copy isn't correct already in case > sysrq_always_enabled is true. So, remove __sysrq_enabled and use a > getter-helper for sysrq enabled status. > > Cc: Iurii Zaikin <yzaikin@google.com> > Cc: Jiri Slaby <jslaby@suse.com> > Cc: Luis Chamberlain <mcgrof@kernel.org> > Cc: Kees Cook <keescook@chromium.org> > Cc: linux-fsdevel@vger.kernel.org > Signed-off-by: Dmitry Safonov <dima@arista.com> > --- > drivers/tty/sysrq.c | 7 +++++++ > include/linux/sysrq.h | 1 + > kernel/sysctl.c | 41 ++++++++++++++++++++++------------------- > 3 files changed, 30 insertions(+), 19 deletions(-) > > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c > index f724962a5906..ef3e78967146 100644 > --- a/drivers/tty/sysrq.c > +++ b/drivers/tty/sysrq.c > @@ -73,6 +73,13 @@ static bool sysrq_on_mask(int mask) > (sysrq_enabled & mask); > } > > +int sysrq_get_mask(void) > +{ > + if (sysrq_always_enabled) > + return 1; > + return sysrq_enabled; > +} > + > static int __init sysrq_always_enabled_setup(char *str) > { > sysrq_always_enabled = true; > diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h > index 8c71874e8485..4a0b351fa2d3 100644 > --- a/include/linux/sysrq.h > +++ b/include/linux/sysrq.h > @@ -50,6 +50,7 @@ int unregister_sysrq_key(int key, struct sysrq_key_op *op); > struct sysrq_key_op *__sysrq_get_key_op(int key); > > int sysrq_toggle_support(int enable_mask); > +int sysrq_get_mask(void); > > #else > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index d396aaaf19a3..6ddb4d7df0e1 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -229,25 +229,8 @@ static int proc_dopipe_max_size(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, loff_t *ppos); > > #ifdef CONFIG_MAGIC_SYSRQ > -/* Note: sysrq code uses its own private copy */ > -static int __sysrq_enabled = CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE; > - > static int sysrq_sysctl_handler(struct ctl_table *table, int write, > - void __user *buffer, size_t *lenp, > - loff_t *ppos) > -{ > - int error; > - > - error = proc_dointvec(table, write, buffer, lenp, ppos); > - if (error) > - return error; > - > - if (write) > - sysrq_toggle_support(__sysrq_enabled); > - > - return 0; > -} > - > + void __user *buffer, size_t *lenp, loff_t *ppos); > #endif > > static struct ctl_table kern_table[]; > @@ -747,7 +730,7 @@ static struct ctl_table kern_table[] = { > #ifdef CONFIG_MAGIC_SYSRQ > { > .procname = "sysrq", > - .data = &__sysrq_enabled, > + .data = NULL, > .maxlen = sizeof (int), > .mode = 0644, > .proc_handler = sysrq_sysctl_handler, > @@ -2844,6 +2827,26 @@ static int proc_dostring_coredump(struct ctl_table *table, int write, > } > #endif > > +#ifdef CONFIG_MAGIC_SYSRQ > +static int sysrq_sysctl_handler(struct ctl_table *table, int write, > + void __user *buffer, size_t *lenp, loff_t *ppos) > +{ > + int tmp, ret; > + > + tmp = sysrq_get_mask(); > + > + ret = __do_proc_dointvec(&tmp, table, write, buffer, > + lenp, ppos, NULL, NULL); > + if (ret || !write) > + return ret; > + > + if (write) > + sysrq_toggle_support(tmp); > + > + return 0; > +} > +#endif Why did you move this function down here? Can't it stay where it is and you can just fix the logic there? Now you have two different #ifdef blocks intead of just one :( thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy 2020-01-10 16:40 ` Greg Kroah-Hartman @ 2020-01-10 21:45 ` Dmitry Safonov 2020-01-10 22:01 ` Greg Kroah-Hartman 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Safonov @ 2020-01-10 21:45 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-kernel, Dmitry Safonov, Jiri Slaby, Vasiliy Khoruzhick, linux-serial, Iurii Zaikin, Luis Chamberlain, Kees Cook, linux-fsdevel Hi Greg, On 1/10/20 4:40 PM, Greg Kroah-Hartman wrote: > On Thu, Jan 09, 2020 at 09:54:43PM +0000, Dmitry Safonov wrote: [..] >> @@ -2844,6 +2827,26 @@ static int proc_dostring_coredump(struct ctl_table *table, int write, >> } >> #endif >> >> +#ifdef CONFIG_MAGIC_SYSRQ >> +static int sysrq_sysctl_handler(struct ctl_table *table, int write, >> + void __user *buffer, size_t *lenp, loff_t *ppos) >> +{ >> + int tmp, ret; >> + >> + tmp = sysrq_get_mask(); >> + >> + ret = __do_proc_dointvec(&tmp, table, write, buffer, >> + lenp, ppos, NULL, NULL); >> + if (ret || !write) >> + return ret; >> + >> + if (write) >> + sysrq_toggle_support(tmp); >> + >> + return 0; >> +} >> +#endif > > Why did you move this function down here? Can't it stay where it is and > you can just fix the logic there? Now you have two different #ifdef > blocks intead of just one :( Yeah, well __do_proc_dointvec() made me do it. sysrq_sysctl_handler() declaration should be before ctl_table array of sysctls, so I couldn't remove the forward-declaration. So, I could forward-declare __do_proc_dointvec() instead, but looking at the neighborhood, I decided to follow the file-style (there is a couple of forward-declarations before the sysctl array, some under ifdefs). I admit that the result is imperfect and can put __do_proc_dointvec() definition before instead, no hard feelings. Thanks, Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy 2020-01-10 21:45 ` Dmitry Safonov @ 2020-01-10 22:01 ` Greg Kroah-Hartman 0 siblings, 0 replies; 7+ messages in thread From: Greg Kroah-Hartman @ 2020-01-10 22:01 UTC (permalink / raw) To: Dmitry Safonov Cc: linux-kernel, Dmitry Safonov, Jiri Slaby, Vasiliy Khoruzhick, linux-serial, Iurii Zaikin, Luis Chamberlain, Kees Cook, linux-fsdevel On Fri, Jan 10, 2020 at 09:45:30PM +0000, Dmitry Safonov wrote: > Hi Greg, > > On 1/10/20 4:40 PM, Greg Kroah-Hartman wrote: > > On Thu, Jan 09, 2020 at 09:54:43PM +0000, Dmitry Safonov wrote: > [..] > >> @@ -2844,6 +2827,26 @@ static int proc_dostring_coredump(struct ctl_table *table, int write, > >> } > >> #endif > >> > >> +#ifdef CONFIG_MAGIC_SYSRQ > >> +static int sysrq_sysctl_handler(struct ctl_table *table, int write, > >> + void __user *buffer, size_t *lenp, loff_t *ppos) > >> +{ > >> + int tmp, ret; > >> + > >> + tmp = sysrq_get_mask(); > >> + > >> + ret = __do_proc_dointvec(&tmp, table, write, buffer, > >> + lenp, ppos, NULL, NULL); > >> + if (ret || !write) > >> + return ret; > >> + > >> + if (write) > >> + sysrq_toggle_support(tmp); > >> + > >> + return 0; > >> +} > >> +#endif > > > > Why did you move this function down here? Can't it stay where it is and > > you can just fix the logic there? Now you have two different #ifdef > > blocks intead of just one :( > > Yeah, well __do_proc_dointvec() made me do it. > > sysrq_sysctl_handler() declaration should be before ctl_table array of > sysctls, so I couldn't remove the forward-declaration. > > So, I could forward-declare __do_proc_dointvec() instead, but looking at > the neighborhood, I decided to follow the file-style (there is a couple > of forward-declarations before the sysctl array, some under ifdefs). > > I admit that the result is imperfect and can put __do_proc_dointvec() > definition before instead, no hard feelings. Ah, no, nevermind, I missed that reason, sorry about that. Moving it is fine. greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-01-10 22:01 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-09 21:54 [PATCH-next 0/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE Dmitry Safonov 2020-01-09 21:54 ` [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy Dmitry Safonov 2020-01-10 3:19 ` Iurii Zaikin 2020-01-10 21:48 ` Dmitry Safonov 2020-01-10 16:40 ` Greg Kroah-Hartman 2020-01-10 21:45 ` Dmitry Safonov 2020-01-10 22:01 ` Greg Kroah-Hartman
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).