linux-kernel - Re: [PATCH printk v2 05/38] printk: use console_is

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: <Y1JlerStRZ85D4Zo@alley>
Date:   Fri, 21 Oct 2022 11:25:14 +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 05/38] printk: use console_is_enabled()

On Wed 2022-10-19 17:01:27, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@...utronix.de>
> ---
>  kernel/printk/printk.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index e8a56056cd50..7ff2fc75fc3b 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2658,7 +2658,7 @@ static bool abandon_console_lock_in_panic(void)
>   */
>  static inline bool console_is_usable(struct console *con)
>  {
> -	if (!(con->flags & CON_ENABLED))
> +	if (!console_is_enabled(con))
>  		return false;

This allows to use console_is_usable() without synchronization against
modification of the state. I guess that it will be needed for the
printk kthreads. But it is not needed at the moment.

We should document why it is needed and why it is safe.

>  
>  	if (!con->write)
> @@ -2944,7 +2944,7 @@ void console_unblank(void)
>  	console_locked = 1;
>  	console_may_schedule = 0;
>  	for_each_console(c)
> -		if ((c->flags & CON_ENABLED) && c->unblank)
> +		if (console_is_enabled(c) && c->unblank)
>  			c->unblank();
>  	console_unlock();

The reading of the flag is actually synchronized against modifications
here. I guess that we need this because c->unblank() probably is not safe
against other operations with the console, e.g. c->write().

Well, it probably does not matter if reading of CON_ENABLED flag is
serialized against modifications. So, it is perfectly fine to use
the "racy" function.

> @@ -3098,7 +3098,7 @@ static int try_enable_preferred_console(struct console *newcon,
>  	 * without matching. Accept the pre-enabled consoles only when match()
>  	 * and setup() had a chance to be called.
>  	 */
> -	if (newcon->flags & CON_ENABLED && c->user_specified ==	user_specified)
> +	if (console_is_enabled(newcon) && (c->user_specified == user_specified))

This is not racy because newcon is not in console_list at this
point. So that the state can't be modified by another callers.

Anyway, it is not explicitly synchronized, so we need to use
console_is_enabled with data_race() annotation.

>  		return 0;
>  
>  	return -ENOENT;

Best Regards,
Petr