linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv2-next 0/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE
@ 2020-01-14 17:19 Dmitry Safonov
  2020-01-14 17:19 ` [PATCHv2-next 1/3] sysctl/sysrq: Remove __sysrq_enabled copy Dmitry Safonov
  0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Safonov @ 2020-01-14 17:19 UTC (permalink / raw)
  To: linux-kernel
  Cc: Dmitry Safonov, Dmitry Safonov, Greg Kroah-Hartman, Iurii Zaikin,
	Jiri Slaby, Joe Perches, Randy Dunlap, Vasiliy Khoruzhick,
	linux-serial, 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].

Changes since v1 [2]:
- Fix typo in pr_info() message (noticed by Randy Dunlap, thanks)
- Add SYSRQ_TIMEOUT define for timeout after BREAK and separate removing
  @unused member of uart_port into cleanup patch (by Greg's review, thanks)
- Add const qualifier, make uart_try_toggle_sysrq() bool function
  (Joe Perches, thanks)
- Fix !CONFIG_SYSRQ and CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE=""
  build failures (kudos to kbuild test robot)

[1]: https://lkml.kernel.org/r/20191213000657.931618-1-dima@arista.com
[2]: https://lkml.kernel.org/r/20200109215444.95995-1-dima@arista.com

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Iurii Zaikin <yzaikin@google.com>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Joe Perches <joe@perches.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Vasiliy Khoruzhick <vasilykh@arista.com>
Cc: linux-serial@vger.kernel.org

Thanks,
             Dmitry

Dmitry Safonov (3):
  sysctl/sysrq: Remove __sysrq_enabled copy
  serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE
  serial_core: Remove unused member in uart_port

 drivers/tty/serial/serial_core.c | 75 +++++++++++++++++++++++++++++---
 drivers/tty/sysrq.c              |  7 +++
 include/linux/serial_core.h      |  2 +-
 include/linux/sysrq.h            |  7 +++
 kernel/sysctl.c                  | 41 +++++++++--------
 lib/Kconfig.debug                |  8 ++++
 6 files changed, 113 insertions(+), 27 deletions(-)

-- 
2.24.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCHv2-next 1/3] sysctl/sysrq: Remove __sysrq_enabled copy
  2020-01-14 17:19 [PATCHv2-next 0/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE Dmitry Safonov
@ 2020-01-14 17:19 ` Dmitry Safonov
  2020-01-15 12:36   ` Greg Kroah-Hartman
  0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Safonov @ 2020-01-14 17:19 UTC (permalink / raw)
  To: linux-kernel
  Cc: Dmitry Safonov, Dmitry Safonov, Greg Kroah-Hartman, Iurii Zaikin,
	Jiri Slaby, Joe Perches, Randy Dunlap, Vasiliy Khoruzhick,
	linux-serial, 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 |  7 +++++++
 kernel/sysctl.c       | 41 ++++++++++++++++++++++-------------------
 3 files changed, 36 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..ad09a7eefda2 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
 
@@ -71,6 +72,12 @@ static inline int unregister_sysrq_key(int key, struct sysrq_key_op *op)
 	return -EINVAL;
 }
 
+static inline int sysrq_get_mask(void)
+{
+	/* Magic SysRq disabled mask */
+	return 0;
+}
+
 #endif
 
 #endif /* _LINUX_SYSRQ_H */
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] 5+ messages in thread

* Re: [PATCHv2-next 1/3] sysctl/sysrq: Remove __sysrq_enabled copy
  2020-01-14 17:19 ` [PATCHv2-next 1/3] sysctl/sysrq: Remove __sysrq_enabled copy Dmitry Safonov
@ 2020-01-15 12:36   ` Greg Kroah-Hartman
  2020-02-23 12:08     ` Dmitry Safonov
  0 siblings, 1 reply; 5+ messages in thread
From: Greg Kroah-Hartman @ 2020-01-15 12:36 UTC (permalink / raw)
  To: Dmitry Safonov
  Cc: linux-kernel, Dmitry Safonov, Iurii Zaikin, Jiri Slaby,
	Joe Perches, Randy Dunlap, Vasiliy Khoruzhick, linux-serial,
	Luis Chamberlain, Kees Cook, linux-fsdevel

On Tue, Jan 14, 2020 at 05:19:10PM +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 |  7 +++++++
>  kernel/sysctl.c       | 41 ++++++++++++++++++++++-------------------
>  3 files changed, 36 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;
> +}

Naming is hard.  And this name is really hard to understand.

Traditionally get/put are used for incrementing reference counts.  You
don't have a sysrq_put_mask() call, right?  :)

I think what you want this function to do is, "is sysrq enabled right
now" (hint, it's a global function, add kernel-doc to it so we know what
it does...).  If so, it should maybe be something like:

	bool sysrq_is_enabled(void);

which to me makes more sense.

thoughts?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCHv2-next 1/3] sysctl/sysrq: Remove __sysrq_enabled copy
  2020-01-15 12:36   ` Greg Kroah-Hartman
@ 2020-02-23 12:08     ` Dmitry Safonov
  2020-02-23 12:10       ` Dmitry Safonov
  0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Safonov @ 2020-02-23 12:08 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Dmitry Safonov, Iurii Zaikin, Jiri Slaby,
	Joe Perches, Randy Dunlap, Vasiliy Khoruzhick, linux-serial,
	Luis Chamberlain, Kees Cook, linux-fsdevel


On 1/15/20 12:36 PM, Greg Kroah-Hartman wrote:
> On Tue, Jan 14, 2020 at 05:19:10PM +0000, Dmitry Safonov wrote:
[..]
>> +int sysrq_get_mask(void)
>> +{
>> +	if (sysrq_always_enabled)
>> +		return 1;
>> +	return sysrq_enabled;
>> +}
> 
> Naming is hard.  And this name is really hard to understand.

Agree.


> Traditionally get/put are used for incrementing reference counts.  You
> don't have a sysrq_put_mask() call, right?  :)

Yes, fair point


> I think what you want this function to do is, "is sysrq enabled right
> now" (hint, it's a global function, add kernel-doc to it so we know what
> it does...).  If so, it should maybe be something like:
> 
> 	bool sysrq_is_enabled(void);
> 
> which to me makes more sense.

Err, not exactly: there is a function for that which is sysrq_on().
But for sysctl the value of the mask (or 1 for always_enabled) is
actually needed to show a proper value back to the userspace reader.

Thanks,
          Dmitry

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCHv2-next 1/3] sysctl/sysrq: Remove __sysrq_enabled copy
  2020-02-23 12:08     ` Dmitry Safonov
@ 2020-02-23 12:10       ` Dmitry Safonov
  0 siblings, 0 replies; 5+ messages in thread
From: Dmitry Safonov @ 2020-02-23 12:10 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Dmitry Safonov, Iurii Zaikin, Jiri Slaby,
	Joe Perches, Randy Dunlap, Vasiliy Khoruzhick, linux-serial,
	Luis Chamberlain, Kees Cook, linux-fsdevel



On 2/23/20 12:08 PM, Dmitry Safonov wrote:
> 
> On 1/15/20 12:36 PM, Greg Kroah-Hartman wrote:
>> On Tue, Jan 14, 2020 at 05:19:10PM +0000, Dmitry Safonov wrote:
> [..]
>>> +int sysrq_get_mask(void)
>>> +{
>>> +	if (sysrq_always_enabled)
>>> +		return 1;
>>> +	return sysrq_enabled;
>>> +}
>>
>> Naming is hard.  And this name is really hard to understand.
> 
> Agree.
> 
> 
>> Traditionally get/put are used for incrementing reference counts.  You
>> don't have a sysrq_put_mask() call, right?  :)
> 
> Yes, fair point
> 
> 
>> I think what you want this function to do is, "is sysrq enabled right
>> now" (hint, it's a global function, add kernel-doc to it so we know what
>> it does...).  If so, it should maybe be something like:
>>
>> 	bool sysrq_is_enabled(void);
>>
>> which to me makes more sense.
> 
> Err, not exactly: there is a function for that which is sysrq_on().
> But for sysctl the value of the mask (or 1 for always_enabled) is
> actually needed to show a proper value back to the userspace reader.

I'll call it sysrq_mask(), add the kernel-doc to it in v3.

Thanks again,
          Dmitry

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-02-23 12:10 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-14 17:19 [PATCHv2-next 0/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE Dmitry Safonov
2020-01-14 17:19 ` [PATCHv2-next 1/3] sysctl/sysrq: Remove __sysrq_enabled copy Dmitry Safonov
2020-01-15 12:36   ` Greg Kroah-Hartman
2020-02-23 12:08     ` Dmitry Safonov
2020-02-23 12:10       ` Dmitry Safonov

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