linux-kernel - Re: [PATCH printk v2 14/26] printk: nbcon: Provide function to flush using write

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, 23 Feb 2024 16:47:53 +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
Subject: Re: [PATCH printk v2 14/26] printk: nbcon: Provide function to flush
 using write_atomic()

On Sun 2024-02-18 20:03:14, John Ogness wrote:
> From: Thomas Gleixner <tglx@...utronix.de>
> 
> Provide nbcon_atomic_flush_all() to perform flushing of all
> registered nbcon consoles using their write_atomic() callback.
> Like with legacy consoles, the nbcon consoles are flushed one
> record per console. This allows all nbcon consoles to print
> lines pseudo-simultaneously, rather than one console waiting
> for the full ringbuffer to dump to another console before
> printing anything.
> 
> Unlike console_flush_all(), nbcon_atomic_flush_all() will only
> flush up through the newest record at the time of the call.
> This prevents a CPU from printing unbounded when other CPUs are
> adding records.

I think about using slightly different name to make the difference
more clear, for example nbcon_atomic_flush_pending() or so.

But I do not have a strong opinion.

> Perform nbcon console atomic flushing in
> console_flush_on_panic(). This function is not only used in
> panic() but also other locations where there may be stored
> messages that need to be flushed.

The above paragraph is a bit misleading. console_flush_on_panic()
is used only in panic(). I guess that you wanted to say something
like:

<proposal>
nbcon_atomic_flush_all() is safe in any context because it uses
write_atomic() and unsafe_takeover is disabled.

Use it in console_flush_on_panic() before flushing legacy consoles.
The legacy write() callbacks are not fully safe when oops_in_progress
is set.
</proposal>

> Co-developed-by: John Ogness <john.ogness@...utronix.de>
> Signed-off-by: John Ogness <john.ogness@...utronix.de>
> Signed-off-by: Thomas Gleixner (Intel) <tglx@...utronix.de>

The code looks good. After updating the commit message,
and eventually the function name, feel free to use:

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

Best Regards,
Petr