[PATCH printk v2 00/38] reduce console_lock scope

[PATCH printk v2 00/38] reduce console_lock scope

[John Ogness]

This is v2 of a series to prepare for threaded/atomic
printing. It is a rework of patches 6-12 of the v1 [0]. From
the v1, patches 1-5 are already mainline and a rework of
patches >12 will be posted in a later series.

This series focuses on reducing the scope of the BKL
console_lock. It achieves this by switching to SRCU and a
dedicated mutex for console list iteration and modification,
respectively. The console_lock will no longer offer this
protection and is completely removed from
(un)register_console() and console_stop/start() code.

All users of the console_lock for list iteration have been
modified. For the call sites where the console_lock is still
needed (because of other reasons), I added comments to explain
exactly why the console_lock was needed.

The base commit for this series is from Paul McKenney's RCU tree
and provides an NMI-safe SRCU implementation [1]. Without the
NMI-safe SRCU implementation, this series is not less safe than
mainline. But we will need the NMI-safe SRCU implementation for
atomic consoles anyway, so we might as well get it in
now. Especially since it _does_ increase the reliability for
mainline in the panic path.

Changes since v2:

general:

- introduce console_is_enabled() to document safe data race on
  console->flags

- switch all "console->flags & CON_ENABLED" code sites to
  console_is_enabled()

- add "for_each_console_srcu" to .clang-format

- cleanup/clarify comments relating to console_lock
  coverage/usage

um:

- kmsg_dumper: use srcu instead of console_lock for list
  iteration

kgdb/kdb:

- configure_kgdboc: keep console_lock for console->device()
  synchronization, use srcu for list iteration

- kgdboc_earlycon_pre_exp_handler: use srcu instead of
  documenting unsafety for list iteration

- kgdboc_earlycon_init: use console_list_lock instead of
  console_lock to lock list

- kdb_msg_write: use srcu instead of documenting unsafety for
  list iteration

tty:

- show_cons_active: keep console_lock for console->device()
  synchronization

fbdev:

- xen-fbfront: xenfb_probe: use srcu instead of console_lock
  for list iteration, introduce console_force_preferred() to
  safely implement hack

proc/consoles:

- show_console_dev: keep console_lock for console->device()
  synchronization

- c_next: use hlist_entry_safe() instead of
  hlist_for_each_entry_continue()

printk:

- remove console_lock from console_stop/start() and
  (un)register_console()

- introduce console_srcu_read_(un)lock() to wrap scru read
  (un)lock

- rename cons_first() macro to console_first()

- for_each_console: add lockdep check instead of introducing
  new for_each_registered_console()

- console_list_lock: add warning if in read-side critical
  section

- release srcu read lock on handover

- console_flush_all: use srcu instead of relying on console
  lock for list iteration

- console_unblank: use srcu instead of relying on console_lock
  for list iteration

- console_flush_on_panic: use srcu for list iteration and
  document console->seq race

- device: keep console_lock for console->device()
  synchronization, usr srcu for list iteration

- register_console: split list adding logic into the 3 distinct
  scenarios

- register_console: set initial sequence number before adding
  to list

- unregister_console: fix ENODEV return value if the console is
  not registered

- console_stop: synchronize srcu

- printk_late_init: use _safe variant of iteration

- __pr_flush: use srcu instead of relying on console_lock for
  list iteration

John Ogness

[0] https://lore.kernel.org/r/20220924000454.3319186-1-john.ogness@linutronix.de
[1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.10.18b

John Ogness (37):
  printk: Convert console_drivers list to hlist
  printk: Prepare for SRCU console list protection
  printk: introduce console_is_enabled() wrapper
  printk: use console_is_enabled()
  tty: nfcon: use console_is_enabled()
  um: kmsg_dump: use console_is_enabled()
  efi: earlycon: use console_is_enabled()
  netconsole: use console_is_enabled()
  tty: hvc: use console_is_enabled()
  tty: serial: earlycon: use console_is_enabled()
  tty: serial: kgdboc: use console_is_enabled()
  tty: serial: pic32_uart: use console_is_enabled()
  tty: serial: samsung_tty: use console_is_enabled()
  tty: serial: serial_core: use console_is_enabled()
  tty: serial: xilinx_uartps: use console_is_enabled()
  tty: tty_io: use console_is_enabled()
  usb: early: xhci-dbc: use console_is_enabled()
  kdb: kdb_io: use console_is_enabled()
  um: kmsg_dumper: use srcu console list iterator
  serial: kgdboc: use srcu console list iterator
  serial: kgdboc: document console_lock usage
  tty: tty_io: document console_lock usage
  xen: fbfront: use srcu console list iterator
  proc: consoles: document console_lock usage
  kdb: use srcu console list iterator
  printk: console_flush_all: use srcu console list iterator
  printk: console_unblank: use srcu console list iterator
  printk: console_flush_on_panic: use srcu console list iterator
  printk: console_device: use srcu console list iterator
  printk: register_console: use srcu console list iterator
  printk: __pr_flush: use srcu console list iterator
  printk: introduce console_list_lock
  serial: kgdboc: use console_list_lock instead of console_lock
  tty: tty_io: use console_list_lock for list synchronization
  proc: consoles: use console_list_lock for list iteration
  printk: relieve console_lock of list synchronization duties
  printk, xen: fbfront: create/use safe function for forcing preferred

Thomas Gleixner (1):
  serial: kgdboc: Lock console list in probe function

 .clang-format                      |   1 +
 arch/m68k/emu/nfcon.c              |   4 +-
 arch/um/kernel/kmsg_dump.c         |  15 +-
 drivers/firmware/efi/earlycon.c    |   4 +-
 drivers/net/netconsole.c           |   4 +-
 drivers/tty/hvc/hvc_console.c      |   2 +-
 drivers/tty/serial/earlycon.c      |   4 +-
 drivers/tty/serial/kgdboc.c        |  37 ++-
 drivers/tty/serial/pic32_uart.c    |   2 +-
 drivers/tty/serial/samsung_tty.c   |   2 +-
 drivers/tty/serial/serial_core.c   |   2 +-
 drivers/tty/serial/xilinx_uartps.c |   2 +-
 drivers/tty/tty_io.c               |  18 +-
 drivers/usb/early/xhci-dbc.c       |   2 +-
 drivers/video/fbdev/xen-fbfront.c  |  16 +-
 fs/proc/consoles.c                 |  20 +-
 include/linux/console.h            |  75 +++++-
 include/linux/serial_core.h        |   2 +-
 kernel/debug/kdb/kdb_io.c          |   7 +-
 kernel/printk/printk.c             | 373 +++++++++++++++++++++--------
 20 files changed, 438 insertions(+), 154 deletions(-)


base-commit: c2d158a284abd63d727dad7402a2eed650dd4233
-- 
2.30.2

[PATCH printk v2 01/38] serial: kgdboc: Lock console list in probe function

[John Ogness]

From: Thomas Gleixner <tglx@linutronix.de>

Unprotected list walks are not necessarily safe.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 drivers/tty/serial/kgdboc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index 7aa37be3216a..e76f0186c335 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -193,6 +193,7 @@ static int configure_kgdboc(void)
 	if (!p)
 		goto noconfig;
 
+	console_lock();
 	for_each_console(cons) {
 		int idx;
 		if (cons->device && cons->device(cons, &idx) == p &&
@@ -201,6 +202,7 @@ static int configure_kgdboc(void)
 			break;
 		}
 	}
+	console_unlock();
 
 	kgdb_tty_driver = p;
 	kgdb_tty_line = tty_line;
-- 
2.30.2

[PATCH printk v2 11/38] tty: serial: earlycon: use console_is_enabled()

[John Ogness]

Replace (console->flags & CON_ENABLED) usage with console_is_enabled().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/earlycon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index a5f380584cda..bdfb60da97bd 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -181,7 +181,7 @@ int __init setup_earlycon(char *buf)
 	if (!buf || !buf[0])
 		return -EINVAL;
 
-	if (early_con.flags & CON_ENABLED)
+	if (console_is_enabled(&early_con))
 		return -EALREADY;
 
 again:
@@ -253,7 +253,7 @@ int __init of_setup_earlycon(const struct earlycon_id *match,
 	bool big_endian;
 	u64 addr;
 
-	if (early_con.flags & CON_ENABLED)
+	if (console_is_enabled(&early_con))
 		return -EALREADY;
 
 	spin_lock_init(&port->lock);
-- 
2.30.2

[PATCH printk v2 12/38] tty: serial: kgdboc: use console_is_enabled()

[John Ogness]

Replace (console->flags & CON_ENABLED) usage with console_is_enabled().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/kgdboc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index e76f0186c335..b17aa7e49894 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -533,7 +533,7 @@ static int __init kgdboc_earlycon_init(char *opt)
 	console_lock();
 	for_each_console(con) {
 		if (con->write && con->read &&
-		    (con->flags & (CON_BOOT | CON_ENABLED)) &&
+		    (console_is_enabled(con) || (con->flags & CON_BOOT)) &&
 		    (!opt || !opt[0] || strcmp(con->name, opt) == 0))
 			break;
 	}
-- 
2.30.2

[PATCH printk v2 13/38] tty: serial: pic32_uart: use console_is_enabled()

[John Ogness]

Replace (console->flags & CON_ENABLED) usage with console_is_enabled().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/pic32_uart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/pic32_uart.c b/drivers/tty/serial/pic32_uart.c
index 2beada66c824..d70344dc6e8a 100644
--- a/drivers/tty/serial/pic32_uart.c
+++ b/drivers/tty/serial/pic32_uart.c
@@ -843,7 +843,7 @@ console_initcall(pic32_console_init);
  */
 static int __init pic32_late_console_init(void)
 {
-	if (!(pic32_console.flags & CON_ENABLED))
+	if (!console_is_enabled(&pic32_console))
 		register_console(&pic32_console);
 
 	return 0;
-- 
2.30.2

[PATCH printk v2 14/38] tty: serial: samsung_tty: use console_is_enabled()

[John Ogness]

Replace (console->flags & CON_ENABLED) usage with console_is_enabled().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/samsung_tty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 77d1363029f5..8142a0e53ffa 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1732,7 +1732,7 @@ static void __init s3c24xx_serial_register_console(void)
 
 static void s3c24xx_serial_unregister_console(void)
 {
-	if (s3c24xx_serial_console.flags & CON_ENABLED)
+	if (console_is_enabled(&s3c24xx_serial_console))
 		unregister_console(&s3c24xx_serial_console);
 }
 
-- 
2.30.2

[PATCH printk v2 15/38] tty: serial: serial_core: use console_is_enabled()

[John Ogness]

Replace (console->flags & CON_ENABLED) usage with console_is_enabled().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/serial_core.c | 2 +-
 include/linux/serial_core.h      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 179ee199df34..ebf609e4e179 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2573,7 +2573,7 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 		 * successfully registered yet, try to re-register it.
 		 * It may be that the port was not available.
 		 */
-		if (port->cons && !(port->cons->flags & CON_ENABLED))
+		if (port->cons && !console_is_enabled(port->cons))
 			register_console(port->cons);
 
 		/*
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index d657f2a42a7b..ed5d1aeb91e1 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -745,7 +745,7 @@ static inline int setup_earlycon(char *buf) { return 0; }
 
 static inline bool uart_console_enabled(struct uart_port *port)
 {
-	return uart_console(port) && (port->cons->flags & CON_ENABLED);
+	return uart_console(port) && console_is_enabled(port->cons);
 }
 
 struct uart_port *uart_get_console(struct uart_port *ports, int nr,
-- 
2.30.2

[PATCH printk v2 16/38] tty: serial: xilinx_uartps: use console_is_enabled()

[John Ogness]

Replace (console->flags & CON_ENABLED) usage with console_is_enabled().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/xilinx_uartps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 2eff7cff57c4..e1fe95bd55c1 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1631,7 +1631,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
 #ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
 	/* This is not port which is used for console that's why clean it up */
 	if (console_port == port &&
-	    !(cdns_uart_uart_driver.cons->flags & CON_ENABLED)) {
+	    !console_is_enabled(cdns_uart_uart_driver.cons)) {
 		console_port = NULL;
 		cdns_uart_console.index = -1;
 	}
-- 
2.30.2

[PATCH printk v2 21/38] serial: kgdboc: use srcu console list iterator

[John Ogness]

Use srcu console list iteration for safe console list traversal.

Note that configure_kgdboc() still requires the console_lock in
order to ensure that no console is in its write() callback when
its direct() callback is called. Add comments to clarify this.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/kgdboc.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index b17aa7e49894..e9d3f8c6e3dc 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -171,6 +171,7 @@ static int configure_kgdboc(void)
 	int err = -ENODEV;
 	char *cptr = config;
 	struct console *cons;
+	int cookie;
 
 	if (!strlen(config) || isspace(config[0])) {
 		err = 0;
@@ -193,8 +194,14 @@ static int configure_kgdboc(void)
 	if (!p)
 		goto noconfig;
 
+	/*
+	 * Stop console printing because the device() callback may
+	 * assume the console is not within its write() callback.
+	 */
 	console_lock();
-	for_each_console(cons) {
+
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(cons) {
 		int idx;
 		if (cons->device && cons->device(cons, &idx) == p &&
 		    idx == tty_line) {
@@ -202,6 +209,8 @@ static int configure_kgdboc(void)
 			break;
 		}
 	}
+	console_srcu_read_unlock(cookie);
+
 	console_unlock();
 
 	kgdb_tty_driver = p;
@@ -451,6 +460,7 @@ static void kgdboc_earlycon_pre_exp_handler(void)
 {
 	struct console *con;
 	static bool already_warned;
+	int cookie;
 
 	if (already_warned)
 		return;
@@ -463,9 +473,14 @@ static void kgdboc_earlycon_pre_exp_handler(void)
 	 * serial drivers might be OK with this, print a warning once per
 	 * boot if we detect this case.
 	 */
-	for_each_console(con)
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(con) {
 		if (con == kgdboc_earlycon_io_ops.cons)
-			return;
+			break;
+	}
+	console_srcu_read_unlock(cookie);
+	if (con)
+		return;
 
 	already_warned = true;
 	pr_warn("kgdboc_earlycon is still using bootconsole\n");
-- 
2.30.2

[PATCH printk v2 22/38] serial: kgdboc: document console_lock usage

[John Ogness]

kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
are unregistered until the kgdboc_earlycon is setup. This is necessary
because the trapping of the exit() callback assumes that the exit()
callback is not called before the trap is setup.

Explicitly document this non-typical console_lock usage.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/kgdboc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index e9d3f8c6e3dc..48000666789a 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -545,6 +545,14 @@ static int __init kgdboc_earlycon_init(char *opt)
 	 * Look for a matching console, or if the name was left blank just
 	 * pick the first one we find.
 	 */
+
+	/*
+	 * Hold the console_lock to guarantee that no consoles are
+	 * unregistered until the kgdboc_earlycon setup is complete.
+	 * Trapping the exit() callback relies on exit() not being
+	 * called until the trap is setup. This also allows safe
+	 * traversal of the console list.
+	 */
 	console_lock();
 	for_each_console(con) {
 		if (con->write && con->read &&
-- 
2.30.2

[PATCH printk v2 34/38] serial: kgdboc: use console_list_lock instead of console_lock

[John Ogness]

kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
are unregistered until the kgdboc_earlycon is setup. The console_list_lock
should be used instead because list synchronization repsponsibility will
be removed from the console_lock in a later change.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/kgdboc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index 48000666789a..0d85596aebe2 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -547,13 +547,13 @@ static int __init kgdboc_earlycon_init(char *opt)
 	 */
 
 	/*
-	 * Hold the console_lock to guarantee that no consoles are
+	 * Hold the console_list_lock to guarantee that no consoles are
 	 * unregistered until the kgdboc_earlycon setup is complete.
 	 * Trapping the exit() callback relies on exit() not being
 	 * called until the trap is setup. This also allows safe
 	 * traversal of the console list.
 	 */
-	console_lock();
+	console_list_lock();
 	for_each_console(con) {
 		if (con->write && con->read &&
 		    (console_is_enabled(con) || (con->flags & CON_BOOT)) &&
@@ -595,7 +595,7 @@ static int __init kgdboc_earlycon_init(char *opt)
 	}
 
 unlock:
-	console_unlock();
+	console_list_unlock();
 
 	/* Non-zero means malformed option so we always return zero */
 	return 0;
-- 
2.30.2

Re: [PATCH printk v2 01/38] serial: kgdboc: Lock console list in probe function

[Greg Kroah-Hartman]

On Wed, Oct 19, 2022 at 05:01:23PM +0206, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Unprotected list walks are not necessarily safe.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> ---
>  drivers/tty/serial/kgdboc.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH printk v2 14/38] tty: serial: samsung_tty: use console_is_enabled()

[Greg Kroah-Hartman]

On Wed, Oct 19, 2022 at 05:01:36PM +0206, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/samsung_tty.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH printk v2 15/38] tty: serial: serial_core: use console_is_enabled()

[Greg Kroah-Hartman]

On Wed, Oct 19, 2022 at 05:01:37PM +0206, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH printk v2 12/38] tty: serial: kgdboc: use console_is_enabled()

[Greg Kroah-Hartman]

On Wed, Oct 19, 2022 at 05:01:34PM +0206, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH printk v2 13/38] tty: serial: pic32_uart: use console_is_enabled()

[Greg Kroah-Hartman]

On Wed, Oct 19, 2022 at 05:01:35PM +0206, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH printk v2 11/38] tty: serial: earlycon: use console_is_enabled()

[Greg Kroah-Hartman]

On Wed, Oct 19, 2022 at 05:01:33PM +0206, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH printk v2 16/38] tty: serial: xilinx_uartps: use console_is_enabled()

[Greg Kroah-Hartman]

On Wed, Oct 19, 2022 at 05:01:38PM +0206, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH printk v2 21/38] serial: kgdboc: use srcu console list iterator

[Greg Kroah-Hartman]

On Wed, Oct 19, 2022 at 05:01:43PM +0206, John Ogness wrote:
> Use srcu console list iteration for safe console list traversal.
> 
> Note that configure_kgdboc() still requires the console_lock in
> order to ensure that no console is in its write() callback when
> its direct() callback is called. Add comments to clarify this.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH printk v2 22/38] serial: kgdboc: document console_lock usage

[Greg Kroah-Hartman]

On Wed, Oct 19, 2022 at 05:01:44PM +0206, John Ogness wrote:
> kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
> are unregistered until the kgdboc_earlycon is setup. This is necessary
> because the trapping of the exit() callback assumes that the exit()
> callback is not called before the trap is setup.
> 
> Explicitly document this non-typical console_lock usage.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/kgdboc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index e9d3f8c6e3dc..48000666789a 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -545,6 +545,14 @@ static int __init kgdboc_earlycon_init(char *opt)
>  	 * Look for a matching console, or if the name was left blank just
>  	 * pick the first one we find.
>  	 */
> +
> +	/*
> +	 * Hold the console_lock to guarantee that no consoles are
> +	 * unregistered until the kgdboc_earlycon setup is complete.
> +	 * Trapping the exit() callback relies on exit() not being
> +	 * called until the trap is setup. This also allows safe
> +	 * traversal of the console list.
> +	 */
>  	console_lock();
>  	for_each_console(con) {
>  		if (con->write && con->read &&
> -- 
> 2.30.2
>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH printk v2 34/38] serial: kgdboc: use console_list_lock instead of console_lock

[Greg Kroah-Hartman]

On Wed, Oct 19, 2022 at 05:01:56PM +0206, John Ogness wrote:
> kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
> are unregistered until the kgdboc_earlycon is setup. The console_list_lock
> should be used instead because list synchronization repsponsibility will
> be removed from the console_lock in a later change.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/kgdboc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH printk v2 11/38] tty: serial: earlycon: use console_is_enabled()

[Petr Mladek]

On Wed 2022-10-19 17:01:33, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 12/38] tty: serial: kgdboc: use console_is_enabled()

[Petr Mladek]

On Wed 2022-10-19 17:01:34, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 13/38] tty: serial: pic32_uart: use console_is_enabled()

[Petr Mladek]

On Wed 2022-10-19 17:01:35, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 14/38] tty: serial: samsung_tty: use console_is_enabled()

[Petr Mladek]

On Wed 2022-10-19 17:01:36, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/samsung_tty.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 77d1363029f5..8142a0e53ffa 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -1732,7 +1732,7 @@ static void __init s3c24xx_serial_register_console(void)
>  
>  static void s3c24xx_serial_unregister_console(void)
>  {
> -	if (s3c24xx_serial_console.flags & CON_ENABLED)
> +	if (console_is_enabled(&s3c24xx_serial_console))
>  		unregister_console(&s3c24xx_serial_console);
>  }

As on many other locations, it would be better to check if the console
is in console_list. CON_ENABLED might be disabled even when the
console is registered.

It would be nice to fix this. But it might be done later.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 15/38] tty: serial: serial_core: use console_is_enabled()

[Petr Mladek]

On Wed 2022-10-19 17:01:37, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 16/38] tty: serial: xilinx_uartps: use console_is_enabled()

[Petr Mladek]

On Wed 2022-10-19 17:01:38, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/xilinx_uartps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
> index 2eff7cff57c4..e1fe95bd55c1 100644
> --- a/drivers/tty/serial/xilinx_uartps.c
> +++ b/drivers/tty/serial/xilinx_uartps.c
> @@ -1631,7 +1631,7 @@ static int cdns_uart_probe(struct platform_device *pdev)
>  #ifdef CONFIG_SERIAL_XILINX_PS_UART_CONSOLE
>  	/* This is not port which is used for console that's why clean it up */
>  	if (console_port == port &&
> -	    !(cdns_uart_uart_driver.cons->flags & CON_ENABLED)) {
> +	    !console_is_enabled(cdns_uart_uart_driver.cons)) {
>  		console_port = NULL;
>  		cdns_uart_console.index = -1;

Again, IMHO, we should check here if the console is in console_list.
We should not clean the port and index when the console is already
registered.

Again, this is old problem that might be fixed later.

The change is straightforward. Feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 21/38] serial: kgdboc: use srcu console list iterator

[Petr Mladek]

On Wed 2022-10-19 17:01:43, John Ogness wrote:
> Use srcu console list iteration for safe console list traversal.
>
> Note that configure_kgdboc() still requires the console_lock in
> order to ensure that no console is in its write() callback when
> its direct() callback is called. Add comments to clarify this.

s/direct()/device()/

Do you know about such requirements or is it just
a conservative approach, please?

I ask because the comment in the code says "may assume".


Anyway, this would deserve a comment why the SRCU list iteration is
needed even when console_lock() is needed as well.

The reason is that further patches are going to synchronize
console_list manipulation with another lock and console_lock()
will be used only to serialize accessing con->write() callbacks.

Best Regards,
Petr

> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/kgdboc.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index b17aa7e49894..e9d3f8c6e3dc 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -171,6 +171,7 @@ static int configure_kgdboc(void)
>  	int err = -ENODEV;
>  	char *cptr = config;
>  	struct console *cons;
> +	int cookie;
>  
>  	if (!strlen(config) || isspace(config[0])) {
>  		err = 0;
> @@ -193,8 +194,14 @@ static int configure_kgdboc(void)
>  	if (!p)
>  		goto noconfig;
>  
> +	/*
> +	 * Stop console printing because the device() callback may
> +	 * assume the console is not within its write() callback.
> +	 */
>  	console_lock();
> -	for_each_console(cons) {
> +
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(cons) {
>  		int idx;
>  		if (cons->device && cons->device(cons, &idx) == p &&
>  		    idx == tty_line) {
> @@ -202,6 +209,8 @@ static int configure_kgdboc(void)
>  			break;
>  		}
>  	}
> +	console_srcu_read_unlock(cookie);
> +
>  	console_unlock();
>  
>  	kgdb_tty_driver = p;
> @@ -451,6 +460,7 @@ static void kgdboc_earlycon_pre_exp_handler(void)
>  {
>  	struct console *con;
>  	static bool already_warned;
> +	int cookie;
>  
>  	if (already_warned)
>  		return;
> @@ -463,9 +473,14 @@ static void kgdboc_earlycon_pre_exp_handler(void)
>  	 * serial drivers might be OK with this, print a warning once per
>  	 * boot if we detect this case.
>  	 */
> -	for_each_console(con)
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(con) {
>  		if (con == kgdboc_earlycon_io_ops.cons)
> -			return;
> +			break;
> +	}
> +	console_srcu_read_unlock(cookie);
> +	if (con)
> +		return;
>  
>  	already_warned = true;
>  	pr_warn("kgdboc_earlycon is still using bootconsole\n");
> -- 
> 2.30.2

Re: [PATCH printk v2 01/38] serial: kgdboc: Lock console list in probe function

[Sergey Senozhatsky]

On (22/10/19 17:01), John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Unprotected list walks are not necessarily safe.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH printk v2 12/38] tty: serial: kgdboc: use console_is_enabled()

[Doug Anderson]

Hi,

On Wed, Oct 19, 2022 at 7:56 AM John Ogness <john.ogness@linutronix.de> wrote:
>
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/kgdboc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index e76f0186c335..b17aa7e49894 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -533,7 +533,7 @@ static int __init kgdboc_earlycon_init(char *opt)
>         console_lock();
>         for_each_console(con) {
>                 if (con->write && con->read &&
> -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> +                   (console_is_enabled(con) || (con->flags & CON_BOOT)) &&

<shrug>. I guess this is OK, but it feels a little pointless. If we're
still directly looking at the CON_BOOT bit in con->flags it seems
weird to be accessing CON_ENABLED through a special wrapper that's
marked as a `data_race`. In our case it's _not_ a data race, right,
since this function continues to hold the console_lock() even at the
end of the series? I personally would drop this patch but if you
really want it I won't object.

-Doug

Re: [PATCH printk v2 21/38] serial: kgdboc: use srcu console list iterator

[Doug Anderson]

Hi,

On Fri, Oct 21, 2022 at 8:09 AM Petr Mladek <pmladek@suse.com> wrote:
>
> On Wed 2022-10-19 17:01:43, John Ogness wrote:
> > Use srcu console list iteration for safe console list traversal.
> >
> > Note that configure_kgdboc() still requires the console_lock in
> > order to ensure that no console is in its write() callback when
> > its direct() callback is called. Add comments to clarify this.
>
> s/direct()/device()/
>
> Do you know about such requirements or is it just
> a conservative approach, please?
>
> I ask because the comment in the code says "may assume".
>
>
> Anyway, this would deserve a comment why the SRCU list iteration is
> needed even when console_lock() is needed as well.
>
> The reason is that further patches are going to synchronize
> console_list manipulation with another lock and console_lock()
> will be used only to serialize accessing con->write() callbacks.

I had the same concern. I'll note that at the end of the series the
documentation for console_lock() still says:

 * Acquires a lock which guarantees that the caller has
 * exclusive access to the console system.

That seems to imply (at least to me) that if you're holding
console_lock() there's no need to hold the SRCU lock.

-Doug

Re: [PATCH printk v2 01/38] serial: kgdboc: Lock console list in probe function

[Doug Anderson]

Hi,

On Wed, Oct 19, 2022 at 7:56 AM John Ogness <john.ogness@linutronix.de> wrote:
>
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Unprotected list walks are not necessarily safe.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> ---
>  drivers/tty/serial/kgdboc.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH printk v2 22/38] serial: kgdboc: document console_lock usage

[Doug Anderson]

Hi,

On Wed, Oct 19, 2022 at 7:56 AM John Ogness <john.ogness@linutronix.de> wrote:
>
> kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
> are unregistered until the kgdboc_earlycon is setup. This is necessary
> because the trapping of the exit() callback assumes that the exit()
> callback is not called before the trap is setup.
>
> Explicitly document this non-typical console_lock usage.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/kgdboc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH printk v2 12/38] tty: serial: kgdboc: use console_is_enabled()

[Doug Anderson]

Hi,

On Mon, Oct 24, 2022 at 3:46 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Oct 19, 2022 at 7:56 AM John Ogness <john.ogness@linutronix.de> wrote:
> >
> > Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> >
> > Signed-off-by: John Ogness <john.ogness@linutronix.de>
> > ---
> >  drivers/tty/serial/kgdboc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > index e76f0186c335..b17aa7e49894 100644
> > --- a/drivers/tty/serial/kgdboc.c
> > +++ b/drivers/tty/serial/kgdboc.c
> > @@ -533,7 +533,7 @@ static int __init kgdboc_earlycon_init(char *opt)
> >         console_lock();
> >         for_each_console(con) {
> >                 if (con->write && con->read &&
> > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > +                   (console_is_enabled(con) || (con->flags & CON_BOOT)) &&
>
> <shrug>. I guess this is OK, but it feels a little pointless. If we're
> still directly looking at the CON_BOOT bit in con->flags it seems
> weird to be accessing CON_ENABLED through a special wrapper that's
> marked as a `data_race`. In our case it's _not_ a data race, right,
> since this function continues to hold the console_lock() even at the
> end of the series? I personally would drop this patch but if you
> really want it I won't object.

I realized that my statement isn't quite true. It actually only holds
console_list_lock() even at the end of the series. Still, it seems
weird that we're declaring the `data_race` on CON_ENABLED but not
CON_BOOT ?

Re: [PATCH printk v2 22/38] serial: kgdboc: document console_lock usage

[Petr Mladek]

On Wed 2022-10-19 17:01:44, John Ogness wrote:
> kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
> are unregistered until the kgdboc_earlycon is setup. This is necessary
> because the trapping of the exit() callback assumes that the exit()
> callback is not called before the trap is setup.

Great catch!

Note: This behavior might be dangerous. printk_late_init() checks if
      the early console code is in the init section. It unregisters
      such consoles even when keep_bootcon parameter is used.

      Well, it is "not serious" and might be improved later.
      If the early console has this code in the early section
      then it is a bug and should be fixed. printk_late_init() does
      the best effort because this problem would block all consoles
      and would be hard to debug. It is the same with kgdb but
      it is still only a corner case and just the best effort.

> Explicitly document this non-typical console_lock usage.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 12/38] tty: serial: kgdboc: use console_is_enabled()

[John Ogness]

On 2022-10-24, Doug Anderson <dianders@chromium.org> wrote:
> It actually only holds console_list_lock() even at the end of the
> series. Still, it seems weird that we're declaring the `data_race` on
> CON_ENABLED but not CON_BOOT ?

You are correct that it is not a data race (because of the
console_list_lock being held.) Petr has suggested adding a separate
function for non-data-race callers. For v3 I will do this and use it
here, probably called console_is_enabled_locked().

Usually CON_ENABLED is the only flag that is interesting during normal
operation. The kgdboc case is an exception. I thought about if we should
have console_flags() and console_flags_locked() to be able to handle
general con->flags access. console_flags() would be marked with
data_race(), console_flags_locked() would use lockdep to ensure the
console_list_lock is held.

But I would also like to have the _is_enabled special variant because
how we check if a console is enabled will be different for the atomic
consoles. I would like to hide those details in the _is_enabled
implementation.

John Ogness

Re: [PATCH printk v2 34/38] serial: kgdboc: use console_list_lock instead of console_lock

[Petr Mladek]

On Wed 2022-10-19 17:01:56, John Ogness wrote:
> kgdboc_earlycon_init() uses the console_lock to ensure that no consoles
> are unregistered until the kgdboc_earlycon is setup. The console_list_lock
> should be used instead because list synchronization repsponsibility will
> be removed from the console_lock in a later change.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk v2 12/38] tty: serial: kgdboc: use console_is_enabled()

[John Ogness]

On 2022-10-24, Doug Anderson <dianders@chromium.org> wrote:
> It actually only holds console_list_lock() even at the end of the
> series. Still, it seems weird that we're declaring the `data_race` on
> CON_ENABLED but not CON_BOOT ?

For my upcoming v3 I decided to drop this patch and will keep the
existing direct reading of @flags. Instead of this patch, for v3 the
comment will additionally mention why @flags is allowed to be directly
read:

/*
 * Hold the console_lock to guarantee that no consoles are
 * unregistered until the kgdboc_earlycon setup is complete.
 * Trapping the exit() callback relies on exit() not being
 * called until the trap is setup. This also allows safe
 * traversal of the console list and race-free reading of @flags.
 */

John Ogness

Re: [PATCH printk v2 12/38] tty: serial: kgdboc: use console_is_enabled()

[Petr Mladek]

On Fri 2022-11-04 17:29:15, John Ogness wrote:
> On 2022-10-24, Doug Anderson <dianders@chromium.org> wrote:
> > It actually only holds console_list_lock() even at the end of the
> > series. Still, it seems weird that we're declaring the `data_race` on
> > CON_ENABLED but not CON_BOOT ?
> 
> For my upcoming v3 I decided to drop this patch and will keep the
> existing direct reading of @flags. Instead of this patch, for v3 the
> comment will additionally mention why @flags is allowed to be directly
> read:
> 
> /*
>  * Hold the console_lock to guarantee that no consoles are
              ^^^^^^^^^^^^
>  * unregistered until the kgdboc_earlycon setup is complete.

My understanding is that this is synchronized by console_list_lock.
Or do I miss something?

>  * Trapping the exit() callback relies on exit() not being
>  * called until the trap is setup. This also allows safe
>  * traversal of the console list and race-free reading of @flags.
>  */

Best Regards,
Petr

Re: [PATCH printk v2 12/38] tty: serial: kgdboc: use console_is_enabled()

[John Ogness]

On 2022-11-07, Petr Mladek <pmladek@suse.com> wrote:
>> /*
>>  * Hold the console_lock to guarantee that no consoles are
>               ^^^^^^^^^^^^
>>  * unregistered until the kgdboc_earlycon setup is complete.
>
> My understanding is that this is synchronized by console_list_lock.
> Or do I miss something?

Yes, in the end the comment will say console_list_lock. At this point in
the series, the console_lock is still used. The comment is updated later
(as in patch 34 of the v2 series).

John