Re: [PATCH printk v1 1/1] printk: nbcon: Allow unsafe write_atomic() for panic

[Petr Mladek <pmladek@suse.com>]

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@linutronix.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