[PATCH-next 0/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[PATCH-next 0/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Dmitry Safonov]

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

[PATCH-next 1/3] serial_core: Move sysrq functions from header file

[Dmitry Safonov]

It's not worth to have them in every serial driver and I'm about to add
another helper function.

Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/tty/serial/serial_core.c | 83 +++++++++++++++++++++++++++++++
 include/linux/serial_core.h      | 84 ++------------------------------
 2 files changed, 88 insertions(+), 79 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 7c2782785736..6ac9dfed3423 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3081,6 +3081,89 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
 }
 EXPORT_SYMBOL_GPL(uart_insert_char);
 
+int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+	if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
+		return 0;
+
+	if (!port->has_sysrq || !port->sysrq)
+		return 0;
+
+	if (ch && time_before(jiffies, port->sysrq)) {
+		handle_sysrq(ch);
+		port->sysrq = 0;
+		return 1;
+	}
+	port->sysrq = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(uart_handle_sysrq_char);
+
+int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
+{
+	if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
+		return 0;
+
+	if (!port->has_sysrq || !port->sysrq)
+		return 0;
+
+	if (ch && time_before(jiffies, port->sysrq)) {
+		port->sysrq_ch = ch;
+		port->sysrq = 0;
+		return 1;
+	}
+	port->sysrq = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
+
+void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
+{
+	int sysrq_ch;
+
+	if (!port->has_sysrq) {
+		spin_unlock_irqrestore(&port->lock, irqflags);
+		return;
+	}
+
+	sysrq_ch = port->sysrq_ch;
+	port->sysrq_ch = 0;
+
+	spin_unlock_irqrestore(&port->lock, irqflags);
+
+	if (sysrq_ch)
+		handle_sysrq(sysrq_ch);
+}
+EXPORT_SYMBOL_GPL(uart_unlock_and_check_sysrq);
+
+/*
+ * We do the SysRQ and SAK checking like this...
+ */
+int uart_handle_break(struct uart_port *port)
+{
+	struct uart_state *state = port->state;
+
+	if (port->handle_break)
+		port->handle_break(port);
+
+	if (port->has_sysrq) {
+		if (port->cons && port->cons->index == port->line) {
+			if (!port->sysrq) {
+				port->sysrq = jiffies + HZ*5;
+				return 1;
+			}
+			port->sysrq = 0;
+		}
+	}
+
+	if (port->flags & UPF_SAK)
+		do_SAK(state->port.tty);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(uart_handle_break);
+
 EXPORT_SYMBOL(uart_write_wakeup);
 EXPORT_SYMBOL(uart_register_driver);
 EXPORT_SYMBOL(uart_unregister_driver);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 9cf1682dc580..255e86a474e9 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -460,85 +460,11 @@ extern void uart_handle_cts_change(struct uart_port *uport,
 extern void uart_insert_char(struct uart_port *port, unsigned int status,
 		 unsigned int overrun, unsigned int ch, unsigned int flag);
 
-static inline int
-uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
-{
-	if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
-		return 0;
-
-	if (!port->has_sysrq || !port->sysrq)
-		return 0;
-
-	if (ch && time_before(jiffies, port->sysrq)) {
-		handle_sysrq(ch);
-		port->sysrq = 0;
-		return 1;
-	}
-	port->sysrq = 0;
-
-	return 0;
-}
-static inline int
-uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
-{
-	if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
-		return 0;
-
-	if (!port->has_sysrq || !port->sysrq)
-		return 0;
-
-	if (ch && time_before(jiffies, port->sysrq)) {
-		port->sysrq_ch = ch;
-		port->sysrq = 0;
-		return 1;
-	}
-	port->sysrq = 0;
-
-	return 0;
-}
-static inline void
-uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
-{
-	int sysrq_ch;
-
-	if (!port->has_sysrq) {
-		spin_unlock_irqrestore(&port->lock, irqflags);
-		return;
-	}
-
-	sysrq_ch = port->sysrq_ch;
-	port->sysrq_ch = 0;
-
-	spin_unlock_irqrestore(&port->lock, irqflags);
-
-	if (sysrq_ch)
-		handle_sysrq(sysrq_ch);
-}
-
-/*
- * We do the SysRQ and SAK checking like this...
- */
-static inline int uart_handle_break(struct uart_port *port)
-{
-	struct uart_state *state = port->state;
-
-	if (port->handle_break)
-		port->handle_break(port);
-
-	if (port->has_sysrq) {
-		if (port->cons && port->cons->index == port->line) {
-			if (!port->sysrq) {
-				port->sysrq = jiffies + HZ*5;
-				return 1;
-			}
-			port->sysrq = 0;
-		}
-	}
-
-	if (port->flags & UPF_SAK)
-		do_SAK(state->port.tty);
-	return 0;
-}
+extern int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch);
+extern int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch);
+extern void uart_unlock_and_check_sysrq(struct uart_port *port,
+					unsigned long irqflags);
+extern int uart_handle_break(struct uart_port *port);
 
 /*
  *	UART_ENABLE_MS - determine if port should enable modem status irqs
-- 
2.24.1

[PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy

[Dmitry Safonov]

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

[PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Dmitry Safonov]

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.

As a measure of balance between on and off options, add
MAGIC_SYSRQ_SERIAL_SEQUENCE which is a string sequence that can enable
sysrq if it follows BREAK on a serial line. The longer the string - the
less likely it may be in the garbage.

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.

Based-on-patch-by: Vasiliy Khoruzhick <vasilykh@arista.com>
Signed-off-by: Dmitry Safonov <dima@arista.com>
---
 drivers/tty/serial/serial_core.c | 52 ++++++++++++++++++++++++++++----
 include/linux/serial_core.h      |  2 +-
 lib/Kconfig.debug                |  8 +++++
 3 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 6ac9dfed3423..f70eba032d0b 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
 }
 EXPORT_SYMBOL_GPL(uart_insert_char);
 
+const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
+
+static void uart_sysrq_on(struct work_struct *w)
+{
+	sysrq_toggle_support(1);
+	pr_info("SysRq is enabled by magic sequience on serial\n");
+}
+static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
+
+static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
+{
+	if (sysrq_toggle_seq[0] == '\0')
+		return 0;
+
+	BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);
+	if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
+		port->sysrq_seq = 0;
+		return 0;
+	}
+
+	/* Without the last \0 */
+	if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
+		port->sysrq = jiffies + HZ*5;
+		return 1;
+	}
+
+	schedule_work(&sysrq_enable_work);
+
+	port->sysrq = 0;
+	return 1;
+}
+
 int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
 {
 	if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
@@ -3090,9 +3122,13 @@ int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
 		return 0;
 
 	if (ch && time_before(jiffies, port->sysrq)) {
-		handle_sysrq(ch);
-		port->sysrq = 0;
-		return 1;
+		if (sysrq_get_mask()) {
+			handle_sysrq(ch);
+			port->sysrq = 0;
+			return 1;
+		}
+		if (uart_try_toggle_sysrq(port, ch))
+			return 1;
 	}
 	port->sysrq = 0;
 
@@ -3109,9 +3145,13 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
 		return 0;
 
 	if (ch && time_before(jiffies, port->sysrq)) {
-		port->sysrq_ch = ch;
-		port->sysrq = 0;
-		return 1;
+		if (sysrq_get_mask()) {
+			port->sysrq_ch = ch;
+			port->sysrq = 0;
+			return 1;
+		}
+		if (uart_try_toggle_sysrq(port, ch))
+			return 1;
 	}
 	port->sysrq = 0;
 
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 255e86a474e9..1f4443db5474 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -243,10 +243,10 @@ struct uart_port {
 	unsigned long		sysrq;			/* sysrq timeout */
 	unsigned int		sysrq_ch;		/* char for sysrq */
 	unsigned char		has_sysrq;
+	unsigned char		sysrq_seq;		/* index in sysrq_toggle_seq */
 
 	unsigned char		hub6;			/* this should be in the 8250 driver */
 	unsigned char		suspended;
-	unsigned char		unused;
 	const char		*name;			/* port name */
 	struct attribute_group	*attr_group;		/* port specific attributes */
 	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 330909c600b1..f6f3e82fee25 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -431,6 +431,14 @@ config MAGIC_SYSRQ_SERIAL
 	  This option allows you to decide whether you want to enable the
 	  magic SysRq key.
 
+config MAGIC_SYSRQ_SERIAL_SEQUENCE
+	string "Char sequence that enables magic SysRq over serial"
+	depends on MAGIC_SYSRQ_SERIAL
+	default ""
+	help
+	  Specifies a sequence of characters that can follow BREAK to enable
+	  SysRq on a serial console.
+
 config DEBUG_FS
 	bool "Debug Filesystem"
 	help
-- 
2.24.1

Re: [PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Randy Dunlap]

Hi,

On 1/9/20 1:54 PM, Dmitry Safonov wrote:
> 
> Based-on-patch-by: Vasiliy Khoruzhick <vasilykh@arista.com>
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---
>  drivers/tty/serial/serial_core.c | 52 ++++++++++++++++++++++++++++----
>  include/linux/serial_core.h      |  2 +-
>  lib/Kconfig.debug                |  8 +++++
>  3 files changed, 55 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 6ac9dfed3423..f70eba032d0b 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
>  }
>  EXPORT_SYMBOL_GPL(uart_insert_char);
>  
> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
> +
> +static void uart_sysrq_on(struct work_struct *w)
> +{
> +	sysrq_toggle_support(1);
> +	pr_info("SysRq is enabled by magic sequience on serial\n");

typo:	                                   sequence

> +}
> +static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);



-- 
~Randy

Re: [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy

[Iurii Zaikin]

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

Re: [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy

[Greg Kroah-Hartman]

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

Re: [PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Greg Kroah-Hartman]

On Thu, Jan 09, 2020 at 09:54:44PM +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.
> 
> As a measure of balance between on and off options, add
> MAGIC_SYSRQ_SERIAL_SEQUENCE which is a string sequence that can enable
> sysrq if it follows BREAK on a serial line. The longer the string - the
> less likely it may be in the garbage.
> 
> 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.
> 
> Based-on-patch-by: Vasiliy Khoruzhick <vasilykh@arista.com>
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---
>  drivers/tty/serial/serial_core.c | 52 ++++++++++++++++++++++++++++----
>  include/linux/serial_core.h      |  2 +-
>  lib/Kconfig.debug                |  8 +++++
>  3 files changed, 55 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 6ac9dfed3423..f70eba032d0b 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
>  }
>  EXPORT_SYMBOL_GPL(uart_insert_char);
>  
> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
> +
> +static void uart_sysrq_on(struct work_struct *w)
> +{
> +	sysrq_toggle_support(1);
> +	pr_info("SysRq is enabled by magic sequience on serial\n");

Do we want to say what serial port it is enabled on?

And why is this done in a workqueue?

> +}
> +static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
> +
> +static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
> +{
> +	if (sysrq_toggle_seq[0] == '\0')
> +		return 0;

Is constantly checking the data stream like this going to slow things
down overall?  Ah, we are just checking this after BREAK, right?  So
that hopefully will not be that bad...

> +
> +	BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);
> +	if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
> +		port->sysrq_seq = 0;
> +		return 0;
> +	}
> +
> +	/* Without the last \0 */
> +	if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
> +		port->sysrq = jiffies + HZ*5;

5 second delay?  You should document what this value is for somewhere
here...

> +		return 1;
> +	}
> +
> +	schedule_work(&sysrq_enable_work);
> +
> +	port->sysrq = 0;
> +	return 1;
> +}
> +
>  int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
>  {
>  	if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
> @@ -3090,9 +3122,13 @@ int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
>  		return 0;
>  
>  	if (ch && time_before(jiffies, port->sysrq)) {
> -		handle_sysrq(ch);
> -		port->sysrq = 0;
> -		return 1;
> +		if (sysrq_get_mask()) {
> +			handle_sysrq(ch);
> +			port->sysrq = 0;
> +			return 1;
> +		}

Isn't this change to test for sysrq_get_mask() a different change than
checking for the "magic" data stream?

> +		if (uart_try_toggle_sysrq(port, ch))
> +			return 1;
>  	}
>  	port->sysrq = 0;
>  
> @@ -3109,9 +3145,13 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
>  		return 0;
>  
>  	if (ch && time_before(jiffies, port->sysrq)) {
> -		port->sysrq_ch = ch;
> -		port->sysrq = 0;
> -		return 1;
> +		if (sysrq_get_mask()) {
> +			port->sysrq_ch = ch;
> +			port->sysrq = 0;
> +			return 1;
> +		}
> +		if (uart_try_toggle_sysrq(port, ch))
> +			return 1;
>  	}
>  	port->sysrq = 0;
>  
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 255e86a474e9..1f4443db5474 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -243,10 +243,10 @@ struct uart_port {
>  	unsigned long		sysrq;			/* sysrq timeout */
>  	unsigned int		sysrq_ch;		/* char for sysrq */
>  	unsigned char		has_sysrq;
> +	unsigned char		sysrq_seq;		/* index in sysrq_toggle_seq */
>  
>  	unsigned char		hub6;			/* this should be in the 8250 driver */
>  	unsigned char		suspended;
> -	unsigned char		unused;

This is an unrelated change, let's leave it for a different patch that
cleans up the layout of this structure, ok?

thanks,

greg k-h

Re: [PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Joe Perches]

On Thu, 2020-01-09 at 21:54 +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.

trivia:

> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
[]
> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
[]
> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;

static const?

> +static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)

This function return might read better as bool not int

> +{
> +	if (sysrq_toggle_seq[0] == '\0')
> +		return 0;
> +
> +	BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);
> +	if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
> +		port->sysrq_seq = 0;
> +		return 0;
> +	}
> +
> +	/* Without the last \0 */
> +	if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
> +		port->sysrq = jiffies + HZ*5;
> +		return 1;
> +	}
> +
> +	schedule_work(&sysrq_enable_work);
> +
> +	port->sysrq = 0;
> +	return 1;
> +}
[]
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 255e86a474e9..1f4443db5474 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -243,10 +243,10 @@ struct uart_port {
>  	unsigned long		sysrq;			/* sysrq timeout */
>  	unsigned int		sysrq_ch;		/* char for sysrq */
>  	unsigned char		has_sysrq;
> +	unsigned char		sysrq_seq;		/* index in sysrq_toggle_seq */

unsigned int?

Or maybe set a maximum length of MAGIC_SYSRQ_SERIAL_SEQUENCE.


>  
>  	unsigned char		hub6;			/* this should be in the 8250 driver */
>  	unsigned char		suspended;
> -	unsigned char		unused;
>  	const char		*name;			/* port name */
>  	struct attribute_group	*attr_group;		/* port specific attributes */
>  	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 330909c600b1..f6f3e82fee25 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -431,6 +431,14 @@ config MAGIC_SYSRQ_SERIAL
>  	  This option allows you to decide whether you want to enable the
>  	  magic SysRq key.
>  
> +config MAGIC_SYSRQ_SERIAL_SEQUENCE
> +	string "Char sequence that enables magic SysRq over serial"
> +	depends on MAGIC_SYSRQ_SERIAL
> +	default ""
> +	help
> +	  Specifies a sequence of characters that can follow BREAK to enable
> +	  SysRq on a serial console.
> +
>  config DEBUG_FS
>  	bool "Debug Filesystem"
>  	help

Re: [PATCH-next 1/3] serial_core: Move sysrq functions from header file

[Greg Kroah-Hartman]

On Thu, Jan 09, 2020 at 09:54:42PM +0000, Dmitry Safonov wrote:
> It's not worth to have them in every serial driver and I'm about to add
> another helper function.
> 
> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---
>  drivers/tty/serial/serial_core.c | 83 +++++++++++++++++++++++++++++++
>  include/linux/serial_core.h      | 84 ++------------------------------
>  2 files changed, 88 insertions(+), 79 deletions(-)

This is a nice cleanup, so I took this patch now, but the last two
patches still need a bit of rework.  Please do that and rebase, no need
to send this patch again.

thanks,

greg k-h

Re: [PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3909 bytes --]

Hi Dmitry,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on next-20200109]
[cannot apply to linux/master usb/usb-testing linus/master v5.5-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dmitry-Safonov/serial-sysrq-Add-MAGIC_SYSRQ_SERIAL_SEQUENCE/20200110-191606
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: microblaze-nommu_defconfig (attached as .config)
compiler: microblaze-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=microblaze 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/tty/serial/serial_core.c:3084:33: error: 'CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE' undeclared here (not in a function)
    const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/tty/serial/serial_core.c: In function 'uart_sysrq_on':
>> drivers/tty/serial/serial_core.c:3088:2: error: implicit declaration of function 'sysrq_toggle_support' [-Werror=implicit-function-declaration]
     sysrq_toggle_support(1);
     ^~~~~~~~~~~~~~~~~~~~
   drivers/tty/serial/serial_core.c: In function 'uart_handle_sysrq_char':
>> drivers/tty/serial/serial_core.c:3125:7: error: implicit declaration of function 'sysrq_get_mask'; did you mean 'xas_get_mark'? [-Werror=implicit-function-declaration]
      if (sysrq_get_mask()) {
          ^~~~~~~~~~~~~~
          xas_get_mark
   cc1: some warnings being treated as errors

vim +/CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE +3084 drivers/tty/serial/serial_core.c

  3083	
> 3084	const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
  3085	
  3086	static void uart_sysrq_on(struct work_struct *w)
  3087	{
> 3088		sysrq_toggle_support(1);
  3089		pr_info("SysRq is enabled by magic sequience on serial\n");
  3090	}
  3091	static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
  3092	
  3093	static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
  3094	{
  3095		if (sysrq_toggle_seq[0] == '\0')
  3096			return 0;
  3097	
  3098		BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);
  3099		if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
  3100			port->sysrq_seq = 0;
  3101			return 0;
  3102		}
  3103	
  3104		/* Without the last \0 */
  3105		if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
  3106			port->sysrq = jiffies + HZ*5;
  3107			return 1;
  3108		}
  3109	
  3110		schedule_work(&sysrq_enable_work);
  3111	
  3112		port->sysrq = 0;
  3113		return 1;
  3114	}
  3115	
  3116	int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
  3117	{
  3118		if (!IS_ENABLED(CONFIG_MAGIC_SYSRQ_SERIAL))
  3119			return 0;
  3120	
  3121		if (!port->has_sysrq || !port->sysrq)
  3122			return 0;
  3123	
  3124		if (ch && time_before(jiffies, port->sysrq)) {
> 3125			if (sysrq_get_mask()) {
  3126				handle_sysrq(ch);
  3127				port->sysrq = 0;
  3128				return 1;
  3129			}
  3130			if (uart_try_toggle_sysrq(port, ch))
  3131				return 1;
  3132		}
  3133		port->sysrq = 0;
  3134	
  3135		return 0;
  3136	}
  3137	EXPORT_SYMBOL_GPL(uart_handle_sysrq_char);
  3138	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 13749 bytes --]

Re: [PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1839 bytes --]

Hi Dmitry,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on next-20200110]
[cannot apply to linux/master usb/usb-testing linus/master v5.5-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dmitry-Safonov/serial-sysrq-Add-MAGIC_SYSRQ_SERIAL_SEQUENCE/20200110-191606
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: mips-randconfig-a001-20200109 (attached as .config)
compiler: mips-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=mips 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/tty/serial/serial_core.c:3084:33: error: 'CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE' undeclared here (not in a function); did you mean 'CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE'?
    const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                    CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE

vim +3084 drivers/tty/serial/serial_core.c

  3083	
> 3084	const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
  3085	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30780 bytes --]

Re: [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy

[Dmitry Safonov]

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

Re: [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy

[Dmitry Safonov]

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

Re: [PATCH-next 2/3] sysctl/sysrq: Remove __sysrq_enabled copy

[Greg Kroah-Hartman]

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

Re: [PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Dmitry Safonov]

Hi Randy,

On 1/9/20 11:53 PM, Randy Dunlap wrote:
> Hi,
> 
> On 1/9/20 1:54 PM, Dmitry Safonov wrote:
>>
>> Based-on-patch-by: Vasiliy Khoruzhick <vasilykh@arista.com>
>> Signed-off-by: Dmitry Safonov <dima@arista.com>
>> ---
>>  drivers/tty/serial/serial_core.c | 52 ++++++++++++++++++++++++++++----
>>  include/linux/serial_core.h      |  2 +-
>>  lib/Kconfig.debug                |  8 +++++
>>  3 files changed, 55 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 6ac9dfed3423..f70eba032d0b 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
>>  }
>>  EXPORT_SYMBOL_GPL(uart_insert_char);
>>  
>> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
>> +
>> +static void uart_sysrq_on(struct work_struct *w)
>> +{
>> +	sysrq_toggle_support(1);
>> +	pr_info("SysRq is enabled by magic sequience on serial\n");
> 
> typo:	                                   sequence

Thanks on catching this,
          Dmitry

Re: [PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Dmitry Safonov]

Hi Joe,

On 1/10/20 4:50 PM, Joe Perches wrote:
> On Thu, 2020-01-09 at 21:54 +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.
> 
> trivia:
> 
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> []
>> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
> []
>> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
> 
> static const?

Will do, thanks!

> 
>> +static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
> 
> This function return might read better as bool not int

Yeah, no hard feelings, will convert.

[..]
>> @@ -243,10 +243,10 @@ struct uart_port {
>>  	unsigned long		sysrq;			/* sysrq timeout */
>>  	unsigned int		sysrq_ch;		/* char for sysrq */
>>  	unsigned char		has_sysrq;
>> +	unsigned char		sysrq_seq;		/* index in sysrq_toggle_seq */
> 
> unsigned int?
> 
> Or maybe set a maximum length of MAGIC_SYSRQ_SERIAL_SEQUENCE.

I think, 256 chars should be enough to send on serial (c)

I'm not aware of a way to put the max string length in Kconfig, so I did
in the patch:
BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >=
sizeof(port->sysrq_seq)*U8_MAX);

Do you have something more elegant in your mind?

Thanks,
          Dmitry

Re: [PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Dmitry Safonov]

On 1/10/20 4:46 PM, Greg Kroah-Hartman wrote:
[..]
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index 6ac9dfed3423..f70eba032d0b 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
>>  }
>>  EXPORT_SYMBOL_GPL(uart_insert_char);
>>  
>> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
>> +
>> +static void uart_sysrq_on(struct work_struct *w)
>> +{
>> +	sysrq_toggle_support(1);
>> +	pr_info("SysRq is enabled by magic sequience on serial\n");
> 
> Do we want to say what serial port it is enabled on?

Makes sense, will add.

> And why is this done in a workqueue?

uart_try_toggle_sysrq() sometimes is called under
spin_lock_irqsave(&port->lock, flags);

And sysrq_toggle_support() calls input_register_handler() internally
which can sleep.

>> +}
>> +static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
>> +
>> +static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
>> +{
>> +	if (sysrq_toggle_seq[0] == '\0')
>> +		return 0;
> 
> Is constantly checking the data stream like this going to slow things
> down overall?  Ah, we are just checking this after BREAK, right?  So
> that hopefully will not be that bad...

Yes, it's after BREAK. In my POV it's fine as originally it would cause
sysrq handler being called (if sysrq is enabled).

> 
>> +
>> +	BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);
>> +	if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
>> +		port->sysrq_seq = 0;
>> +		return 0;
>> +	}
>> +
>> +	/* Without the last \0 */
>> +	if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
>> +		port->sysrq = jiffies + HZ*5;
> 
> 5 second delay?  You should document what this value is for somewhere
> here...

Fair enough, I'll add
#define SYSRQ_TIMEOUT	(HZ*5)

And use it in uart_handle_break() too.

>> @@ -3090,9 +3122,13 @@ int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch)
>>  		return 0;
>>  
>>  	if (ch && time_before(jiffies, port->sysrq)) {
>> -		handle_sysrq(ch);
>> -		port->sysrq = 0;
>> -		return 1;
>> +		if (sysrq_get_mask()) {
>> +			handle_sysrq(ch);
>> +			port->sysrq = 0;
>> +			return 1;
>> +		}
> 
> Isn't this change to test for sysrq_get_mask() a different change than
> checking for the "magic" data stream?

It's for the case when sysrq is already enabled.
Than sysrq_get_mask() will return something and it makes uart call
handle_sysrq() instead of checking the toggle sequence.

>> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
>> index 255e86a474e9..1f4443db5474 100644
>> --- a/include/linux/serial_core.h
>> +++ b/include/linux/serial_core.h
>> @@ -243,10 +243,10 @@ struct uart_port {
>>  	unsigned long		sysrq;			/* sysrq timeout */
>>  	unsigned int		sysrq_ch;		/* char for sysrq */
>>  	unsigned char		has_sysrq;
>> +	unsigned char		sysrq_seq;		/* index in sysrq_toggle_seq */
>>  
>>  	unsigned char		hub6;			/* this should be in the 8250 driver */
>>  	unsigned char		suspended;
>> -	unsigned char		unused;
> 
> This is an unrelated change, let's leave it for a different patch that
> cleans up the layout of this structure, ok?

Yes, sure.

Thanks,
          Dmitry

Re: [PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[kbuild test robot]

Hi Dmitry,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on next-20200110]
[cannot apply to linux/master usb/usb-testing linus/master v5.5-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Dmitry-Safonov/serial-sysrq-Add-MAGIC_SYSRQ_SERIAL_SEQUENCE/20200110-191606
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

New smatch warnings:
drivers/tty/serial/serial_core.c:3105 uart_try_toggle_sysrq() warn: unsigned '++port->sysrq_seq' is never less than zero.

Old smatch warnings:
drivers/tty/serial/serial_core.c:295 uart_shutdown() error: we previously assumed 'uport' could be null (see line 291)
drivers/tty/serial/serial_core.c:2729 uart_get_attr_iomem_base() warn: argument 4 to %lX specifier is cast from pointer

vim +3105 drivers/tty/serial/serial_core.c

  3092	
  3093	static int uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
  3094	{
  3095		if (sysrq_toggle_seq[0] == '\0')
  3096			return 0;
  3097	
  3098		BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= sizeof(port->sysrq_seq)*U8_MAX);
  3099		if (sysrq_toggle_seq[port->sysrq_seq] != ch) {
  3100			port->sysrq_seq = 0;
  3101			return 0;
  3102		}
  3103	
  3104		/* Without the last \0 */
> 3105		if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
  3106			port->sysrq = jiffies + HZ*5;
  3107			return 1;
  3108		}
  3109	
  3110		schedule_work(&sysrq_enable_work);
  3111	
  3112		port->sysrq = 0;
  3113		return 1;
  3114	}
  3115	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

Re: [PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Joe Perches]

On Fri, 2020-01-10 at 22:10 +0000, Dmitry Safonov wrote:
> Hi Joe,

Hi Dmitry.

> I'm not aware of a way to put the max string length in Kconfig,

Nor am I.


>  so I did
> in the patch:
> BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >=
> sizeof(port->sysrq_seq)*U8_MAX);
> 
> Do you have something more elegant in your mind?

No, I didn't notice this and think this is fine
so thanks for that.

It's be nice to have something like BUILD_BUG_ON_MSG.

Re: [PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Dmitry Safonov]

On 1/10/20 10:32 PM, Dmitry Safonov wrote:
> On 1/10/20 4:46 PM, Greg Kroah-Hartman wrote:
> [..]
>>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>>> index 6ac9dfed3423..f70eba032d0b 100644
>>> --- a/drivers/tty/serial/serial_core.c
>>> +++ b/drivers/tty/serial/serial_core.c
>>> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
>>>  }
>>>  EXPORT_SYMBOL_GPL(uart_insert_char);
>>>  
>>> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
>>> +
>>> +static void uart_sysrq_on(struct work_struct *w)
>>> +{
>>> +	sysrq_toggle_support(1);
>>> +	pr_info("SysRq is enabled by magic sequience on serial\n");
>>
>> Do we want to say what serial port it is enabled on?
> 
> Makes sense, will add.

Ah, I've managed to forget to mention that I didn't add the port name
into the message in v2. I experimented a bit - it's getting a bit
complicated how-to protect (char *name) for just this message.
Like, SysRq can be theoretically enabled on two serials at the same
moment - so some locking is needed to make the printed name sane.

As sysrq_toggle_support() is a global-enable knob for sysrq (also can be
switched in /proc/sys/kernel/sysrq) I'm not sure if it's worth to
complicate code to print through which serial console SysRq has been
enabled.

I can still do it in v3 if you insist.

And sorry about forgetting to mention this - thought I'll write reply
after I send v2 and somehow it slipped my mind.

Thanks,
           Dmitry

Re: [PATCH-next 3/3] serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE

[Greg Kroah-Hartman]

On Tue, Jan 14, 2020 at 07:10:47PM +0000, Dmitry Safonov wrote:
> On 1/10/20 10:32 PM, Dmitry Safonov wrote:
> > On 1/10/20 4:46 PM, Greg Kroah-Hartman wrote:
> > [..]
> >>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> >>> index 6ac9dfed3423..f70eba032d0b 100644
> >>> --- a/drivers/tty/serial/serial_core.c
> >>> +++ b/drivers/tty/serial/serial_core.c
> >>> @@ -3081,6 +3081,38 @@ void uart_insert_char(struct uart_port *port, unsigned int status,
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(uart_insert_char);
> >>>  
> >>> +const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
> >>> +
> >>> +static void uart_sysrq_on(struct work_struct *w)
> >>> +{
> >>> +	sysrq_toggle_support(1);
> >>> +	pr_info("SysRq is enabled by magic sequience on serial\n");
> >>
> >> Do we want to say what serial port it is enabled on?
> > 
> > Makes sense, will add.
> 
> Ah, I've managed to forget to mention that I didn't add the port name
> into the message in v2. I experimented a bit - it's getting a bit
> complicated how-to protect (char *name) for just this message.
> Like, SysRq can be theoretically enabled on two serials at the same
> moment - so some locking is needed to make the printed name sane.
> 
> As sysrq_toggle_support() is a global-enable knob for sysrq (also can be
> switched in /proc/sys/kernel/sysrq) I'm not sure if it's worth to
> complicate code to print through which serial console SysRq has been
> enabled.
> 
> I can still do it in v3 if you insist.

No worries, if it's too hard to get to, that's fine.  If it was
available, it would have been nice to have.

thanks,

greg k-h