linux-kernel - Re: [PATCH printk v2 09/18] printk: nbcon: Start printing threads

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZnGo-7ctV9oidQM8@pathway.suse.cz>
Date: Tue, 18 Jun 2024 17:34:19 +0200
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH printk v2 09/18] printk: nbcon: Start printing threads

On Tue 2024-06-04 01:30:44, John Ogness wrote:
> If there are no boot consoles, the printing threads are started
> in early_initcall.
> 
> If there are boot consoles, the printing threads are started
> after the last boot console has unregistered. The printing
> threads do not need to be concerned about boot consoles because
> boot consoles cannot register once a non-boot console has
> registered.
> 
> Until a printing thread of a console has started, that console
> will print using atomic_write() in the printk() caller context.
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1568,6 +1570,19 @@ void nbcon_kthread_create(struct console *con)
>  	sched_set_normal(con->kthread, -20);
>  }
>  
> +static int __init printk_setup_threads(void)
> +{
> +	struct console *con;
> +
> +	console_list_lock();
> +	printk_threads_enabled = true;

What is the actual meaning of the variable?

Does it mean that kthreads can be created? (can be forked?)

It does not guarantee that the kthreads will be running.
They might still get blocked by a boot console.


> +	for_each_console(con)
> +		nbcon_kthread_create(con);
> +	console_list_unlock();
> +	return 0;
> +}
> +early_initcall(printk_setup_threads);
> +
>  /**
>   * nbcon_alloc - Allocate buffers needed by the nbcon console
>   * @con:	Console to allocate buffers for
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2397,6 +2400,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  		 * consoles cannot print simultaneously with boot consoles.
>  		 */
>  		if (is_panic_context ||
> +		    !printk_threads_enabled ||
>  		    (system_state > SYSTEM_RUNNING)) {
>  			nbcon_atomic_flush_pending();

IMHO, this is not safe. nbcon_atomic_flush_pending()
is synchronized only via the nbcon console context.
It means that there might be races with boot consoles.

Another problem is the meaning of @printk_threads_enabled variable.
It does not guarantee that the kthreads are running. They might
still be blocked by boot consoles.


BTW: The same problem might have the system_state > SYSTEM_RUNNING.
     The boot consoles are removed only when the preferred console
     is registered and "keep_bootcon" is not set.

     Note that printk_late_init() unregisters the boot consoles
     only when they are using a code in the init sections.

>  		}

My view:

We need to enable creating the kthreads when:

  1. the "kthreadd_task" is running. It is a kthread responsible for
     forking new kthreads. And it is started after the init task.

  2. There is no boot console.

Plus we could use con->write_atomic() only under console_lock()
when there is still a boot console.

Best Regards,
Petr