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]

Message-ID: <Yk2u7dgm2VQk1DGn@alley>
Date:   Wed, 6 Apr 2022 17:17:01 +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 08/12] printk: add pr_flush()

On Tue 2022-04-05 15:31:31, John Ogness wrote:
> Provide a might-sleep function to allow waiting for console printers
> to catch up to the latest logged message.
> 
> Use pr_flush() whenever it is desirable to get buffered messages
> printed before continuing: suspend_console(), resume_console(),
> console_stop(), console_start(), console_unblank().
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 06f0b29d909d..d0369afaf365 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -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 auggest 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()

> +
> +	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.

As a result, the code would non-necessarily sleep 100ms before
it realizes that the job is done.

One might argue that the problem will be only when there are
non-handled messages and in situations when the delay probably
is not critical.

Well, it is ugly to keep it this way. I suggest to add the extra
console_trylock()/unlock() at the beginning.

> +
> +		if (diff != last_diff && reset_on_progress)
> +			remaining = timeout_ms;
> +
> +		if (diff == 0 || remaining == 0)
> +			break;
> +
> +		if (remaining < 0) {
> +			/* no timeout limit */
> +			msleep(100);
> +		} else if (remaining < 100) {
> +			msleep(remaining);
> +			remaining = 0;
> +		} else {
> +			msleep(100);
> +			remaining -= 100;
> +		}
> +
> +		last_diff = diff;
> +	}
> +
> +	return (diff == 0);
> +}

Otherwise, it looks good to me. It is an interesting function.
I wonder if it gets popular also outside printk code. ;-)

Best Regards,
Petr