[PATCH printk v4 00/39] reduce console_lock scope

[PATCH printk v4 00/39] reduce console_lock scope

[John Ogness]

This is v4 of a series to prepare for threaded/atomic
printing. v3 is here [0]. 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.

Also, during the review of v2 it came to our attention that
many console drivers are checking CON_ENABLED to see if they
are registered. Because this flag can change without
unregistering and because this flag does not represent an
atomic point when an (un)registration process is complete,
a new console_is_registered() function is introduced. This
function uses the console_list_lock to synchronize with the
(un)registration process to provide a reliable status.

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

All users of CON_ENABLED for registration status have been
modified to use console_is_registered(). Note that there are
still users of CON_ENABLED, but this is for legitimate purposes
about a registered console being able to print.

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 v3:

general:

- Implement console_srcu_read_flags() and console_srcu_write_flags()
  to be used for console->flags access under the srcu_read_lock or
  console_list_lock, respectively. The functions document their
  relationship to one another and use data_race(), READ_ONCE(), and
  WRITE_ONCE() macros to annotate their relationship. They also make
  use of lockdep to warn if used in improper contexts.

- Replace all console_is_enabled() usage with
  console_srcu_read_flags() (all were under the srcu_read_lock).

serial_core:

- For uart_console_registered(), check uart_console() before taking
  the console_list_lock to avoid unnecessary lock contention for
  non-console ports.

m68k/emu/nfcon:

- Only explicitly enable the console if registering via debug=nfcon.

tty/serial/sh-sci:

- Add comments about why @options will always be NULL for the
  earlyprintk console.

kdb:

- Add comments explaining the expectations for console drivers to
  work correctly.

printk:

- Some code sites under SRCU were checking flags directly. Use
  console_srcu_read_flags() instead.

- In register_console() rename bootcon_enabled/realcon_enabled to
  bootcon_registered/realcon_registered to avoid confusion.

- In register_console() only check for boot console sequences if a
  boot console is registered and !keep_bootcon. In this case, also
  take the console_lock to guarantee safe access to console->seq.

- In console_force_preferred_locked() use hlist_del_rcu() instead of
  hlist_del_init_rcu() so that there is never a window where the
  console can be viewed as unregistered.

John Ogness

[0] https://lore.kernel.org/lkml/20221107141638.3790965-1-john.ogness@linutronix.de
[1] https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/log/?h=srcunmisafe.2022.11.09a

John Ogness (37):
  printk: Prepare for SRCU console list protection
  printk: register_console: use "registered" for variable names
  printk: fix setting first seq for consoles
  um: kmsg_dump: only dump when no output console available
  tty: serial: kgdboc: document console_lock usage
  tty: tty_io: document console_lock usage
  proc: consoles: document console_lock usage
  printk: introduce console_list_lock
  console: introduce wrappers to read/write console flags
  um: kmsg_dumper: use srcu console list iterator
  kdb: use srcu console list iterator
  printk: console_flush_all: use srcu console list iterator
  printk: __pr_flush: use srcu console list iterator
  printk: console_is_usable: use console_srcu_read_flags
  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
  console: introduce console_is_registered()
  serial_core: replace uart_console_enabled() with
    uart_console_registered()
  tty: nfcon: use console_is_registered()
  efi: earlycon: use console_is_registered()
  tty: hvc: use console_is_registered()
  tty: serial: earlycon: use console_is_registered()
  tty: serial: pic32_uart: use console_is_registered()
  tty: serial: samsung_tty: use console_is_registered()
  tty: serial: xilinx_uartps: use console_is_registered()
  usb: early: xhci-dbc: use console_is_registered()
  netconsole: avoid CON_ENABLED misuse to track registration
  printk, xen: fbfront: create/use safe function for forcing preferred
  tty: tty_io: use console_list_lock for list synchronization
  proc: consoles: use console_list_lock for list iteration
  tty: serial: kgdboc: use srcu console list iterator
  tty: serial: kgdboc: use console_list_lock for list traversal
  tty: serial: kgdboc: synchronize tty_find_polling_driver() and
    register_console()
  tty: serial: kgdboc: use console_list_lock to trap exit
  printk: relieve console_lock of list synchronization duties
  tty: serial: sh-sci: use setup() callback for early console

Thomas Gleixner (2):
  serial: kgdboc: Lock console list in probe function
  printk: Convert console_drivers list to hlist

 .clang-format                       |   1 +
 arch/m68k/emu/nfcon.c               |   9 +-
 arch/um/kernel/kmsg_dump.c          |  24 +-
 drivers/firmware/efi/earlycon.c     |   8 +-
 drivers/net/netconsole.c            |  21 +-
 drivers/tty/hvc/hvc_console.c       |   4 +-
 drivers/tty/serial/8250/8250_core.c |   2 +-
 drivers/tty/serial/earlycon.c       |   4 +-
 drivers/tty/serial/kgdboc.c         |  46 ++-
 drivers/tty/serial/pic32_uart.c     |   4 +-
 drivers/tty/serial/samsung_tty.c    |   2 +-
 drivers/tty/serial/serial_core.c    |  14 +-
 drivers/tty/serial/sh-sci.c         |  20 +-
 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   |  12 +-
 fs/proc/consoles.c                  |  21 +-
 include/linux/console.h             | 129 +++++++-
 include/linux/serial_core.h         |  10 +-
 kernel/debug/kdb/kdb_io.c           |  18 +-
 kernel/printk/printk.c              | 459 +++++++++++++++++++++-------
 22 files changed, 648 insertions(+), 182 deletions(-)


base-commit: f733615e39aa2d6ddeef33b7b2c9aa6a5a2c2785
-- 
2.30.2

[PATCH printk v4 31/39] printk, xen: fbfront: create/use safe function for forcing preferred

[John Ogness]

With commit 9e124fe16ff2("xen: Enable console tty by default in domU
if it's not a dummy") a hack was implemented to make sure that the
tty console remains the console behind the /dev/console device. The
main problem with the hack is that, after getting the console pointer
to the tty console, it is assumed the pointer is still valid after
releasing the console_sem. This assumption is incorrect and unsafe.

Make the hack safe by introducing a new function
console_force_preferred_locked() and perform the full operation
under the console_list_lock.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/video/fbdev/xen-fbfront.c | 12 +++-----
 include/linux/console.h           |  1 +
 kernel/printk/printk.c            | 49 +++++++++++++++++++++++++++++--
 3 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/drivers/video/fbdev/xen-fbfront.c b/drivers/video/fbdev/xen-fbfront.c
index 4d2694d904aa..8752d389e382 100644
--- a/drivers/video/fbdev/xen-fbfront.c
+++ b/drivers/video/fbdev/xen-fbfront.c
@@ -504,18 +504,14 @@ static void xenfb_make_preferred_console(void)
 	if (console_set_on_cmdline)
 		return;
 
-	console_lock();
+	console_list_lock();
 	for_each_console(c) {
 		if (!strcmp(c->name, "tty") && c->index == 0)
 			break;
 	}
-	console_unlock();
-	if (c) {
-		unregister_console(c);
-		c->flags |= CON_CONSDEV;
-		c->flags &= ~CON_PRINTBUFFER; /* don't print again */
-		register_console(c);
-	}
+	if (c)
+		console_force_preferred_locked(c);
+	console_list_unlock();
 }
 
 static int xenfb_resume(struct xenbus_device *dev)
diff --git a/include/linux/console.h b/include/linux/console.h
index f716e1dd9eaf..9cea254b34b8 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -291,6 +291,7 @@ enum con_flush_mode {
 };
 
 extern int add_preferred_console(char *name, int idx, char *options);
+extern void console_force_preferred_locked(struct console *con);
 extern void register_console(struct console *);
 extern int unregister_console(struct console *);
 extern void console_lock(void);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e770b1ede6c9..dff76c1cef80 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -247,9 +247,10 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
 void console_list_lock(void)
 {
 	/*
-	 * In unregister_console(), synchronize_srcu() is called with the
-	 * console_list_lock held. Therefore it is not allowed that the
-	 * console_list_lock is taken with the srcu_lock held.
+	 * In unregister_console() and console_force_preferred_locked(),
+	 * synchronize_srcu() is called with the console_list_lock held.
+	 * Therefore it is not allowed that the console_list_lock is taken
+	 * with the srcu_lock held.
 	 *
 	 * Detecting if this context is really in the read-side critical
 	 * section is only possible if the appropriate debug options are
@@ -3461,6 +3462,48 @@ int unregister_console(struct console *console)
 }
 EXPORT_SYMBOL(unregister_console);
 
+/**
+ * console_force_preferred_locked - force a registered console preferred
+ * @con: The registered console to force preferred.
+ *
+ * Must be called under console_list_lock().
+ */
+void console_force_preferred_locked(struct console *con)
+{
+	struct console *cur_pref_con;
+
+	if (!console_is_registered_locked(con))
+		return;
+
+	cur_pref_con = console_first();
+
+	/* Already preferred? */
+	if (cur_pref_con == con)
+		return;
+
+	/*
+	 * Delete, but do not re-initialize the entry. This allows the console
+	 * to continue to appear registered (via any hlist_unhashed_lockless()
+	 * checks), even though it was briefly removed from the console list.
+	 */
+	hlist_del_rcu(&con->node);
+
+	/*
+	 * Ensure that all SRCU list walks have completed so that the console
+	 * can be added to the beginning of the console list and its forward
+	 * list pointer can be re-initialized.
+	 */
+	synchronize_srcu(&console_srcu);
+
+	con->flags |= CON_CONSDEV;
+	WARN_ON(!con->device);
+
+	/* Only the new head can have CON_CONSDEV set. */
+	console_srcu_write_flags(cur_pref_con, cur_pref_con->flags & ~CON_CONSDEV);
+	hlist_add_behind_rcu(&con->node, console_list.first);
+}
+EXPORT_SYMBOL(console_force_preferred_locked);
+
 /*
  * Initialize the console device. This is called *early*, so
  * we can't necessarily depend on lots of kernel help here.
-- 
2.30.2

Re: [PATCH printk v4 31/39] printk, xen: fbfront: create/use safe function for forcing preferred

[John Ogness]

Hi,

After more detailed runtime testing I discovered that I didn't re-insert
the console to the correct place in the list. More below...

On 2022-11-14, John Ogness <john.ogness@linutronix.de> wrote:
> diff --git a/include/linux/console.h b/include/linux/console.h
> index f716e1dd9eaf..9cea254b34b8 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -291,6 +291,7 @@ enum con_flush_mode {
>  };
>  
>  extern int add_preferred_console(char *name, int idx, char *options);
> +extern void console_force_preferred_locked(struct console *con);
>  extern void register_console(struct console *);
>  extern int unregister_console(struct console *);
>  extern void console_lock(void);
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index e770b1ede6c9..dff76c1cef80 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3461,6 +3462,48 @@ int unregister_console(struct console *console)
>  }
>  EXPORT_SYMBOL(unregister_console);
>  
> +/**
> + * console_force_preferred_locked - force a registered console preferred
> + * @con: The registered console to force preferred.
> + *
> + * Must be called under console_list_lock().
> + */
> +void console_force_preferred_locked(struct console *con)
> +{
> +	struct console *cur_pref_con;
> +
> +	if (!console_is_registered_locked(con))
> +		return;
> +
> +	cur_pref_con = console_first();
> +
> +	/* Already preferred? */
> +	if (cur_pref_con == con)
> +		return;
> +
> +	/*
> +	 * Delete, but do not re-initialize the entry. This allows the console
> +	 * to continue to appear registered (via any hlist_unhashed_lockless()
> +	 * checks), even though it was briefly removed from the console list.
> +	 */
> +	hlist_del_rcu(&con->node);
> +
> +	/*
> +	 * Ensure that all SRCU list walks have completed so that the console
> +	 * can be added to the beginning of the console list and its forward
> +	 * list pointer can be re-initialized.
> +	 */
> +	synchronize_srcu(&console_srcu);
> +
> +	con->flags |= CON_CONSDEV;
> +	WARN_ON(!con->device);
> +
> +	/* Only the new head can have CON_CONSDEV set. */
> +	console_srcu_write_flags(cur_pref_con, cur_pref_con->flags & ~CON_CONSDEV);
> +	hlist_add_behind_rcu(&con->node, console_list.first);

This is adding the console as the 2nd item. It should be the new
head. The patch below fixes it.

I have done careful runtime testing with this fixup. After the
force_preferred, the console is the new head and sending data to
/dev/console redirects to that console.

It would be nice if we could fold this in. Sorry.

John Ogness

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8d635467882f..4b77586cf4cb 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3494,7 +3494,7 @@ void console_force_preferred_locked(struct console *con)
 
 	/* Only the new head can have CON_CONSDEV set. */
 	console_srcu_write_flags(cur_pref_con, cur_pref_con->flags & ~CON_CONSDEV);
-	hlist_add_behind_rcu(&con->node, console_list.first);
+	hlist_add_head_rcu(&con->node, &console_list);
 }
 EXPORT_SYMBOL(console_force_preferred_locked);
 

Re: [PATCH printk v4 31/39] printk, xen: fbfront: create/use safe function for forcing preferred

[Petr Mladek]

On Mon 2022-11-14 20:57:18, John Ogness wrote:
> Hi,
> 
> After more detailed runtime testing I discovered that I didn't re-insert
> the console to the correct place in the list. More below...
> 
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3461,6 +3462,48 @@ int unregister_console(struct console *console)
> >  }
> >  EXPORT_SYMBOL(unregister_console);
> >  
> > +/**
> > + * console_force_preferred_locked - force a registered console preferred
> > + * @con: The registered console to force preferred.
> > + *
> > + * Must be called under console_list_lock().
> > + */
> > +void console_force_preferred_locked(struct console *con)
> > +{
> > +	struct console *cur_pref_con;
> > +
> > +	if (!console_is_registered_locked(con))
> > +		return;
> > +
> > +	cur_pref_con = console_first();
> > +
> > +	/* Already preferred? */
> > +	if (cur_pref_con == con)
> > +		return;
> > +
> > +	/*
> > +	 * Delete, but do not re-initialize the entry. This allows the console
> > +	 * to continue to appear registered (via any hlist_unhashed_lockless()
> > +	 * checks), even though it was briefly removed from the console list.
> > +	 */
> > +	hlist_del_rcu(&con->node);
> > +
> > +	/*
> > +	 * Ensure that all SRCU list walks have completed so that the console
> > +	 * can be added to the beginning of the console list and its forward
> > +	 * list pointer can be re-initialized.
> > +	 */
> > +	synchronize_srcu(&console_srcu);
> > +
> > +	con->flags |= CON_CONSDEV;
> > +	WARN_ON(!con->device);
> > +
> > +	/* Only the new head can have CON_CONSDEV set. */
> > +	console_srcu_write_flags(cur_pref_con, cur_pref_con->flags & ~CON_CONSDEV);
> > +	hlist_add_behind_rcu(&con->node, console_list.first);
> 
> This is adding the console as the 2nd item. It should be the new
> head. The patch below fixes it.
> 
> I have done careful runtime testing with this fixup. After the
> force_preferred, the console is the new head and sending data to
> /dev/console redirects to that console.

Great catch!

> It would be nice if we could fold this in. Sorry.

I have missed it as well :-/

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 8d635467882f..4b77586cf4cb 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3494,7 +3494,7 @@ void console_force_preferred_locked(struct console *con)
>  
>  	/* Only the new head can have CON_CONSDEV set. */
>  	console_srcu_write_flags(cur_pref_con, cur_pref_con->flags & ~CON_CONSDEV);
> -	hlist_add_behind_rcu(&con->node, console_list.first);
> +	hlist_add_head_rcu(&con->node, &console_list);
>  }
>  EXPORT_SYMBOL(console_force_preferred_locked);

With this change:

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

Best Regards,
Petr