| 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
| ||
|
Message-ID: <aMl8xX9QCM9jslLH@pathway.suse.cz>
Date: Tue, 16 Sep 2025 17:05:41 +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>,
Breno Leitao <leitao@...ian.org>, Mike Galbraith <efault@....de>,
linux-kernel@...r.kernel.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk v1 1/1] printk: nbcon: Allow unsafe write_atomic()
for panic
On Fri 2025-09-12 14:24:52, John Ogness wrote:
> There may be console drivers that have not yet figured out a way
> to implement safe atomic printing (->write_atomic() callback).
> These drivers could choose to only implement threaded printing
> (->write_thread() callback), but then it is guaranteed that _no_
> output will be printed during panic. Not even attempted.
>
> As a result, developers may be tempted to implement unsafe
> ->write_atomic() callbacks and/or implement some sort of custom
> deferred printing trickery to try to make it work. This goes
> against the principle intention of the nbcon API as well as
> endangers other nbcon drivers that are doing things correctly
> (safely).
>
> As a compromise, allow nbcon drivers to implement unsafe
> ->write_atomic() callbacks by providing a new console flag
> CON_NBCON_ATOMIC_UNSAFE. When specified, the ->write_atomic()
> callback for that console will *only* be called during the
> final "hope and pray" flush attempt at the end of a panic:
> nbcon_atomic_flush_unsafe().
>
> Signed-off-by: John Ogness <john.ogness@...utronix.de>
> Link: https://lore.kernel.org/lkml/b2qps3uywhmjaym4mht2wpxul4yqtuuayeoq4iv4k3zf5wdgh3@tocu6c7mj4lt
> ---
> include/linux/console.h | 3 +++
> kernel/printk/nbcon.c | 17 ++++++++++++++---
> kernel/printk/printk.c | 23 ++++++++++++++++-------
> 3 files changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 8f10d0a85bb4..ec68ecd13f85 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -185,6 +185,8 @@ static inline void con_debug_leave(void) { }
> * printing callbacks must not be called.
> * @CON_NBCON: Console can operate outside of the legacy style console_lock
> * constraints.
> + * @CON_NBCON_ATOMIC_UNSAFE: The write_atomic() callback is not safe and is
> + * therefore only used by nbcon_atomic_flush_unsafe().
> */
> enum cons_flags {
> CON_PRINTBUFFER = BIT(0),
> @@ -196,6 +198,7 @@ enum cons_flags {
> CON_EXTENDED = BIT(6),
> CON_SUSPENDED = BIT(7),
> CON_NBCON = BIT(8),
> + CON_NBCON_ATOMIC_UNSAFE = BIT(9),
> };
>
> /**
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 646801813415..8c2966b85ac3 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -972,14 +972,18 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt, bool use_a
> /*
> * This function should never be called for consoles that have not
> * implemented the necessary callback for writing: i.e. legacy
> - * consoles and, when atomic, nbcon consoles with no write_atomic().
> + * consoles and, when atomic, nbcon consoles with no write_atomic()
> + * or an unsafe write_atomic() without allowing unsafe takeovers.
> * Handle it as if ownership was lost and try to continue.
> *
> * Note that for nbcon consoles the write_thread() callback is
> * mandatory and was already checked in nbcon_alloc().
> */
> - if (WARN_ON_ONCE((use_atomic && !con->write_atomic) ||
> - !(console_srcu_read_flags(con) & CON_NBCON))) {
> + if (WARN_ON_ONCE(!(console_srcu_read_flags(con) & CON_NBCON) ||
> + (use_atomic &&
> + (!con->write_atomic ||
> + (!ctxt->allow_unsafe_takeover &&
> + (console_srcu_read_flags(con) & CON_NBCON_ATOMIC_UNSAFE)))))) {
The condition seems to be correct. But it is evil. I wonder whether
it would make sense to replace this with:
flags = console_srcu_read_flags(con);
if (WARN_ON_ONCE(!(flags & CON_NBCON) ||
!console_is_usable(con, flags, use_atomic, ctxt->allow_unsafe_takeover))) {
Note that I have added the 4th parameter intentionally, see below.
> nbcon_context_release(ctxt);
> return false;
> }
> @@ -1606,6 +1610,13 @@ static void __nbcon_atomic_flush_pending(u64 stop_seq, bool allow_unsafe_takeove
> if (!console_is_usable(con, flags, true))
> continue;
>
> + /*
> + * It is only allowed to use unsafe ->write_atomic() from
> + * nbcon_atomic_flush_unsafe().
> + */
> + if ((flags & CON_NBCON_ATOMIC_UNSAFE) && !allow_unsafe_takeover)
> + continue;
> +
> if (nbcon_seq_read(con) >= stop_seq)
> continue;
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 0efbcdda9aab..1cfc6801eed0 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3206,13 +3206,22 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
> u64 printk_seq;
> bool progress;
>
> - /*
> - * console_flush_all() is only responsible for nbcon
> - * consoles when the nbcon consoles cannot print via
> - * their atomic or threaded flushing.
> - */
> - if ((flags & CON_NBCON) && (ft.nbcon_atomic || ft.nbcon_offload))
> - continue;
> + if (flags & CON_NBCON) {
> + /*
> + * console_flush_all() is only responsible for nbcon
> + * consoles when the nbcon consoles cannot print via
> + * their atomic or threaded flushing.
> + */
> + if (ft.nbcon_atomic || ft.nbcon_offload)
> + continue;
> +
> + /*
> + * It is only allowed to use unsafe ->write_atomic() from
> + * nbcon_atomic_flush_unsafe().
> + */
> + if ((flags & CON_NBCON_ATOMIC_UNSAFE) && !do_cond_resched)
> + continue;
> + }
>
> if (!console_is_usable(con, flags, !do_cond_resched))
> continue;
Adding extra check looks error prone. I think that also
__nbcon_atomic_flush_pending_con() has to be patched.
Otherwise, it might end up in an infinite loop.
It is called directly from nbcon_device_release().
Note that this patch added the check only to the other caller
__nbcon_atomic_flush_pending().
It would be more reliable when the check was integrated into
console_is_usable(). I guess that you did not do it because
it added another parameter...
I would personally prefer to add the 4th parameter.
Or maybe, we could define @allow_unsafe_takeover via a global variable,
e.g. panic_nbcon_allow_unsafe_takeover. And it might be valid
only on the panic CPU, e.g.
static inline
bool nbcon_allow_unsafe_takeover(void)
{
return panic_on_this_cpu() && panic_nbcon_allow_unsafe_takeover;
}
It is a kind of hack. But it might be better than the 4th parameter.
And it would simplify few other APIs.
Best Regards,
Petr
Powered by blists - more mailing lists