linux-kernel - Re: [PATCH printk v2 20/38] um: kmsg_dumper: use srcu console list iterator

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]

Date:   Fri, 21 Oct 2022 16:56:03 +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, Richard Weinberger <richard@....at>,
        Anton Ivanov <anton.ivanov@...bridgegreys.com>,
        Johannes Berg <johannes@...solutions.net>,
        Thomas Meyer <thomas@...3r.de>, linux-um@...ts.infradead.org
Subject: Re: [PATCH printk v2 20/38] um: kmsg_dumper: use srcu console list
 iterator

On Wed 2022-10-19 17:01:42, John Ogness wrote:
> Rather than using the console_lock to guarantee safe console list
> traversal, use srcu console list iteration.
> 
> Signed-off-by: John Ogness <john.ogness@...utronix.de>
> ---
>  arch/um/kernel/kmsg_dump.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
> index 3a3bbbb22090..44a418dec919 100644
> --- a/arch/um/kernel/kmsg_dump.c
> +++ b/arch/um/kernel/kmsg_dump.c
> @@ -16,20 +16,17 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
>  	struct console *con;
>  	unsigned long flags;
>  	size_t len = 0;
> +	int cookie;
>  
>  	/* only dump kmsg when no console is available */

I agree that it is perfectly fine to use RCU here. The previous
code was just a best effort. The kdump was done without console_lock()
so that the console might get available in the meantime.

I guess that it is not a big deal. The dumper is called typically only
during panic() where new consoles are not added.

> -	if (!console_trylock())
> -		return;
> -
> -	for_each_console(con) {
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(con) {
>  		if (strcmp(con->name, "tty") == 0 &&
>  		    (console_is_enabled(con) || (con->flags & CON_CONSDEV))) {

This is slightly more racy than the previous code. Only one console
could have CON_CONSDEV. It might be moved to another console when the
list is manipulated. As a result, rcu walk might see zero, one, or two
consoles with this flag unless the flag is moved carefully.

Anyway, this check does not match the comment and does not make much
sense to me. It is true that CON_CONSDEV flag is used only for
registered consoles. But messages are printed on the console only
when CON_ENABLED flag is set.

IMHO, we should check only console_is_enabled() here.

Adding Thomas Meyer and Richard Weinberger <richard@....at> into Cc.
Thomas added this check in the commit e23fe90dec286cd77e90
("um: kmsg_dumper: always dump when not tty console") and
Richard pushed it.

>  			break;
>  		}
>  	}
> -
> -	console_unlock();
> -
> +	console_srcu_read_unlock(cookie);
>
>  	if (con)
>  		return;

Best Regards,
Petr