This is v5 of a series to prepare for threaded/atomic
printing. v4 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 is 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 v4:
printk:
- Introduce console_init_seq() to handle the now rather complex
procedure to find an appropriate start sequence number for a
new console upon registration.
- When registering a non-boot console and boot consoles are
registered, try to flush all the consoles to get the next @seq
value before falling back to use the @seq of the enabled boot
console that is furthest behind.
- For console_force_preferred_locked(), make the console the
head of the console list.
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 (38):
printk: Prepare for SRCU console list protection
printk: register_console: use "registered" for variable names
printk: move @seq initialization to helper
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 | 493 +++++++++++++++++++++-------
22 files changed, 680 insertions(+), 184 deletions(-)
base-commit: f733615e39aa2d6ddeef33b7b2c9aa6a5a2c2785
--
2.30.2
All users of console_is_usable() are SRCU iterators. Use the
appropriate wrapper function to locklessly read the flags.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
kernel/printk/printk.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 6666cc27a014..75951c4bda05 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2708,11 +2708,13 @@ static bool abandon_console_lock_in_panic(void)
* Check if the given console is currently capable and allowed to print
* records.
*
- * Requires the console_lock.
+ * Requires the console_srcu_read_lock.
*/
static inline bool console_is_usable(struct console *con)
{
- if (!(con->flags & CON_ENABLED))
+ short flags = console_srcu_read_flags(con);
+
+ if (!(flags & CON_ENABLED))
return false;
if (!con->write)
@@ -2723,8 +2725,7 @@ static inline bool console_is_usable(struct console *con)
* allocated. So unless they're explicitly marked as being able to
* cope (CON_ANYTIME) don't call them until this CPU is officially up.
*/
- if (!cpu_online(raw_smp_processor_id()) &&
- !(con->flags & CON_ANYTIME))
+ if (!cpu_online(raw_smp_processor_id()) && !(flags & CON_ANYTIME))
return false;
return true;
--
2.30.2
Use srcu console list iteration for safe console list traversal.
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]>
---
drivers/tty/serial/kgdboc.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index 5be381003e58..c6df9ef34099 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -451,6 +451,7 @@ static void kgdboc_earlycon_pre_exp_handler(void)
{
struct console *con;
static bool already_warned;
+ int cookie;
if (already_warned)
return;
@@ -463,9 +464,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
Currently there exist races in register_console(), where the types
of registered consoles are checked (without holding the console_lock)
and then after acquiring the console_lock, it is assumed that the list
has not changed. Also, some code that performs console_unregister()
make similar assumptions.
It might be possible to fix these races using the console_lock. But
it would require a complex analysis of all console drivers to make
sure that the console_lock is not taken in match() and setup()
callbacks. And we really prefer to split up and reduce the
responsibilities of console_lock rather than expand its complexity.
Therefore, introduce a new console_list_lock to provide full
synchronization for any console list changes.
In addition, also use console_list_lock for synchronization of
console->flags updates. All flags are either static or modified only
during the console registration. There are only two exceptions.
The first exception is CON_ENABLED, which is also modified by
console_start()/console_stop(). Therefore, these functions must
also take the console_list_lock.
The second exception is when the flags are modified by the console
driver init code before the console is registered. These will be
ignored because they are not visible to the rest of the system
via the console_drivers list.
Note that one of the various responsibilities of the console_lock is
also intended to provide console list and console->flags
synchronization. Later changes will update call sites relying on the
console_lock for these purposes. Once all call sites have been
updated, the console_lock will be relieved of synchronizing
console_list and console->flags updates.
Suggested-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
include/linux/console.h | 23 +++++++++--
kernel/printk/printk.c | 88 ++++++++++++++++++++++++++++++++++++-----
2 files changed, 99 insertions(+), 12 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index f4f0c9523835..24d83e24840b 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -158,6 +158,14 @@ struct console {
struct hlist_node node;
};
+#ifdef CONFIG_LOCKDEP
+extern void lockdep_assert_console_list_lock_held(void);
+#else
+static inline void lockdep_assert_console_list_lock_held(void)
+{
+}
+#endif
+
#ifdef CONFIG_DEBUG_LOCK_ALLOC
extern bool console_srcu_read_lock_is_held(void);
#else
@@ -170,6 +178,9 @@ static inline bool console_srcu_read_lock_is_held(void)
extern int console_srcu_read_lock(void);
extern void console_srcu_read_unlock(int cookie);
+extern void console_list_lock(void) __acquires(console_mutex);
+extern void console_list_unlock(void) __releases(console_mutex);
+
extern struct hlist_head console_list;
/**
@@ -186,10 +197,16 @@ extern struct hlist_head console_list;
hlist_for_each_entry_srcu(con, &console_list, node, \
console_srcu_read_lock_is_held())
-/*
- * for_each_console() allows you to iterate on each console
+/**
+ * for_each_console() - Iterator over registered consoles
+ * @con: struct console pointer used as loop cursor
+ *
+ * The console list and the console->flags are immutable while iterating.
+ *
+ * Requires console_list_lock to be held.
*/
-#define for_each_console(con) \
+#define for_each_console(con) \
+ lockdep_assert_console_list_lock_held(); \
hlist_for_each_entry(con, &console_list, node)
extern int console_set_on_cmdline;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c84654444a02..f7479fd73114 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -78,6 +78,13 @@ EXPORT_SYMBOL(ignore_console_lock_warning);
int oops_in_progress;
EXPORT_SYMBOL(oops_in_progress);
+/*
+ * console_mutex protects console_list updates and console->flags updates.
+ * The flags are synchronized only for consoles that are registered, i.e.
+ * accessible via the console list.
+ */
+static DEFINE_MUTEX(console_mutex);
+
/*
* console_sem protects console_list and console->flags updates, and also
* provides serialization for access to the entire console driver system.
@@ -103,6 +110,11 @@ static int __read_mostly suppress_panic_printk;
static struct lockdep_map console_lock_dep_map = {
.name = "console_lock"
};
+
+void lockdep_assert_console_list_lock_held(void)
+{
+ lockdep_assert_held(&console_mutex);
+}
#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -227,6 +239,40 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
}
#endif /* CONFIG_PRINTK && CONFIG_SYSCTL */
+/**
+ * console_list_lock - Lock the console list
+ *
+ * For console list or console->flags updates
+ */
+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.
+ *
+ * Detecting if this context is really in the read-side critical
+ * section is only possible if the appropriate debug options are
+ * enabled.
+ */
+ WARN_ON_ONCE(debug_lockdep_rcu_enabled() &&
+ srcu_read_lock_held(&console_srcu));
+
+ mutex_lock(&console_mutex);
+}
+EXPORT_SYMBOL(console_list_lock);
+
+/**
+ * console_list_unlock - Unlock the console list
+ *
+ * Counterpart to console_list_lock()
+ */
+void console_list_unlock(void)
+{
+ mutex_unlock(&console_mutex);
+}
+EXPORT_SYMBOL(console_list_unlock);
+
/**
* console_srcu_read_lock - Register a new reader for the
* SRCU-protected console list
@@ -3020,9 +3066,11 @@ struct tty_driver *console_device(int *index)
void console_stop(struct console *console)
{
__pr_flush(console, 1000, true);
+ console_list_lock();
console_lock();
console->flags &= ~CON_ENABLED;
console_unlock();
+ console_list_unlock();
/*
* Ensure that all SRCU list walks have completed. All contexts must
@@ -3036,9 +3084,11 @@ EXPORT_SYMBOL(console_stop);
void console_start(struct console *console)
{
+ console_list_lock();
console_lock();
console->flags |= CON_ENABLED;
console_unlock();
+ console_list_unlock();
__pr_flush(console, 1000, true);
}
EXPORT_SYMBOL(console_start);
@@ -3187,6 +3237,8 @@ static void console_init_seq(struct console *newcon, bool bootcon_registered)
#define console_first() \
hlist_entry(console_list.first, struct console, node)
+static int unregister_console_locked(struct console *console);
+
/*
* The console driver calls this routine during kernel initialization
* to register the console printing procedure with printk() and to
@@ -3213,13 +3265,14 @@ void register_console(struct console *newcon)
bool realcon_registered = false;
int err;
+ console_list_lock();
+
for_each_console(con) {
if (WARN(con == newcon, "console '%s%d' already registered\n",
- con->name, con->index))
- return;
- }
+ con->name, con->index)) {
+ goto unlock;
+ }
- for_each_console(con) {
if (con->flags & CON_BOOT)
bootcon_registered = true;
else
@@ -3230,7 +3283,7 @@ void register_console(struct console *newcon)
if ((newcon->flags & CON_BOOT) && realcon_registered) {
pr_info("Too late to register bootconsole %s%d\n",
newcon->name, newcon->index);
- return;
+ goto unlock;
}
/*
@@ -3261,7 +3314,7 @@ void register_console(struct console *newcon)
/* printk() messages are not printed to the Braille console. */
if (err || newcon->flags & CON_BRL)
- return;
+ goto unlock;
/*
* If we have a bootconsole, and are switching to a real console,
@@ -3320,16 +3373,21 @@ void register_console(struct console *newcon)
hlist_for_each_entry_safe(con, tmp, &console_list, node) {
if (con->flags & CON_BOOT)
- unregister_console(con);
+ unregister_console_locked(con);
}
}
+unlock:
+ console_list_unlock();
}
EXPORT_SYMBOL(register_console);
-int unregister_console(struct console *console)
+/* Must be called under console_list_lock(). */
+static int unregister_console_locked(struct console *console)
{
int res;
+ lockdep_assert_console_list_lock_held();
+
con_printk(KERN_INFO, console, "disabled\n");
res = _braille_unregister_console(console);
@@ -3378,6 +3436,16 @@ int unregister_console(struct console *console)
return res;
}
+
+int unregister_console(struct console *console)
+{
+ int res;
+
+ console_list_lock();
+ res = unregister_console_locked(console);
+ console_list_unlock();
+ return res;
+}
EXPORT_SYMBOL(unregister_console);
/*
@@ -3430,6 +3498,7 @@ static int __init printk_late_init(void)
struct console *con;
int ret;
+ console_list_lock();
hlist_for_each_entry_safe(con, tmp, &console_list, node) {
if (!(con->flags & CON_BOOT))
continue;
@@ -3447,9 +3516,10 @@ static int __init printk_late_init(void)
*/
pr_warn("bootconsole [%s%d] uses init memory and must be disabled even before the real one is ready\n",
con->name, con->index);
- unregister_console(con);
+ unregister_console_locked(con);
}
}
+ console_list_unlock();
ret = cpuhp_setup_state_nocalls(CPUHP_PRINTK_DEAD, "printk:dead", NULL,
console_cpu_notify);
--
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/hvc/hvc_console.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 4802cfaa107f..a683e21df19c 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -264,8 +264,8 @@ static void hvc_port_destruct(struct tty_port *port)
static void hvc_check_console(int index)
{
- /* Already enabled, bail out */
- if (hvc_console.flags & CON_ENABLED)
+ /* Already registered, bail out */
+ if (console_is_registered(&hvc_console))
return;
/* If this index is what the user requested, then register
--
2.30.2
show_cons_active() uses the console_lock to gather information
on registered consoles. Since the console_lock is being used for
multiple reasons, explicitly document these reasons. This will
be useful when the console_lock is split into fine-grained
locking.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
drivers/tty/tty_io.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index de06c3c2ff70..ee4da2fec328 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3526,6 +3526,16 @@ static ssize_t show_cons_active(struct device *dev,
struct console *c;
ssize_t count = 0;
+ /*
+ * Hold the console_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();
for_each_console(c) {
if (!c->device)
--
2.30.2
configure_kgdboc() uses the console_lock for console list iteration. Use
the console_list_lock instead because list synchronization responsibility
will be removed from the console_lock in a later change.
The SRCU iterator could have been used here, but a later change will
relocate the locking of the console_list_lock to also provide
synchronization against register_console().
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: Petr Mladek <[email protected]>
---
drivers/tty/serial/kgdboc.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index c6df9ef34099..82b4b4d67823 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -193,7 +193,16 @@ static int configure_kgdboc(void)
if (!p)
goto noconfig;
+ /* For safe traversal of the console list. */
+ console_list_lock();
+
+ /*
+ * 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(cons) {
int idx;
if (cons->device && cons->device(cons, &idx) == p &&
@@ -202,8 +211,11 @@ static int configure_kgdboc(void)
break;
}
}
+
console_unlock();
+ console_list_unlock();
+
kgdb_tty_driver = p;
kgdb_tty_line = tty_line;
--
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]>
Reviewed-by: Petr Mladek <[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
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 responsibility will
be removed from the console_lock in a later change.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
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 8c2b7ccdfebf..a3ed9b34e2ab 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -558,13 +558,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 and race-free reading of @flags.
*/
- console_lock();
+ console_list_lock();
for_each_console(con) {
if (con->write && con->read &&
(con->flags & (CON_BOOT | CON_ENABLED)) &&
@@ -606,7 +606,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
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/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..0cbd1892c53b 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_registered(cdns_uart_uart_driver.cons)) {
console_port = NULL;
cdns_uart_console.index = -1;
}
--
2.30.2
The CON_ENABLED status of a console is a runtime setting that does not
involve the console driver. Drivers must not assume that if the console
is disabled then proper hardware management is not needed. For the EFI
earlycon case, it is about remapping/unmapping memory for the
framebuffer.
Use console_is_registered() instead of checking CON_ENABLED.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
drivers/firmware/efi/earlycon.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/firmware/efi/earlycon.c b/drivers/firmware/efi/earlycon.c
index a52236e11e5f..4d6c5327471a 100644
--- a/drivers/firmware/efi/earlycon.c
+++ b/drivers/firmware/efi/earlycon.c
@@ -29,8 +29,8 @@ static void *efi_fb;
*/
static int __init efi_earlycon_remap_fb(void)
{
- /* bail if there is no bootconsole or it has been disabled already */
- if (!earlycon_console || !(earlycon_console->flags & CON_ENABLED))
+ /* bail if there is no bootconsole or it was unregistered already */
+ if (!earlycon_console || !console_is_registered(earlycon_console))
return 0;
efi_fb = memremap(fb_base, screen_info.lfb_size,
@@ -42,8 +42,8 @@ early_initcall(efi_earlycon_remap_fb);
static int __init efi_earlycon_unmap_fb(void)
{
- /* unmap the bootconsole fb unless keep_bootcon has left it enabled */
- if (efi_fb && !(earlycon_console->flags & CON_ENABLED))
+ /* unmap the bootconsole fb unless keep_bootcon left it registered */
+ if (efi_fb && !console_is_registered(earlycon_console))
memunmap(efi_fb);
return 0;
}
--
2.30.2
The @bootcon_enabled and @realcon_enabled local variables actually
represent if such console types are registered. In general there
has been a confusion about enabled vs. registered. Incorrectly
naming such variables promotes such confusion.
Rename the variables to _registered.
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
kernel/printk/printk.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 173f46a29252..3d449dfb1ed6 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3156,8 +3156,8 @@ static void try_enable_default_console(struct console *newcon)
void register_console(struct console *newcon)
{
struct console *con;
- bool bootcon_enabled = false;
- bool realcon_enabled = false;
+ bool bootcon_registered = false;
+ bool realcon_registered = false;
int err;
for_each_console(con) {
@@ -3168,13 +3168,13 @@ void register_console(struct console *newcon)
for_each_console(con) {
if (con->flags & CON_BOOT)
- bootcon_enabled = true;
+ bootcon_registered = true;
else
- realcon_enabled = true;
+ realcon_registered = true;
}
/* Do not register boot consoles when there already is a real one. */
- if (newcon->flags & CON_BOOT && realcon_enabled) {
+ if ((newcon->flags & CON_BOOT) && realcon_registered) {
pr_info("Too late to register bootconsole %s%d\n",
newcon->name, newcon->index);
return;
@@ -3216,7 +3216,7 @@ void register_console(struct console *newcon)
* the real console are the same physical device, it's annoying to
* see the beginning boot messages twice
*/
- if (bootcon_enabled &&
+ if (bootcon_registered &&
((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV)) {
newcon->flags &= ~CON_PRINTBUFFER;
}
@@ -3268,7 +3268,7 @@ void register_console(struct console *newcon)
* went to the bootconsole (that they do not see on the real console)
*/
con_printk(KERN_INFO, newcon, "enabled\n");
- if (bootcon_enabled &&
+ if (bootcon_registered &&
((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
!keep_bootcon) {
struct hlist_node *tmp;
--
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]>
Reviewed-by: Petr Mladek <[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 f7479fd73114..35018f18f5aa 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);
@@ -3342,7 +3342,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 {
@@ -3399,7 +3399,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();
@@ -3418,7 +3418,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
Hi,
On Wed, Nov 16, 2022 at 8:22 AM John Ogness <[email protected]> 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 responsibility will
> be removed from the console_lock in a later change.
>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Daniel Thompson <[email protected]>
> Reviewed-by: Petr Mladek <[email protected]>
> ---
> 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 8c2b7ccdfebf..a3ed9b34e2ab 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -558,13 +558,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 and race-free reading of @flags.
> */
> - console_lock();
> + console_list_lock();
> for_each_console(con) {
> if (con->write && con->read &&
> (con->flags & (CON_BOOT | CON_ENABLED)) &&
Officially don't we need both the list lock and normal lock here since
we're reaching into the consoles?
-Doug
Hi,
On Wed, Nov 16, 2022 at 8:22 AM John Ogness <[email protected]> wrote:
>
> configure_kgdboc() uses the console_lock for console list iteration. Use
> the console_list_lock instead because list synchronization responsibility
> will be removed from the console_lock in a later change.
>
> The SRCU iterator could have been used here, but a later change will
> relocate the locking of the console_list_lock to also provide
> synchronization against register_console().
>
> 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: Petr Mladek <[email protected]>
> ---
> drivers/tty/serial/kgdboc.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Wed, Nov 16, 2022 at 8:22 AM John Ogness <[email protected]> wrote:
>
> Use srcu console list iteration for safe console list traversal.
> 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]>
> ---
> drivers/tty/serial/kgdboc.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 5be381003e58..c6df9ef34099 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -451,6 +451,7 @@ static void kgdboc_earlycon_pre_exp_handler(void)
> {
> struct console *con;
> static bool already_warned;
> + int cookie;
>
> if (already_warned)
> return;
> @@ -463,9 +464,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;
Is there truly any guarantee that "con" will be NULL if
for_each_console_srcu() finishes naturally (AKA without a "break"
being executed)?
It looks as if currently this will be true but nothing in the comments
of for_each_console_srcu() nor hlist_for_each_entry_srcu() (which it
calls) guarantees this, right? It would be nice if that was
documented, but I guess it's not a huge deal.
Also: wasn't there just some big issue about people using loop
iteration variables after the loop finished?
https://lwn.net/Articles/885941/
Ah, I guess that's a slightly different problem and probably not relevant here.
So it seems like this is fine.
Reviewed-by: Douglas Anderson <[email protected]>
On Wed, Nov 16, 2022 at 5:22 PM John Ogness <[email protected]> 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]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
Acked-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Doug,
On 2022-11-16, Doug Anderson <[email protected]> wrote:
>> @@ -463,9 +464,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;
>
> Is there truly any guarantee that "con" will be NULL if
> for_each_console_srcu() finishes naturally (AKA without a "break"
> being executed)?
Right now it is true because @con becoming NULL is the exit criteria for
the loop.
> It looks as if currently this will be true but nothing in the comments
> of for_each_console_srcu() nor hlist_for_each_entry_srcu() (which it
> calls) guarantees this, right? It would be nice if that was
> documented, but I guess it's not a huge deal.
Yes, if it is frowned upon that the iterator is used outside the loop,
it would be nice if the for_each macros explicitly provided some hints
in their documentation.
> Also: wasn't there just some big issue about people using loop
> iteration variables after the loop finished?
>
> https://lwn.net/Articles/885941/
Thanks for referencing that article! Indeed if the macros are changed so
that the iterator is defined in the loop, then code like this will
break. But I would expect that making such macro changes will also
require updating the call sites to avoid unused variables outside the
loops. And then this code could receive the appropriate fixup.
I feel like if I add extra code to guarantee a NULL without relying on
the macro implementation, I'll get more resistance due to unnecessarily
adding code and variables. But I may be wrong.
> Reviewed-by: Douglas Anderson <[email protected]>
Thanks.
John
On 2022-11-16, Doug Anderson <[email protected]> wrote:
>> /*
>> - * 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 and race-free reading of @flags.
>> */
>> - console_lock();
>> + console_list_lock();
>> for_each_console(con) {
>> if (con->write && con->read &&
>> (con->flags & (CON_BOOT | CON_ENABLED)) &&
>
> Officially don't we need both the list lock and normal lock here since
> we're reaching into the consoles?
AFAICT the only synchronization we need here is iterating the console
list, reading con->flags of a registered console, and modifying
con->exit of a registered console. The console_list_lock provides
synchronization for all of these things. By the end of this series the
console_lock does not provide synchronization for any of these things.
Is there something else that requires synchronization here?
After this series the console_lock is still responsible for:
- serializing console->write() callbacks
- stopping console->write() callbacks
- stopping console->device() callbacks
- synchronizing console->seq
- synchronizing console->dropped
- synchronizing the global @console_suspended
- providing various unclear protection for vt consoles
- some bizarre misuses in bcache
The scope may be larger than the above list. The investigation is still
ongoing.
John
On Wed 2022-11-16 17:27:12, John Ogness wrote:
> This is v5 of a series to prepare for threaded/atomic
> printing. v4 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.
The patchset looks ready for linux-next from my POV.
I am going to push it there right now to get as much testing
as possible before the merge window.
Any review and comments are still appreciate. We could always
take it back if some critical problems are discovered and
can't be solved easily.
Best Regards,
Petr
On Fri 2022-11-18 12:22:58, Petr Mladek wrote:
> On Wed 2022-11-16 17:27:12, John Ogness wrote:
> > This is v5 of a series to prepare for threaded/atomic
> > printing. v4 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.
>
> The patchset looks ready for linux-next from my POV.
>
> I am going to push it there right now to get as much testing
> as possible before the merge window.
JFYI, the patchset is committed in printk/linux.git,
branch rework/console-list-lock.
I'll eventually merge it into rework/kthreads. But I wanted to have
it separated until it gets some more testing in linux-next and
eventually some more review.
Best Regards,
Petr
Currently there exist races in register_console(), where the types
of registered consoles are checked (without holding the console_lock)
and then after acquiring the console_lock, it is assumed that the list
has not changed. Also, some code that performs console_unregister()
make similar assumptions.
It might be possible to fix these races using the console_lock. But
it would require a complex analysis of all console drivers to make
sure that the console_lock is not taken in match() and setup()
callbacks. And we really prefer to split up and reduce the
responsibilities of console_lock rather than expand its complexity.
Therefore, introduce a new console_list_lock to provide full
synchronization for any console list changes.
In addition, also use console_list_lock for synchronization of
console->flags updates. All flags are either static or modified only
during the console registration. There are only two exceptions.
The first exception is CON_ENABLED, which is also modified by
console_start()/console_stop(). Therefore, these functions must
also take the console_list_lock.
The second exception is when the flags are modified by the console
driver init code before the console is registered. These will be
ignored because they are not visible to the rest of the system
via the console_drivers list.
Note that one of the various responsibilities of the console_lock is
also intended to provide console list and console->flags
synchronization. Later changes will update call sites relying on the
console_lock for these purposes. Once all call sites have been
updated, the console_lock will be relieved of synchronizing
console_list and console->flags updates.
Suggested-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
---
Stephen reported [0] a build failure with linux-next. The problem was a
missing EXPORT of @lockdep_assert_console_list_lock_held for when
drivers are built as modules.
[0] https://lore.kernel.org/lkml/[email protected]
include/linux/console.h | 23 +++++++++--
kernel/printk/printk.c | 89 ++++++++++++++++++++++++++++++++++++-----
2 files changed, 100 insertions(+), 12 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index f4f0c9523835..24d83e24840b 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -158,6 +158,14 @@ struct console {
struct hlist_node node;
};
+#ifdef CONFIG_LOCKDEP
+extern void lockdep_assert_console_list_lock_held(void);
+#else
+static inline void lockdep_assert_console_list_lock_held(void)
+{
+}
+#endif
+
#ifdef CONFIG_DEBUG_LOCK_ALLOC
extern bool console_srcu_read_lock_is_held(void);
#else
@@ -170,6 +178,9 @@ static inline bool console_srcu_read_lock_is_held(void)
extern int console_srcu_read_lock(void);
extern void console_srcu_read_unlock(int cookie);
+extern void console_list_lock(void) __acquires(console_mutex);
+extern void console_list_unlock(void) __releases(console_mutex);
+
extern struct hlist_head console_list;
/**
@@ -186,10 +197,16 @@ extern struct hlist_head console_list;
hlist_for_each_entry_srcu(con, &console_list, node, \
console_srcu_read_lock_is_held())
-/*
- * for_each_console() allows you to iterate on each console
+/**
+ * for_each_console() - Iterator over registered consoles
+ * @con: struct console pointer used as loop cursor
+ *
+ * The console list and the console->flags are immutable while iterating.
+ *
+ * Requires console_list_lock to be held.
*/
-#define for_each_console(con) \
+#define for_each_console(con) \
+ lockdep_assert_console_list_lock_held(); \
hlist_for_each_entry(con, &console_list, node)
extern int console_set_on_cmdline;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c84654444a02..2b4506673a86 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -78,6 +78,13 @@ EXPORT_SYMBOL(ignore_console_lock_warning);
int oops_in_progress;
EXPORT_SYMBOL(oops_in_progress);
+/*
+ * console_mutex protects console_list updates and console->flags updates.
+ * The flags are synchronized only for consoles that are registered, i.e.
+ * accessible via the console list.
+ */
+static DEFINE_MUTEX(console_mutex);
+
/*
* console_sem protects console_list and console->flags updates, and also
* provides serialization for access to the entire console driver system.
@@ -103,6 +110,12 @@ static int __read_mostly suppress_panic_printk;
static struct lockdep_map console_lock_dep_map = {
.name = "console_lock"
};
+
+void lockdep_assert_console_list_lock_held(void)
+{
+ lockdep_assert_held(&console_mutex);
+}
+EXPORT_SYMBOL(lockdep_assert_console_list_lock_held);
#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -227,6 +240,40 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
}
#endif /* CONFIG_PRINTK && CONFIG_SYSCTL */
+/**
+ * console_list_lock - Lock the console list
+ *
+ * For console list or console->flags updates
+ */
+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.
+ *
+ * Detecting if this context is really in the read-side critical
+ * section is only possible if the appropriate debug options are
+ * enabled.
+ */
+ WARN_ON_ONCE(debug_lockdep_rcu_enabled() &&
+ srcu_read_lock_held(&console_srcu));
+
+ mutex_lock(&console_mutex);
+}
+EXPORT_SYMBOL(console_list_lock);
+
+/**
+ * console_list_unlock - Unlock the console list
+ *
+ * Counterpart to console_list_lock()
+ */
+void console_list_unlock(void)
+{
+ mutex_unlock(&console_mutex);
+}
+EXPORT_SYMBOL(console_list_unlock);
+
/**
* console_srcu_read_lock - Register a new reader for the
* SRCU-protected console list
@@ -3020,9 +3067,11 @@ struct tty_driver *console_device(int *index)
void console_stop(struct console *console)
{
__pr_flush(console, 1000, true);
+ console_list_lock();
console_lock();
console->flags &= ~CON_ENABLED;
console_unlock();
+ console_list_unlock();
/*
* Ensure that all SRCU list walks have completed. All contexts must
@@ -3036,9 +3085,11 @@ EXPORT_SYMBOL(console_stop);
void console_start(struct console *console)
{
+ console_list_lock();
console_lock();
console->flags |= CON_ENABLED;
console_unlock();
+ console_list_unlock();
__pr_flush(console, 1000, true);
}
EXPORT_SYMBOL(console_start);
@@ -3187,6 +3238,8 @@ static void console_init_seq(struct console *newcon, bool bootcon_registered)
#define console_first() \
hlist_entry(console_list.first, struct console, node)
+static int unregister_console_locked(struct console *console);
+
/*
* The console driver calls this routine during kernel initialization
* to register the console printing procedure with printk() and to
@@ -3213,13 +3266,14 @@ void register_console(struct console *newcon)
bool realcon_registered = false;
int err;
+ console_list_lock();
+
for_each_console(con) {
if (WARN(con == newcon, "console '%s%d' already registered\n",
- con->name, con->index))
- return;
- }
+ con->name, con->index)) {
+ goto unlock;
+ }
- for_each_console(con) {
if (con->flags & CON_BOOT)
bootcon_registered = true;
else
@@ -3230,7 +3284,7 @@ void register_console(struct console *newcon)
if ((newcon->flags & CON_BOOT) && realcon_registered) {
pr_info("Too late to register bootconsole %s%d\n",
newcon->name, newcon->index);
- return;
+ goto unlock;
}
/*
@@ -3261,7 +3315,7 @@ void register_console(struct console *newcon)
/* printk() messages are not printed to the Braille console. */
if (err || newcon->flags & CON_BRL)
- return;
+ goto unlock;
/*
* If we have a bootconsole, and are switching to a real console,
@@ -3320,16 +3374,21 @@ void register_console(struct console *newcon)
hlist_for_each_entry_safe(con, tmp, &console_list, node) {
if (con->flags & CON_BOOT)
- unregister_console(con);
+ unregister_console_locked(con);
}
}
+unlock:
+ console_list_unlock();
}
EXPORT_SYMBOL(register_console);
-int unregister_console(struct console *console)
+/* Must be called under console_list_lock(). */
+static int unregister_console_locked(struct console *console)
{
int res;
+ lockdep_assert_console_list_lock_held();
+
con_printk(KERN_INFO, console, "disabled\n");
res = _braille_unregister_console(console);
@@ -3378,6 +3437,16 @@ int unregister_console(struct console *console)
return res;
}
+
+int unregister_console(struct console *console)
+{
+ int res;
+
+ console_list_lock();
+ res = unregister_console_locked(console);
+ console_list_unlock();
+ return res;
+}
EXPORT_SYMBOL(unregister_console);
/*
@@ -3430,6 +3499,7 @@ static int __init printk_late_init(void)
struct console *con;
int ret;
+ console_list_lock();
hlist_for_each_entry_safe(con, tmp, &console_list, node) {
if (!(con->flags & CON_BOOT))
continue;
@@ -3447,9 +3517,10 @@ static int __init printk_late_init(void)
*/
pr_warn("bootconsole [%s%d] uses init memory and must be disabled even before the real one is ready\n",
con->name, con->index);
- unregister_console(con);
+ unregister_console_locked(con);
}
}
+ console_list_unlock();
ret = cpuhp_setup_state_nocalls(CPUHP_PRINTK_DEAD, "printk:dead", NULL,
console_cpu_notify);
--
2.30.2
On Mon 2022-11-21 12:16:12, John Ogness wrote:
> Currently there exist races in register_console(), where the types
> of registered consoles are checked (without holding the console_lock)
> and then after acquiring the console_lock, it is assumed that the list
> has not changed. Also, some code that performs console_unregister()
> make similar assumptions.
>
> It might be possible to fix these races using the console_lock. But
> it would require a complex analysis of all console drivers to make
> sure that the console_lock is not taken in match() and setup()
> callbacks. And we really prefer to split up and reduce the
> responsibilities of console_lock rather than expand its complexity.
> Therefore, introduce a new console_list_lock to provide full
> synchronization for any console list changes.
>
> In addition, also use console_list_lock for synchronization of
> console->flags updates. All flags are either static or modified only
> during the console registration. There are only two exceptions.
>
> The first exception is CON_ENABLED, which is also modified by
> console_start()/console_stop(). Therefore, these functions must
> also take the console_list_lock.
>
> The second exception is when the flags are modified by the console
> driver init code before the console is registered. These will be
> ignored because they are not visible to the rest of the system
> via the console_drivers list.
>
> Note that one of the various responsibilities of the console_lock is
> also intended to provide console list and console->flags
> synchronization. Later changes will update call sites relying on the
> console_lock for these purposes. Once all call sites have been
> updated, the console_lock will be relieved of synchronizing
> console_list and console->flags updates.
>
> Suggested-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Petr Mladek <[email protected]>
> ---
> Stephen reported [0] a build failure with linux-next. The problem was a
> missing EXPORT of @lockdep_assert_console_list_lock_held for when
> drivers are built as modules.
>
> [0] https://lore.kernel.org/lkml/[email protected]
JFYI, the branch rework/console-list-lock in printk/linux.git has been
rebased using this version of the patch.
Thanks a lot for the fix.
Best Regards,
Petr
On Wed, Nov 16, 2022 at 05:27:12PM +0106, John Ogness wrote:
> This is v5 of a series to prepare for threaded/atomic
> printing. v4 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 is 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 v4:
>
> printk:
>
> - Introduce console_init_seq() to handle the now rather complex
> procedure to find an appropriate start sequence number for a
> new console upon registration.
>
> - When registering a non-boot console and boot consoles are
> registered, try to flush all the consoles to get the next @seq
> value before falling back to use the @seq of the enabled boot
> console that is furthest behind.
>
> - For console_force_preferred_locked(), make the console the
> head of the console list.
>
Reviewed-by: Greg Kroah-Hartman <[email protected]>