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/[email protected]
[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
Currently CON_ENABLED is being (mis)used to identify if the console
has been registered. This is not reliable because it can be set even
though registration failed or it can be unset, even though the console
is registered. Use console_is_registered() instead.
Signed-off-by: John Ogness <[email protected]>
---
arch/m68k/emu/nfcon.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/m68k/emu/nfcon.c b/arch/m68k/emu/nfcon.c
index 557d60867f98..6fdc13610565 100644
--- a/arch/m68k/emu/nfcon.c
+++ b/arch/m68k/emu/nfcon.c
@@ -49,7 +49,7 @@ static void nfcon_write(struct console *con, const char *str,
static struct tty_driver *nfcon_device(struct console *con, int *index)
{
*index = 0;
- return (con->flags & CON_ENABLED) ? nfcon_tty_driver : NULL;
+ return console_is_registered(con) ? nfcon_tty_driver : NULL;
}
static struct console nf_console = {
@@ -107,6 +107,11 @@ static int __init nf_debug_setup(char *arg)
stderr_id = nf_get_id("NF_STDERR");
if (stderr_id) {
+ /*
+ * The console will be enabled when debug=nfcon is specified
+ * as a kernel parameter. Since this is a non-standard way
+ * of enabling consoles, it must be explicitly enabled.
+ */
nf_console.flags |= CON_ENABLED;
register_console(&nf_console);
}
@@ -151,7 +156,7 @@ static int __init nfcon_init(void)
nfcon_tty_driver = driver;
- if (!(nf_console.flags & CON_ENABLED))
+ if (!console_is_registered(&nf_console))
register_console(&nf_console);
return 0;
--
2.30.2
It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
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 1183b2a26539..c38754d593ca 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_registered(&pic32_console))
register_console(&pic32_console);
return 0;
--
2.30.2
The CON_ENABLED flag is being misused to track whether or not the
extended console should be or has been registered. Instead use
a local variable to decide if the extended console should be
registered and console_is_registered() to determine if it has
been registered.
Also add a check in cleanup_netconsole() to only unregister the
extended console if it has been registered.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
drivers/net/netconsole.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index bdff9ac5056d..4f4f79532c6c 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -332,10 +332,8 @@ static ssize_t enabled_store(struct config_item *item,
}
if (enabled) { /* true */
- if (nt->extended && !(netconsole_ext.flags & CON_ENABLED)) {
- netconsole_ext.flags |= CON_ENABLED;
+ if (nt->extended && !console_is_registered(&netconsole_ext))
register_console(&netconsole_ext);
- }
/*
* Skip netpoll_parse_options() -- all the attributes are
@@ -869,7 +867,7 @@ static void write_msg(struct console *con, const char *msg, unsigned int len)
static struct console netconsole_ext = {
.name = "netcon_ext",
- .flags = CON_EXTENDED, /* starts disabled, registered on first use */
+ .flags = CON_ENABLED | CON_EXTENDED,
.write = write_ext_msg,
};
@@ -883,6 +881,7 @@ static int __init init_netconsole(void)
{
int err;
struct netconsole_target *nt, *tmp;
+ bool extended = false;
unsigned long flags;
char *target_config;
char *input = config;
@@ -895,11 +894,12 @@ static int __init init_netconsole(void)
goto fail;
}
/* Dump existing printks when we register */
- if (nt->extended)
- netconsole_ext.flags |= CON_PRINTBUFFER |
- CON_ENABLED;
- else
+ if (nt->extended) {
+ extended = true;
+ netconsole_ext.flags |= CON_PRINTBUFFER;
+ } else {
netconsole.flags |= CON_PRINTBUFFER;
+ }
spin_lock_irqsave(&target_list_lock, flags);
list_add(&nt->list, &target_list);
@@ -915,7 +915,7 @@ static int __init init_netconsole(void)
if (err)
goto undonotifier;
- if (netconsole_ext.flags & CON_ENABLED)
+ if (extended)
register_console(&netconsole_ext);
register_console(&netconsole);
pr_info("network logging started\n");
@@ -945,7 +945,8 @@ static void __exit cleanup_netconsole(void)
{
struct netconsole_target *nt, *tmp;
- unregister_console(&netconsole_ext);
+ if (console_is_registered(&netconsole_ext))
+ unregister_console(&netconsole_ext);
unregister_console(&netconsole);
dynamic_netconsole_exit();
unregister_netdevice_notifier(&netconsole_netdev_notifier);
--
2.30.2
It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.
Signed-off-by: John Ogness <[email protected]>
---
drivers/usb/early/xhci-dbc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/early/xhci-dbc.c b/drivers/usb/early/xhci-dbc.c
index bfb7e2b85299..797047154820 100644
--- a/drivers/usb/early/xhci-dbc.c
+++ b/drivers/usb/early/xhci-dbc.c
@@ -927,7 +927,7 @@ void __init early_xdbc_register_console(void)
static void xdbc_unregister_console(void)
{
- if (early_xdbc_console.flags & CON_ENABLED)
+ if (console_is_registered(&early_xdbc_console))
unregister_console(&early_xdbc_console);
}
--
2.30.2
After switching to SRCU for console list iteration, some readers
will begin readings console->flags as a data race. Locklessly
reading console->flags provides a consistent value because there
is at most one CPU modifying console->flags and that CPU is
using only read-modify-write operations.
Introduce a wrapper for SRCU iterators to read console flags.
Introduce a matching wrapper to write to flags of registered
consoles. Writing to flags of registered consoles is synchronized
by the console_list_lock.
Signed-off-by: John Ogness <[email protected]>
---
include/linux/console.h | 45 +++++++++++++++++++++++++++++++++++++++++
kernel/printk/printk.c | 10 ++++-----
2 files changed, 50 insertions(+), 5 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index 24d83e24840b..c1ca461d088a 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -183,6 +183,51 @@ extern void console_list_unlock(void) __releases(console_mutex);
extern struct hlist_head console_list;
+/**
+ * console_srcu_read_flags - Locklessly read the console flags
+ * @con: struct console pointer of console to read flags from
+ *
+ * This function provides the necessary READ_ONCE() and data_race()
+ * notation for locklessly reading the console flags. The READ_ONCE()
+ * in this function matches the WRITE_ONCE() when @flags are modified
+ * for registered consoles with console_srcu_write_flags().
+ *
+ * Only use this function to read console flags when locklessly
+ * iterating the console list via srcu.
+ *
+ * Context: Any context.
+ */
+static inline short console_srcu_read_flags(const struct console *con)
+{
+ WARN_ON_ONCE(!console_srcu_read_lock_is_held());
+
+ /*
+ * Locklessly reading console->flags provides a consistent
+ * read value because there is at most one CPU modifying
+ * console->flags and that CPU is using only read-modify-write
+ * operations to do so.
+ */
+ return data_race(READ_ONCE(con->flags));
+}
+
+/**
+ * console_srcu_write_flags - Write flags for a registered console
+ * @con: struct console pointer of console to write flags to
+ * @flags: new flags value to write
+ *
+ * Only use this function to write flags for registered consoles. It
+ * requires holding the console_list_lock.
+ *
+ * Context: Any context.
+ */
+static inline void console_srcu_write_flags(struct console *con, short flags)
+{
+ lockdep_assert_console_list_lock_held();
+
+ /* This matches the READ_ONCE() in console_srcu_read_flags(). */
+ WRITE_ONCE(con->flags, flags);
+}
+
/**
* for_each_console_srcu() - Iterator over registered consoles
* @con: struct console pointer used as loop cursor
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e3f81dda5b09..41c90b768c1c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3068,7 +3068,7 @@ void console_stop(struct console *console)
__pr_flush(console, 1000, true);
console_list_lock();
console_lock();
- console->flags &= ~CON_ENABLED;
+ console_srcu_write_flags(console, console->flags & ~CON_ENABLED);
console_unlock();
console_list_unlock();
@@ -3086,7 +3086,7 @@ void console_start(struct console *console)
{
console_list_lock();
console_lock();
- console->flags |= CON_ENABLED;
+ console_srcu_write_flags(console, console->flags | CON_ENABLED);
console_unlock();
console_list_unlock();
__pr_flush(console, 1000, true);
@@ -3314,7 +3314,7 @@ void register_console(struct console *newcon)
} else if (newcon->flags & CON_CONSDEV) {
/* Only the new head can have CON_CONSDEV set. */
- console_first()->flags &= ~CON_CONSDEV;
+ console_srcu_write_flags(console_first(), console_first()->flags & ~CON_CONSDEV);
hlist_add_head_rcu(&newcon->node, &console_list);
} else {
@@ -3371,7 +3371,7 @@ static int unregister_console_locked(struct console *console)
console_lock();
/* Disable it unconditionally */
- console->flags &= ~CON_ENABLED;
+ console_srcu_write_flags(console, console->flags & ~CON_ENABLED);
if (hlist_unhashed(&console->node)) {
console_unlock();
@@ -3390,7 +3390,7 @@ static int unregister_console_locked(struct console *console)
* console has any device attached. Oh well....
*/
if (!hlist_empty(&console_list) && console->flags & CON_CONSDEV)
- console_first()->flags |= CON_CONSDEV;
+ console_srcu_write_flags(console_first(), console_first()->flags | CON_CONSDEV);
console_unlock();
--
2.30.2
Guarantee safe iteration of the console list by using SRCU.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
kernel/debug/kdb/kdb_io.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 67d3c48a1522..5c7e9ba7cd6b 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -545,6 +545,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
{
struct console *c;
const char *cp;
+ int cookie;
int len;
if (msg_len == 0)
@@ -558,8 +559,20 @@ static void kdb_msg_write(const char *msg, int msg_len)
cp++;
}
- for_each_console(c) {
- if (!(c->flags & CON_ENABLED))
+ /*
+ * The console_srcu_read_lock() only provides safe console list
+ * traversal. The use of the ->write() callback relies on all other
+ * CPUs being stopped at the moment and console drivers being able to
+ * handle reentrance when @oops_in_progress is set.
+ *
+ * There is no guarantee that every console driver can handle
+ * reentrance in this way; the developer deploying the debugger
+ * is responsible for ensuring that the console drivers they
+ * have selected handle reentrance appropriately.
+ */
+ cookie = console_srcu_read_lock();
+ for_each_console_srcu(c) {
+ if (!(console_srcu_read_flags(c) & CON_ENABLED))
continue;
if (c == dbg_io_ops->cons)
continue;
@@ -577,6 +590,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
--oops_in_progress;
touch_nmi_watchdog();
}
+ console_srcu_read_unlock(cookie);
}
int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
--
2.30.2
It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
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..4f6e9bf57169 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_registered(&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_registered(&early_con))
return -EALREADY;
spin_lock_init(&port->lock);
--
2.30.2
It is not reliable to check for CON_ENABLED in order to identify if a
console is registered. Use console_is_registered() instead.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
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..9c252c9ca95a 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_registered(&s3c24xx_serial_console))
unregister_console(&s3c24xx_serial_console);
}
--
2.30.2
Use srcu console list iteration for console list traversal. It is
acceptable because the consoles might come and go at any time.
Strict synchronizing with console registration code would not bring
any advantage over srcu.
Document why the console_lock is still necessary. Note that this
is a preparatory change for when console_lock no longer provides
synchronization for the console list.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
kernel/printk/printk.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index f536233d8234..fb609a19f797 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3076,15 +3076,25 @@ struct tty_driver *console_device(int *index)
{
struct console *c;
struct tty_driver *driver = NULL;
+ int cookie;
+ /*
+ * Take console_lock to serialize device() callback with
+ * other console operations. For example, fg_console is
+ * modified under console_lock when switching vt.
+ */
console_lock();
- for_each_console(c) {
+
+ cookie = console_srcu_read_lock();
+ for_each_console_srcu(c) {
if (!c->device)
continue;
driver = c->device(c, index);
if (driver)
break;
}
+ console_srcu_read_unlock(cookie);
+
console_unlock();
return driver;
}
--
2.30.2
With SRCU it is now safe to traverse the console list, even if
the console_trylock() failed. However, overwriting console->seq
when console_trylock() failed is still an issue.
Switch to SRCU iteration and document remaining issue with
console->seq.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
kernel/printk/printk.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 7304cbcd6dd4..f536233d8234 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3049,21 +3049,22 @@ void console_flush_on_panic(enum con_flush_mode mode)
console_may_schedule = 0;
if (mode == CONSOLE_REPLAY_ALL) {
- struct hlist_node *tmp;
struct console *c;
+ int cookie;
u64 seq;
seq = prb_first_valid_seq(prb);
- /*
- * This cannot use for_each_console() because it's not established
- * that the current context has console locked and neither there is
- * a guarantee that there is no concurrency in that case.
- *
- * Open code it for documentation purposes and pretend that
- * it works.
- */
- hlist_for_each_entry_safe(c, tmp, &console_list, node)
+
+ cookie = console_srcu_read_lock();
+ for_each_console_srcu(c) {
+ /*
+ * If the above console_trylock() failed, this is an
+ * unsynchronized assignment. But in that case, the
+ * kernel is in "hope and pray" mode anyway.
+ */
c->seq = seq;
+ }
+ console_srcu_read_unlock(cookie);
}
console_unlock();
}
--
2.30.2
From: Thomas Gleixner <[email protected]>
Unprotected list walks are not necessarily safe.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Greg Kroah-Hartman <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
---
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
show_cons_active() uses the console_lock to gather information
on registered consoles. It requires that no consoles are unregistered
until it is finished. The console_list_lock should be used because
list synchronization responsibility will be removed from the
console_lock in a later change.
Note, the console_lock is still needed to serialize the device()
callback with other console operations.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Greg Kroah-Hartman <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
drivers/tty/tty_io.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index ee4da2fec328..cafdff575716 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3527,16 +3527,13 @@ static ssize_t show_cons_active(struct device *dev,
ssize_t count = 0;
/*
- * Hold the console_lock to guarantee that no consoles are
+ * Hold the console_list_lock to guarantee that no consoles are
* unregistered until all console processing is complete.
* This also allows safe traversal of the console list and
* race-free reading of @flags.
- *
- * Take console_lock to serialize device() callback with
- * other console operations. For example, fg_console is
- * modified under console_lock when switching vt.
*/
- console_lock();
+ console_list_lock();
+
for_each_console(c) {
if (!c->device)
continue;
@@ -3548,6 +3545,13 @@ static ssize_t show_cons_active(struct device *dev,
if (i >= ARRAY_SIZE(cs))
break;
}
+
+ /*
+ * Take console_lock to serialize device() callback with
+ * other console operations. For example, fg_console is
+ * modified under console_lock when switching vt.
+ */
+ console_lock();
while (i--) {
int index = cs[i]->index;
struct tty_driver *drv = cs[i]->device(cs[i], &index);
@@ -3563,6 +3567,8 @@ static ssize_t show_cons_active(struct device *dev,
}
console_unlock();
+ console_list_unlock();
+
return count;
}
static DEVICE_ATTR(active, S_IRUGO, show_cons_active, NULL);
--
2.30.2
On Mon 2022-11-14 17:35 +0106, John Ogness wrote:
> Guarantee safe iteration of the console list by using SRCU.
>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Petr Mladek <[email protected]>
> ---
> kernel/debug/kdb/kdb_io.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 67d3c48a1522..5c7e9ba7cd6b 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -545,6 +545,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
> {
> struct console *c;
> const char *cp;
> + int cookie;
> int len;
>
> if (msg_len == 0)
> @@ -558,8 +559,20 @@ static void kdb_msg_write(const char *msg, int msg_len)
> cp++;
> }
>
> - for_each_console(c) {
> - if (!(c->flags & CON_ENABLED))
> + /*
> + * The console_srcu_read_lock() only provides safe console list
> + * traversal. The use of the ->write() callback relies on all other
> + * CPUs being stopped at the moment and console drivers being able to
> + * handle reentrance when @oops_in_progress is set.
> + *
> + * There is no guarantee that every console driver can handle
> + * reentrance in this way; the developer deploying the debugger
> + * is responsible for ensuring that the console drivers they
> + * have selected handle reentrance appropriately.
> + */
> + cookie = console_srcu_read_lock();
> + for_each_console_srcu(c) {
> + if (!(console_srcu_read_flags(c) & CON_ENABLED))
> continue;
> if (c == dbg_io_ops->cons)
> continue;
> @@ -577,6 +590,7 @@ static void kdb_msg_write(const char *msg, int msg_len)
> --oops_in_progress;
> touch_nmi_watchdog();
> }
> + console_srcu_read_unlock(cookie);
> }
>
> int vkdb_printf(enum kdb_msgsrc src, const char *fmt, va_list ap)
> --
> 2.30.2
>
Reviewed-by: Aaron Tomlin <[email protected]>
--
Aaron Tomlin
On Mon 2022-11-14 17:35:15, John Ogness wrote:
> Currently CON_ENABLED is being (mis)used to identify if the console
> has been registered. This is not reliable because it can be set even
> though registration failed or it can be unset, even though the console
> is registered. Use console_is_registered() instead.
>
> Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Mon 2022-11-14 17:35:04, John Ogness wrote:
> After switching to SRCU for console list iteration, some readers
> will begin readings console->flags as a data race. Locklessly
> reading console->flags provides a consistent value because there
> is at most one CPU modifying console->flags and that CPU is
> using only read-modify-write operations.
>
> Introduce a wrapper for SRCU iterators to read console flags.
> Introduce a matching wrapper to write to flags of registered
> consoles. Writing to flags of registered consoles is synchronized
> by the console_list_lock.
>
> Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr