[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


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH printk v2 07/38] um: kmsg_dump: 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>
---
 arch/um/kernel/kmsg_dump.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
index 0224fcb36e22..3a3bbbb22090 100644
--- a/arch/um/kernel/kmsg_dump.c
+++ b/arch/um/kernel/kmsg_dump.c
@@ -22,8 +22,8 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
 		return;
 
 	for_each_console(con) {
-		if(strcmp(con->name, "tty") == 0 &&
-		   (con->flags & (CON_ENABLED | CON_CONSDEV)) != 0) {
+		if (strcmp(con->name, "tty") == 0 &&
+		    (console_is_enabled(con) || (con->flags & CON_CONSDEV))) {
 			break;
 		}
 	}
-- 
2.30.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH printk v2 20/38] um: kmsg_dumper: use srcu console list iterator

[John Ogness]

Rather than using the console_lock to guarantee safe console list
traversal, use srcu console list iteration.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 arch/um/kernel/kmsg_dump.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
index 3a3bbbb22090..44a418dec919 100644
--- a/arch/um/kernel/kmsg_dump.c
+++ b/arch/um/kernel/kmsg_dump.c
@@ -16,20 +16,17 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
 	struct console *con;
 	unsigned long flags;
 	size_t len = 0;
+	int cookie;
 
 	/* only dump kmsg when no console is available */
-	if (!console_trylock())
-		return;
-
-	for_each_console(con) {
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(con) {
 		if (strcmp(con->name, "tty") == 0 &&
 		    (console_is_enabled(con) || (con->flags & CON_CONSDEV))) {
 			break;
 		}
 	}
-
-	console_unlock();
-
+	console_srcu_read_unlock(cookie);
 	if (con)
 		return;
 
-- 
2.30.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH printk v2 07/38] um: kmsg_dump: use console_is_enabled()

[Petr Mladek]

On Wed 2022-10-19 17:01:29, 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

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH printk v2 20/38] um: kmsg_dumper: use srcu console list iterator

[Petr Mladek]

On Wed 2022-10-19 17:01:42, John Ogness wrote:
> Rather than using the console_lock to guarantee safe console list
> traversal, use srcu console list iteration.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  arch/um/kernel/kmsg_dump.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
> index 3a3bbbb22090..44a418dec919 100644
> --- a/arch/um/kernel/kmsg_dump.c
> +++ b/arch/um/kernel/kmsg_dump.c
> @@ -16,20 +16,17 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
>  	struct console *con;
>  	unsigned long flags;
>  	size_t len = 0;
> +	int cookie;
>  
>  	/* only dump kmsg when no console is available */

I agree that it is perfectly fine to use RCU here. The previous
code was just a best effort. The kdump was done without console_lock()
so that the console might get available in the meantime.

I guess that it is not a big deal. The dumper is called typically only
during panic() where new consoles are not added.

> -	if (!console_trylock())
> -		return;
> -
> -	for_each_console(con) {
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(con) {
>  		if (strcmp(con->name, "tty") == 0 &&
>  		    (console_is_enabled(con) || (con->flags & CON_CONSDEV))) {

This is slightly more racy than the previous code. Only one console
could have CON_CONSDEV. It might be moved to another console when the
list is manipulated. As a result, rcu walk might see zero, one, or two
consoles with this flag unless the flag is moved carefully.

Anyway, this check does not match the comment and does not make much
sense to me. It is true that CON_CONSDEV flag is used only for
registered consoles. But messages are printed on the console only
when CON_ENABLED flag is set.

IMHO, we should check only console_is_enabled() here.

Adding Thomas Meyer and Richard Weinberger <richard@nod.at> into Cc.
Thomas added this check in the commit e23fe90dec286cd77e90
("um: kmsg_dumper: always dump when not tty console") and
Richard pushed it.

>  			break;
>  		}
>  	}
> -
> -	console_unlock();
> -
> +	console_srcu_read_unlock(cookie);
>
>  	if (con)
>  		return;

Best Regards,
Petr

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um