linux-kernel - Re: [PATCH 4.19 regression fix] printk: For early boot messages check loglevel when flushing the buffer

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:   Wed, 12 Sep 2018 15:33:47 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Hans de Goede <hdegoede@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        x86@...nel.org, linux-kernel@...r.kernel.org,
        Steven Rostedt <rostedt@...dmis.org>,
        Maninder Singh <maninder1.s@...sung.com>
Subject: Re: [PATCH 4.19 regression fix] printk: For early boot messages
 check loglevel when flushing the buffer

On Wed 2018-09-12 16:49:30, Sergey Senozhatsky wrote:
> On (09/11/18 10:47), Petr Mladek wrote:
> > > > Most problems should probably be solved when we store console_seq
> > > > before setting exclusive_console. Then we could clear
> > > > exclusive_console when reaching the stored sequence number.
> 
> Hmm, wouldn't the same "it might be problematic with slow consoles"
> argument apply to this solution as well?

It would just fix a clear bug.

> So maybe we can do the following:
> 
> - store console_seq when we register exclusive console
> - in flush_on_panic, if we have exclusive console set
>     - clear exclusive console
>     - rollback console_seq to the value it had before we set exclusive console
 
> ---
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 53c94cbce0af..4ef199572df7 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -424,6 +424,7 @@ static u32 log_next_idx;
>  /* the next printk record to write to the console */
>  static u64 console_seq;
>  static u32 console_idx;
> +static u64 rollback_console_seq;
>  
>  /* the next printk record to read after the last 'clear' command */
>  static u64 clear_seq;
> @@ -2592,6 +2593,10 @@ void console_flush_on_panic(void)
>  	 */
>  	console_trylock();
>  	console_may_schedule = 0;
> +	if (exclusive_console) {
> +		exclusive_console = NULL;
> +		console_seq = rollback_console_seq;

This might be controversial. Yes, it might help in some situations
(slow exclusive console). But it might be counterproductive
when the exclusive console is the one that is readable
after the crash, used for debugging and this change causes
an incomplete log.

I would do this change only when people complains about
the current behavior.

I think that more important is to do:

@@ -2415,6 +2415,9 @@ void console_unlock(void)
                console_seq++;
                raw_spin_unlock(&logbuf_lock);
 
+               if (exclusive_console && console_seq >= rollback_console_seq)
+                       exclusive_console = NULL;
+
                /*
                 * While actively printing out messages, if another printk()
                 * were to occur on another CPU, it may wait for this one to

IMHO, this is a clear win. It fixes a clear mistake.

I would just rename the variable to exclusive_console_seq_stop or so.

Best Regards,
Petr