linux-kernel - READ_ONCE: was: Re: [PATCH printk v2 19/26] printk: Avoid 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]

Message-ID: <ZeCueLN0avUV0Yhr@alley>
Date: Thu, 29 Feb 2024 17:19:04 +0100
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,
	"Paul E. McKenney" <paulmck@...nel.org>
Subject: READ_ONCE: was: Re: [PATCH printk v2 19/26] printk: Avoid
 console_lock dance if no legacy or boot consoles

Adding Paul into Cc.

On Sun 2024-02-18 20:03:19, John Ogness wrote:
> Currently the console lock is used to attempt legacy-type
> printing even if there are no legacy or boot consoles registered.
> If no such consoles are registered, the console lock does not
> need to be taken.
> 
> Add tracking of legacy console registration and use it with
> boot console tracking to avoid unnecessary code paths, i.e.
> do not use the console lock if there are no boot consoles
> and no legacy consoles.
> 
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -44,6 +44,16 @@ enum printk_info_flags {
>  };
>  
>  extern struct printk_ringbuffer *prb;
> +extern bool have_legacy_console;
> +extern bool have_boot_console;
> +
> +/*
> + * Specifies if the console lock/unlock dance is needed for console
> + * printing. If @have_boot_console is true, the nbcon consoles will
> + * be printed serially along with the legacy consoles because nbcon
> + * consoles cannot print simultaneously with boot consoles.
> + */
> +#define printing_via_unlock (have_legacy_console || have_boot_console)
>  
>  __printf(4, 0)
>  int vprintk_store(int facility, int level,
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -463,6 +463,13 @@ static int console_msg_format = MSG_FORMAT_DEFAULT;
>  /* syslog_lock protects syslog_* variables and write access to clear_seq. */
>  static DEFINE_MUTEX(syslog_lock);
>  
> +/*
> + * Specifies if a legacy console is registered. If legacy consoles are
> + * present, it is necessary to perform the console_lock/console_unlock dance
> + * whenever console flushing should occur.
> + */
> +bool have_legacy_console;
> +
>  /*
>   * Specifies if a boot console is registered. If boot consoles are present,
>   * nbcon consoles cannot print simultaneously and must be synchronized by
> @@ -3790,22 +3807,28 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
>  	seq = prb_next_reserve_seq(prb);
>  
>  	/* Flush the consoles so that records up to @seq are printed. */
> -	console_lock();
> -	console_unlock();
> +	if (printing_via_unlock) {
> +		console_lock();
> +		console_unlock();
> +	}
>  
>  	for (;;) {
>  		unsigned long begin_jiffies;
>  		unsigned long slept_jiffies;
>  
> +		locked = false;
>  		diff = 0;
>  
> -		/*
> -		 * Hold the console_lock to guarantee safe access to
> -		 * console->seq. Releasing console_lock flushes more
> -		 * records in case @seq is still not printed on all
> -		 * usable consoles.
> -		 */
> -		console_lock();
> +		if (printing_via_unlock) {
> +			/*
> +			 * Hold the console_lock to guarantee safe access to
> +			 * console->seq. Releasing console_lock flushes more
> +			 * records in case @seq is still not printed on all
> +			 * usable consoles.
> +			 */
> +			console_lock();
> +			locked = true;
> +		}
>  
>  		cookie = console_srcu_read_lock();
>  		for_each_console_srcu(c) {
> @@ -3836,7 +3860,8 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
>  		if (diff != last_diff && reset_on_progress)
>  			remaining_jiffies = timeout_jiffies;
>  
> -		console_unlock();
> +		if (locked)
> +			console_unlock();

Is this actually safe?

What prevents the compiler from optimizing out the "locked" variable
and reading "printing_via_unlock" once again here?

It is not exactly the same but it is similar to "invented loads"
described at
https://lwn.net/Articles/793253/#Invented%20Loads

The writes affecting printing_via_unlock are not synchronized
by console_lock().

Should we do the following?

/*
 * Specifies if the console lock/unlock dance is needed for console
 * printing. If @have_boot_console is true, the nbcon consoles will
 * be printed serially along with the legacy consoles because nbcon
 * consoles cannot print simultaneously with boot consoles.
 *
 * Prevent compiler speculations when checking the values.
 */
#define printing_via_unlock (READ_ONCE(have_legacy_console) ||     \
			     READ_ONCE(have_boot_console))


or

		if (printing_via_unlock) {
		[...]
			WRITE_ONCE(locked, true);
		}

Or am I too paranoid?

Best Regards,
Petr