linux-kernel - Re: [PATCH printk v2 37/38] printk: relieve console

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:   Thu, 27 Oct 2022 14:40:22 +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 37/38] printk: relieve console_lock of list
 synchronization duties

On Wed 2022-10-19 17:01:59, John Ogness wrote:
> The console_list_lock provides synchronization for console list and
> console->flags updates. All call sites that were using the console_lock
> for this synchronization have either switched to use the
> console_list_lock or the SRCU list iterator.
> 
> Remove console_lock usage for console list updates and console->flags
> updates.
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 3615ee6d68fd..840d581c4b23 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3370,8 +3363,6 @@ static int unregister_console_locked(struct console *console)
>  	if (res > 0)
>  		return 0;
>  
> -	console_lock();
> -
>  	/* Disable it unconditionally */
>  	console->flags &= ~CON_ENABLED;
>  
> @@ -3394,8 +3385,6 @@ static int unregister_console_locked(struct console *console)
>  	if (!hlist_empty(&console_list) && console->flags & CON_CONSDEV)
>  		console_first()->flags |= CON_CONSDEV;
>  
> -	console_unlock();
> -
>  	/* Ensure that all SRCU list walks have completed */
>  	synchronize_srcu(&console_srcu);

In addition, in unregister_console_locked(), we need to remove goto out_unlock:

	if (hlist_unhashed(&console->node)) {
		res = -ENODEV;
		goto out_unlock;
	}

out_unlock:
	console_unlock();
	return res;


I did my best to check all users that either iterate over the list
and/or access console->flags. And I did not find any real problems.

With the removal of the above mentioned out_unlock:

Reviewed-by: Petr Mladek <pmladek@...e.com>


Best Regards,
Petr

PS: Two ideas for possible future improvement:

  1. It might make sense to synchronize also @console_cmdline using
     the console_list_lock. It is modified only in
     __add_preferred_console() and read only from register_console().

     It would remove another possible race. Well, it is probably hard
     to hit, especially when new entries are always added at the end.


  2. It might make sense to create an API to modify and check
     console->flags. It might print a warning when the flags
     are accessed without console_list_lock or console_srcu.

     These days, it is pretty hard to catch all users.