[PATCH printk v2] printk: Avoid multiple unregistration in unregister_console_locked()

[PATCH printk v2] printk: Avoid multiple unregistration in unregister_console_locked()

[John Ogness]

unregister_console_locked() is called from within
hlist_for_each_entry_safe(console_list) loops. This would be safe if
unregister_console_locked() only unregistered the specified console. After
all, that is what the _safe variant is for.

However, unregister_console_locked() calls printk_kthreads_check_locked(),
which can unregister _multiple_ consoles if kthreads fail to start. This
could lead to hlist_for_each_entry_safe() use-after-free scenarios because
not only was the list entry removed, but possibly the following entry as
well (the @n temporary storage of the next entry).

Move the call to printk_kthreads_check_locked() outside of
unregister_console_locked(). Add comments to both functions to clarify
if they are safe to be used in hlist_for_each_entry_safe(console_list)
loops.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/lkml/87ik31iz4x.fsf@jogness.linutronix.de
Fixes: 76f258bf3f2a ("printk: nbcon: Introduce printer kthreads")
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
v1: https://lore.kernel.org/lkml/20261029152454.638952-1-john.ogness@linutronix.de

changes since v1:

- In printk_late_init(), move printk_kthreads_check_locked() outside of
  the hlist_for_each_entry_safe(console_list) loop.

 kernel/printk/printk.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9a205f94bfac7..5dbcf8a9909bb 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3792,6 +3792,13 @@ static struct syscore printk_syscore = {
  * If appropriate, start nbcon kthreads and set @printk_kthreads_running.
  * If any kthreads fail to start, those consoles are unregistered.
  *
+ * Called after consoles are registered or unregistered.
+ *
+ * This MUST NOT be called within an hlist_for_each_entry_safe(console_list)
+ * loop because it can unregister multiple consoles if any kthreads fail to
+ * start. That would lead to use-after-free scenarios within the
+ * hlist_for_each_entry_safe(console_list) loop of the caller.
+ *
  * Must be called under console_list_lock().
  */
 static void printk_kthreads_check_locked(void)
@@ -4230,7 +4237,20 @@ void register_console(struct console *newcon)
 }
 EXPORT_SYMBOL(register_console);
 
-/* Must be called under console_list_lock(). */
+/*
+ * Unregister the specified console.
+ *
+ * This can be called safely from within an
+ * hlist_for_each_entry_safe(console_list) loop because it unregisters
+ * _only_ the specified console.
+ *
+ * Afterwards the caller should call printk_kthreads_check_locked() since
+ * unregistering a console may require printer threads to start/stop. Note
+ * that printk_kthreads_check_locked() cannot be called safely from within
+ * an hlist_for_each_entry_safe(console_list) loop.
+ *
+ * Must be called under console_list_lock().
+ */
 static int unregister_console_locked(struct console *console)
 {
 	bool use_device_lock = (console->flags & CON_NBCON) && console->write_atomic;
@@ -4322,9 +4342,6 @@ static int unregister_console_locked(struct console *console)
 	if (console->exit)
 		res = console->exit(console);
 
-	/* Changed console list, may require printer threads to start/stop. */
-	printk_kthreads_check_locked();
-
 	return res;
 }
 
@@ -4333,7 +4350,12 @@ int unregister_console(struct console *console)
 	int res;
 
 	console_list_lock();
+
 	res = unregister_console_locked(console);
+
+	/* Changed console list, may require printer threads to start/stop. */
+	printk_kthreads_check_locked();
+
 	console_list_unlock();
 	return res;
 }
@@ -4472,6 +4494,10 @@ static int __init printk_late_init(void)
 
 		unregister_console_locked(con);
 	}
+
+	/* Changed console list, may require printer threads to start/stop. */
+	printk_kthreads_check_locked();
+
 	console_list_unlock();
 
 	ret = cpuhp_setup_state_nocalls(CPUHP_PRINTK_DEAD, "printk:dead", NULL,

base-commit: 647ef123d19695b5d8c79e03c86fb17c558e30b7
-- 
2.47.3

Re: [PATCH printk v2] printk: Avoid multiple unregistration in unregister_console_locked()

[Petr Mladek]

On Fri 2026-10-30 15:03:55, John Ogness wrote:
> unregister_console_locked() is called from within
> hlist_for_each_entry_safe(console_list) loops. This would be safe if
> unregister_console_locked() only unregistered the specified console. After
> all, that is what the _safe variant is for.
> 
> However, unregister_console_locked() calls printk_kthreads_check_locked(),
> which can unregister _multiple_ consoles if kthreads fail to start. This
> could lead to hlist_for_each_entry_safe() use-after-free scenarios because
> not only was the list entry removed, but possibly the following entry as
> well (the @n temporary storage of the next entry).
> 
> Move the call to printk_kthreads_check_locked() outside of
> unregister_console_locked(). Add comments to both functions to clarify
> if they are safe to be used in hlist_for_each_entry_safe(console_list)
> loops.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/lkml/87ik31iz4x.fsf@jogness.linutronix.de
> Fixes: 76f258bf3f2a ("printk: nbcon: Introduce printer kthreads")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Looks good to me:

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

Now, I am not sure whether to queue this for 7.4-rc2 or 7.5.
One one hand, it is a potential use-after free. On the other hand,
it is rather a corner case which is hard to hit. And it would likely
happen during boot where it is not that dangerous.

Well, I am rather confident with the fix now. So I personally
prefer 7.4-rc2 to be on the safe side sooner. And there are
already two more changes in the queue for 7.4-rc2 anyway.

Best Regards,
Petr

Re: [PATCH printk v2] printk: Avoid multiple unregistration in unregister_console_locked()

[Derek Barbosa]

On Fri, Oct 30, 2026 at 03:03:55PM +0106, John Ogness wrote:
> unregister_console_locked() is called from within
> hlist_for_each_entry_safe(console_list) loops. This would be safe if
> unregister_console_locked() only unregistered the specified console. After
> all, that is what the _safe variant is for.
> 
> However, unregister_console_locked() calls printk_kthreads_check_locked(),
> which can unregister _multiple_ consoles if kthreads fail to start. This
> could lead to hlist_for_each_entry_safe() use-after-free scenarios because
> not only was the list entry removed, but possibly the following entry as
> well (the @n temporary storage of the next entry).
> 
> Move the call to printk_kthreads_check_locked() outside of
> unregister_console_locked(). Add comments to both functions to clarify
> if they are safe to be used in hlist_for_each_entry_safe(console_list)
> loops.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/lkml/87ik31iz4x.fsf@jogness.linutronix.de
> Fixes: 76f258bf3f2a ("printk: nbcon: Introduce printer kthreads")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
> v1: https://lore.kernel.org/lkml/20261029152454.638952-1-john.ogness@linutronix.de
> 
> changes since v1:
> 
> - In printk_late_init(), move printk_kthreads_check_locked() outside of
>   the hlist_for_each_entry_safe(console_list) loop.
> 
>  kernel/printk/printk.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 9a205f94bfac7..5dbcf8a9909bb 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3792,6 +3792,13 @@ static struct syscore printk_syscore = {
>   * If appropriate, start nbcon kthreads and set @printk_kthreads_running.
>   * If any kthreads fail to start, those consoles are unregistered.
>   *
> + * Called after consoles are registered or unregistered.
> + *
> + * This MUST NOT be called within an hlist_for_each_entry_safe(console_list)
> + * loop because it can unregister multiple consoles if any kthreads fail to
> + * start. That would lead to use-after-free scenarios within the
> + * hlist_for_each_entry_safe(console_list) loop of the caller.
> + *
>   * Must be called under console_list_lock().
>   */
>  static void printk_kthreads_check_locked(void)
> @@ -4230,7 +4237,20 @@ void register_console(struct console *newcon)
>  }
>  EXPORT_SYMBOL(register_console);
>  
> -/* Must be called under console_list_lock(). */
> +/*
> + * Unregister the specified console.
> + *
> + * This can be called safely from within an
> + * hlist_for_each_entry_safe(console_list) loop because it unregisters
> + * _only_ the specified console.
> + *
> + * Afterwards the caller should call printk_kthreads_check_locked() since
> + * unregistering a console may require printer threads to start/stop. Note
> + * that printk_kthreads_check_locked() cannot be called safely from within
> + * an hlist_for_each_entry_safe(console_list) loop.
> + *
> + * Must be called under console_list_lock().
> + */
>  static int unregister_console_locked(struct console *console)
>  {
>  	bool use_device_lock = (console->flags & CON_NBCON) && console->write_atomic;
> @@ -4322,9 +4342,6 @@ static int unregister_console_locked(struct console *console)
>  	if (console->exit)
>  		res = console->exit(console);
>  
> -	/* Changed console list, may require printer threads to start/stop. */
> -	printk_kthreads_check_locked();
> -
>  	return res;
>  }
>  
> @@ -4333,7 +4350,12 @@ int unregister_console(struct console *console)
>  	int res;
>  
>  	console_list_lock();
> +
>  	res = unregister_console_locked(console);
> +
> +	/* Changed console list, may require printer threads to start/stop. */
> +	printk_kthreads_check_locked();
> +
>  	console_list_unlock();
>  	return res;
>  }
> @@ -4472,6 +4494,10 @@ static int __init printk_late_init(void)
>  
>  		unregister_console_locked(con);
>  	}
> +
> +	/* Changed console list, may require printer threads to start/stop. */
> +	printk_kthreads_check_locked();
> +
>  	console_list_unlock();
>  
>  	ret = cpuhp_setup_state_nocalls(CPUHP_PRINTK_DEAD, "printk:dead", NULL,
> 
> base-commit: 647ef123d19695b5d8c79e03c86fb17c558e30b7
> -- 
> 2.47.3
> 

Tested on an x86_64 machine with 16550 UART (console=ttyS0,115200) with patch
applied on top of commit:

d89e1d186ec9271bf78e62771779e9fc03bf0fc3 ("Merge tag 'fscrypt-for-linus' of
git://git.kernel.org/pub/scm/fs/fscrypt/linux")

Machine information:

    Dell PowerEdge R660

    Architecture:                x86_64
      CPU op-mode(s):            32-bit, 64-bit
      Address sizes:             52 bits physical, 57 bits virtual
      Byte Order:                Little Endian
    CPU(s):                      96
      On-line CPU(s) list:       0-95
    Vendor ID:                   GenuineIntel
      BIOS Vendor ID:            Intel
      Model name:                INTEL(R) XEON(R) GOLD 6542Y
        BIOS Model name:         INTEL(R) XEON(R) GOLD 6542Y  CPU @ 2.9GHz

Tested with qemu_kvm, console_blast, and printk_kthread tests. [0]

[0] https://gitlab.com/debarbos/printktorture

Tested-by: Derek Barbosa <debarbos@redhat.com>
-- 
Derek <debarbos@redhat.com>