linux-kernel - Re: [PATCH printk v2 08/12] printk: add pr

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, 08 Apr 2022 21:03:03 +0206
From:   John Ogness <john.ogness@...utronix.de>
To:     Petr Mladek <pmladek@...e.com>
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 08/12] printk: add pr_flush()

On 2022-04-06, Petr Mladek <pmladek@...e.com> wrote:
>> @@ -3321,6 +3331,79 @@ static int __init printk_late_init(void)
>>  late_initcall(printk_late_init);
>>  
>>  #if defined CONFIG_PRINTK
>> +/* If @con is specified, only wait for that console. Otherwise wait for all. */
>> +static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress)
>> +{
>> +	int remaining = timeout_ms;
>> +	struct console *c;
>> +	u64 last_diff = 0;
>> +	u64 printk_seq;
>> +	u64 diff;
>> +	u64 seq;
>> +
>> +	might_sleep();
>> +
>> +	seq = prb_next_seq(prb);
>
> I suggest to add here:
>
> 	/*
> 	 * Try to flush the messages when kthreads are not available
> 	 * and there is not other console lock owner.
> 	 */
> 	if (console_trylock())
> 		console_unlock()

At this point in the series, we do not have kthreads yet. ;-)

And besides, you are implying that there are messages in the ringbuffer,
but _nobody_ is printing them. That would be a bug in our
implementation.

>> +
>> +	for (;;) {
>> +		diff = 0;
>> +
>> +		console_lock();
>> +		for_each_console(c) {
>> +			if (con && con != c)
>> +				continue;
>> +			if (!console_is_usable(c))
>> +				continue;
>> +			printk_seq = c->seq;
>> +			if (printk_seq < seq)
>> +				diff += seq - printk_seq;
>> +		}
>> +		console_unlock();
>
> This is a bit sub-optimal when the kthreads are not available or
> are disabled. In this case, the messages are flushed [*] by
> the console_unlock() and the diff is outdated.

The messages must be flushed by another context. It is wrong to rely on
this context to do the flushing. In direct mode, those messages should
have been flushed before the above console_lock() succeeded. Otherwise
it is a bug. (For threaded mode, console_unlock() does not flush
anyway.)

> Well, it is ugly to keep it this way.

If you don't mind, I will not change this function for v3. I do not want
to start sprinkling console_trylock()/console_unlock() calls all over
the code to cover up possible bugs and races in the implementation.

John