[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 09/38] netconsole: 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/net/netconsole.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index bdff9ac5056d..073e59a06f21 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -332,7 +332,7 @@ static ssize_t enabled_store(struct config_item *item,
 	}
 
 	if (enabled) {	/* true */
-		if (nt->extended && !(netconsole_ext.flags & CON_ENABLED)) {
+		if (nt->extended && !console_is_enabled(&netconsole_ext)) {
 			netconsole_ext.flags |= CON_ENABLED;
 			register_console(&netconsole_ext);
 		}
@@ -915,7 +915,7 @@ static int __init init_netconsole(void)
 	if (err)
 		goto undonotifier;
 
-	if (netconsole_ext.flags & CON_ENABLED)
+	if (console_is_enabled(&netconsole_ext))
 		register_console(&netconsole_ext);
 	register_console(&netconsole);
 	pr_info("network logging started\n");
-- 
2.30.2

Re: [PATCH printk v2 09/38] netconsole: use console_is_enabled()

[Petr Mladek]

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

The change is straightforward:

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

The comment below is just a lamentation about the netconsole code.

> ---
>  drivers/net/netconsole.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index bdff9ac5056d..073e59a06f21 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -332,7 +332,7 @@ static ssize_t enabled_store(struct config_item *item,
>  	}
>  
>  	if (enabled) {	/* true */
> -		if (nt->extended && !(netconsole_ext.flags & CON_ENABLED)) {
> +		if (nt->extended && !console_is_enabled(&netconsole_ext)) {
>  			netconsole_ext.flags |= CON_ENABLED;
>  			register_console(&netconsole_ext);
>  		}
> @@ -915,7 +915,7 @@ static int __init init_netconsole(void)
>  	if (err)
>  		goto undonotifier;
>  
> -	if (netconsole_ext.flags & CON_ENABLED)
> +	if (console_is_enabled(&netconsole_ext))
>  		register_console(&netconsole_ext);
>  	register_console(&netconsole);
>  	pr_info("network logging started\n");

Just for record:

This looks like a (mis)use of CON_ENABLED flag. It took me some time
to understand why pre-enabled consoles are handled special way in
register_console(). I partly documented it in
try_enable_preferred_console():

	/*
	 * Some consoles, such as pstore and netconsole, can be enabled even
	 * without matching. Accept the pre-enabled consoles only when match()
	 * and setup() had a chance to be called.
	 */
	if (console_is_enabled(newcon) && (c->user_specified == user_specified))
		return 0;

In my bottom driver, I have a patch cleaning this. It is part of a bigger
clean up that is not ready for upstream :-/

Best Regards,
Petr

Re: [PATCH printk v2 09/38] netconsole: use console_is_enabled()

[John Ogness]

On 2022-10-21, Petr Mladek <pmladek@suse.com> wrote:
>> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
>> index bdff9ac5056d..073e59a06f21 100644
>> --- a/drivers/net/netconsole.c
>> +++ b/drivers/net/netconsole.c
>> @@ -332,7 +332,7 @@ static ssize_t enabled_store(struct config_item *item,
>>  	}
>>  
>>  	if (enabled) {	/* true */
>> -		if (nt->extended && !(netconsole_ext.flags & CON_ENABLED)) {
>> +		if (nt->extended && !console_is_enabled(&netconsole_ext)) {
>>  			netconsole_ext.flags |= CON_ENABLED;
>>  			register_console(&netconsole_ext);
>>  		}
>> @@ -915,7 +915,7 @@ static int __init init_netconsole(void)
>>  	if (err)
>>  		goto undonotifier;
>>  
>> -	if (netconsole_ext.flags & CON_ENABLED)
>> +	if (console_is_enabled(&netconsole_ext))
>>  		register_console(&netconsole_ext);
>>  	register_console(&netconsole);
>>  	pr_info("network logging started\n");
>
> This looks like a (mis)use of CON_ENABLED flag.

Yes. When @netconsole_ext is registered, CON_ENABLED is always set. So
it should be set in the static initialization. The first hunk should be
using the new console_is_registered(). The second hunk should be using a
local @extended bool variable. Also, in cleanup_netconsole() it should
check if the console is registered:

if (console_is_registered(&netconsole_ext))
        unregister_console(&netconsole_ext);

I will make all of these changes for v3. Then there will be no
checking/setting of CON_ENABLED in the driver.

John Ogness