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

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

[Petr Mladek]

On Thu 2026-10-29 16:48:53, John Ogness wrote:
> On 2026-10-29, Sashiko wrote:
> >> @@ -4471,6 +4493,9 @@ 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();
> >>  	}
> >
> > Should printk_kthreads_check_locked() be called after the
> > hlist_for_each_entry_safe(con, tmp, &console_list, node) loop in
> > printk_late_init() rather than inside the loop body?
> >
> > In kernel/printk/printk.c:printk_late_init():
> >
> >	hlist_for_each_entry_safe(con, tmp, &console_list, node) {
> >		if (!(con->flags & CON_BOOT))
> >			continue;
> >		...
> >		unregister_console_locked(con);
> >
> >		/* Changed console list, may require printer threads to start/stop. */
> >		printk_kthreads_check_locked();
> >	}
> >	console_list_unlock();
> >
> > Because printk_kthreads_check_locked() is placed inside the loop,
> > unregistering the last boot console sets have_boot_console to false and
> > immediately attempts to start printer kthreads while the loop is still
> > iterating over console_list.
> >
> > If kthread creation in legacy_kthread_create() or nbcon_kthread_create()
> > fails for the console stored in tmp, printk_kthreads_check_locked() calls
> > unregister_console_locked() on that console, removing it with
> > hlist_del_init_rcu(), freeing nbcon resources, and invoking console->exit().
> > On the next iteration of printk_late_init(), con is set to tmp, accessing the
> > already-unregistered console and terminating the walk early because
> > hlist_del_init_rcu() cleared tmp->next.
> 
> Geez, sorry. Obviously printk_kthreads_check_locked() needs to be
> outside of the hlist_for_each_entry_safe(console_list) loop. That was
> the whole point of the series!
> 
> I will wait a day for v2 in case there is any feedback regarding my
> comments (or lack thereof).

JFYI, the comments look good to me.

It is quite twisted problem. I have got lost again when looking at
this patch. I am not surprised that the 1st version was not right. ;-)

Best Regards,
Petr

[PATCH printk] 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>
---
 kernel/printk/printk.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9a205f94bfac7..d7763a7269918 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;
 }
@@ -4471,6 +4493,9 @@ 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();
 

base-commit: 647ef123d19695b5d8c79e03c86fb17c558e30b7
-- 
2.47.3

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

[John Ogness]

On 2026-10-29, Sashiko wrote:
>> @@ -4471,6 +4493,9 @@ 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();
>>  	}
>
> Should printk_kthreads_check_locked() be called after the
> hlist_for_each_entry_safe(con, tmp, &console_list, node) loop in
> printk_late_init() rather than inside the loop body?
>
> In kernel/printk/printk.c:printk_late_init():
>
>	hlist_for_each_entry_safe(con, tmp, &console_list, node) {
>		if (!(con->flags & CON_BOOT))
>			continue;
>		...
>		unregister_console_locked(con);
>
>		/* Changed console list, may require printer threads to start/stop. */
>		printk_kthreads_check_locked();
>	}
>	console_list_unlock();
>
> Because printk_kthreads_check_locked() is placed inside the loop,
> unregistering the last boot console sets have_boot_console to false and
> immediately attempts to start printer kthreads while the loop is still
> iterating over console_list.
>
> If kthread creation in legacy_kthread_create() or nbcon_kthread_create()
> fails for the console stored in tmp, printk_kthreads_check_locked() calls
> unregister_console_locked() on that console, removing it with
> hlist_del_init_rcu(), freeing nbcon resources, and invoking console->exit().
> On the next iteration of printk_late_init(), con is set to tmp, accessing the
> already-unregistered console and terminating the walk early because
> hlist_del_init_rcu() cleared tmp->next.

Geez, sorry. Obviously printk_kthreads_check_locked() needs to be
outside of the hlist_for_each_entry_safe(console_list) loop. That was
the whole point of the series!

I will wait a day for v2 in case there is any feedback regarding my
comments (or lack thereof).

John

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

[Derek Barbosa]

On Thu, Oct 29, 2026 at 04:48:53PM +0106, John Ogness wrote:
> On 2026-10-29, Sashiko wrote:
> >> @@ -4471,6 +4493,9 @@ 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();
> >>  	}
> >
> > Should printk_kthreads_check_locked() be called after the
> > hlist_for_each_entry_safe(con, tmp, &console_list, node) loop in
> > printk_late_init() rather than inside the loop body?
> >
> > In kernel/printk/printk.c:printk_late_init():
> >
> >	hlist_for_each_entry_safe(con, tmp, &console_list, node) {
> >		if (!(con->flags & CON_BOOT))
> >			continue;
> >		...
> >		unregister_console_locked(con);
> >
> >		/* Changed console list, may require printer threads to start/stop. */
> >		printk_kthreads_check_locked();
> >	}
> >	console_list_unlock();
> >
> > Because printk_kthreads_check_locked() is placed inside the loop,
> > unregistering the last boot console sets have_boot_console to false and
> > immediately attempts to start printer kthreads while the loop is still
> > iterating over console_list.
> >
> > If kthread creation in legacy_kthread_create() or nbcon_kthread_create()
> > fails for the console stored in tmp, printk_kthreads_check_locked() calls
> > unregister_console_locked() on that console, removing it with
> > hlist_del_init_rcu(), freeing nbcon resources, and invoking console->exit().
> > On the next iteration of printk_late_init(), con is set to tmp, accessing the
> > already-unregistered console and terminating the walk early because
> > hlist_del_init_rcu() cleared tmp->next.
> 
> Geez, sorry. Obviously printk_kthreads_check_locked() needs to be
> outside of the hlist_for_each_entry_safe(console_list) loop. That was
> the whole point of the series!
> 
> I will wait a day for v2 in case there is any feedback regarding my
> comments (or lack thereof).

Thank you for adding in the comments above check_locked and
unregister_console_lock :)

In fact, It looks like it even helped Sashiko catch this.

Once v2 is live I will pull it down and spin some tests.

> 
> John
> 

-- 
Derek <debarbos@redhat.com>