Re: [PATCH printk v2] printk: Postpone kthread startup

[Petr Mladek <pmladek@suse.com>]

On Fri 2026-10-16 15:00:54, John Ogness wrote:
> 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.

Heh, I have missed this recursion. It is obviously problematic.

> > 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:

Breaking the regression is an obvious solution.

Alternative solution might be to split printk_kthreads_check_locked()
into two similar functions. One for creating kthreads and 2nd for
removing them. We need to combine creating+removing only when
a kthread can't be created. The removal part should not need
recursion in principle. But it might require duplication of
the various checks.

Your proposal looks easier.

> 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. */

It might make sense to warn about the potential recursion. Otherwise,
someone might try to optimize it back. And the problem is far from
obvious. We should not rely on Sashiko...

Something like:

	/*
	 * Changed console list, may require printer threads to	start/stop.
	 * Do this outside of unregister_console_locked() to avoid regression.
	 */

Or we could add a comment above printk_kthreads_check_locked(). But it
would be easier to overlook.

> +	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. */

Same here.

> +		printk_kthreads_check_locked();
>  	}
>  	console_list_unlock();
> 
> I can submit an official patch after the 7.4 merge window.

That would be great. Thanks in advance.

Best Regards,
Petr