mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: John Ogness <john.ogness@linutronix.de>
To: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Derek Barbosa <debarbos@redhat.com>,
	linux-kernel@vger.kernel.org, Thomas Gleixner <tglx@kernel.org>
Subject: Re: [PATCH printk v2] printk: Postpone kthread startup
Date: Fri, 16 Oct 2026 15:00:54 +0206	[thread overview]
Message-ID: <87ik31iz4x.fsf@jogness.linutronix.de> (raw)
In-Reply-To: <20261016085441.857695-1-john.ogness@linutronix.de>

On 2026-10-16, sashiko.dev wrote:
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index 0ed86b5e8b160..9a205f94bfac7 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -3864,7 +3864,7 @@ static int __init printk_set_kthreads_ready(void)
>>
>>  	return 0;
>>  }
>> -early_initcall(printk_set_kthreads_ready);
>> +subsys_initcall(printk_set_kthreads_ready);
>
> Can this delay expose a regression where a use-after-free occurs
> during failure paths?
>
> If nbcon_kthread_create(con) fails, it calls
> unregister_console_locked(con).  This unregisters the console and
> recursively calls printk_kthreads_check_locked().
>
> The recursive call restarts iterating over console_list and may
> unregister subsequent consoles, including the tmp node held by the
> outer loop:
>
> kernel/printk/printk.c:printk_kthreads_check_locked() {
> 	hlist_for_each_entry_safe(con, tmp, &console_list, node) {
>		if (!(con->flags & CON_NBCON))
>			continue;
>
>		if (!nbcon_kthread_create(con))
>			unregister_console_locked(con);
>	}
> }
>
> kernel/printk/printk.c:unregister_console_locked() {
>	...
>	/* Changed console list, may require printer threads to start/stop. */
>	printk_kthreads_check_locked();
>	...
> }
>
> Because hlist_for_each_entry_safe relies on the tmp pointer, the outer
> loop will later process the already unregistered tmp console and call
> nbcon_kthread_create(tmp) on it.
>
> If this succeeds on the retry, it spawns a zombie kthread for a
> detached console. If the detached console is dynamically allocated and
> freed by its driver's exit routine, the kthread and the outer loop
> will dereference freed memory.
>
> Can the loop in printk_kthreads_check_locked() handle the recursive
> unregistration of the tmp pointer safely without resulting in a
> use-after-free?

The case Sashiko presents is possible (even if unlikely). I would not
associate it with this patch though. It is a pre-existing
condition. Probably the cleanest fix is to pull
printk_kthreads_check_locked() out of
unregister_console_locked(). Something like this:

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 0ed86b5e8b160..0a87f49b3908a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -4322,9 +4322,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 +4330,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 +4473,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();

I can submit an official patch after the 7.4 merge window.

John

  reply	other threads:[~2026-10-16 12:54 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-10-16  8:54 John Ogness
2026-10-16 12:54 ` John Ogness [this message]
2026-10-16 14:43   ` Petr Mladek
2026-10-16 15:56     ` Derek Barbosa
2026-10-16 13:36 ` Derek Barbosa
2026-10-16 15:06   ` Petr Mladek
2026-10-16 16:51     ` Derek Barbosa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87ik31iz4x.fsf@jogness.linutronix.de \
    --to=john.ogness@linutronix.de \
    --cc=debarbos@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pmladek@suse.com \
    --cc=rostedt@goodmis.org \
    --cc=senozhatsky@chromium.org \
    --cc=tglx@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®