Re: [PATCH printk v2] printk/nbcon: WARN on unsafe reentrance

[Petr Mladek <pmladek@suse.com>]

On Fri 2026-07-31 16:45:46, John Ogness wrote:
> Since the nbcon unsafe enter/exit functions are simply toggling a
> state boolean, a buggy nbcon driver might enter an unsafe section
> when the context is already in an unsafe section and it would go
> unnoticed, even though doing so is a bug. Unsafe sections are not
> reentrant!
> 
> Add a WARN_ON_ONCE() to nbcon_enter_unsafe() if the context is
> already in an unsafe section.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Link: https://lore.kernel.org/lkml/87o6fwms02.fsf@jogness.linutronix.de
> ---
>  Changes since v1:
> 
>  - Move the WARN to after the can_proceed() check. Otherwise
>    cur.unsafe could be referring to a different owning context.
> 
>  kernel/printk/nbcon.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 4b03b019cd5ee..e88ddd742d5a0 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -848,6 +848,9 @@ static bool __nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsaf
>  		if (!nbcon_context_can_proceed(ctxt, &cur))
>  			return false;
>  
> +		/* Unsafe sections are not reentrant. */
> +		WARN_ON_ONCE(unsafe && cur.unsafe);

Sashiko AI has some good points:

| Will this trigger a spurious warning during a panic if the console was
| acquired via an unsafe hostile takeover?
| 
| During a panic, if nbcon_context_try_acquire_hostile() acquires an
| interrupted console, it preserves the unsafe state (cur.unsafe = true and
| cur.unsafe_takeover = true).
| 
| When nbcon_emit_next_record() then processes the next record, it calls
| nbcon_context_enter_unsafe(), which invokes __nbcon_context_update_unsafe()
| with unsafe = true. Since cur.unsafe remains true from the hostile takeover,
| this condition evaluates to true, potentially polluting the panic log with a
| spurious stack trace.

IMHO, it has a point. A solution might be to print the warning only
when there was no unsafe_takeover, e.g.

	/*
	 * Unsafe sections are not reentrant except when an unsafe_takeover
	 * already happened.
	 */
	if (!cur.unsafe_takeover)
		WARN_ON_ONCE(unsafe && cur.unsafe);


| Does this also trigger a spurious warning when KDB outputs to an nbcon
| console?
| 
| Looking at kdb_msg_write(), it calls nbcon_kdb_try_acquire() which
| explicitly puts the console into the unsafe state:
| 
| kernel/printk/nbcon.c:nbcon_kdb_try_acquire() {
| ...
|         if (!nbcon_context_enter_unsafe(ctxt))
|                 return false;
| ...
| }
| 
| After doing so, KDB invokes the driver's write_atomic callback directly:
| 
| kernel/debug/kdb/kdb_io.c:kdb_msg_write() {
| ...
|                 c->write_atomic(c, &wctxt);
| ...
| }
| 
| According to the API, driver write_atomic() implementations must call
| nbcon_enter_unsafe() at the beginning of their execution. Since KDB's
| wrapper already forced the unsafe state, the driver's subsequent call
| evaluates unsafe == true and cur.unsafe == true, firing this warning
| on every KDB output.

This concern looks valid as well. IMHO, the right fix is that
nbcon_kdb_try_acquire() should not call nbcon_context_enter_unsafe().

IMHO, we called nbcon_context_enter_unsafe() in nbcon_kdb_try_acquire()
because of nbcon_write_context_set_buf(). We did not want to call
nbcon_context_enter_unsafe() in kdb_msg_write() because
it would require to access the private wctxt.ctxt.

But nbcon_write_context_set_buf() can be called even when the safe
takeover is allowed. It is done this way even in nbcon_emit_next_record().

So, I think that we could do something like:

diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index c399f11740ef..51d4b573b44d 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -604,8 +604,8 @@ static void kdb_msg_write(const char *msg, int msg_len)
 				continue;
 
 			nbcon_write_context_set_buf(&wctxt, (char *)msg, msg_len);
-
 			c->write_atomic(c, &wctxt);
+
 			nbcon_kdb_release(&wctxt);
 		} else {
 			/*
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index e88ddd742d5a..a82a5cebb469 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1944,8 +1944,7 @@ void nbcon_device_release(struct console *con)
 EXPORT_SYMBOL_GPL(nbcon_device_release);
 
 /**
- * nbcon_kdb_try_acquire - Try to acquire nbcon console and enter unsafe
- *			   section
+ * nbcon_kdb_try_acquire - Try to acquire nbcon console
  * @con:	The nbcon console to acquire
  * @wctxt:	The nbcon write context to be used on success
  *
@@ -1959,8 +1958,7 @@ EXPORT_SYMBOL_GPL(nbcon_device_release);
  * storing them into the ring buffer. It has to acquire the console
  * ownership so that it could call con->write_atomic() callback a safe way.
  *
- * This function acquires the nbcon console using priority NBCON_PRIO_EMERGENCY
- * and marks it unsafe for handover/takeover.
+ * This function acquires the nbcon console using priority NBCON_PRIO_EMERGENCY.
  */
 bool nbcon_kdb_try_acquire(struct console *con,
 			   struct nbcon_write_context *wctxt)
@@ -1974,14 +1972,11 @@ bool nbcon_kdb_try_acquire(struct console *con,
 	if (!nbcon_context_try_acquire(ctxt, false))
 		return false;
 
-	if (!nbcon_context_enter_unsafe(ctxt))
-		return false;
-
 	return true;
 }
 
 /**
- * nbcon_kdb_release - Exit unsafe section and release the nbcon console
+ * nbcon_kdb_release - Release the nbcon console
  *
  * @wctxt:	The nbcon write context initialized by a successful
  *		nbcon_kdb_try_acquire()
@@ -1990,9 +1985,6 @@ void nbcon_kdb_release(struct nbcon_write_context *wctxt)
 {
 	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
 
-	if (!nbcon_context_exit_unsafe(ctxt))
-		return;
-
 	nbcon_context_release(ctxt);
 
 	/*


>  		new.atom = cur.atom;
>  		new.unsafe = unsafe;
>  	} while (!nbcon_state_try_cmpxchg(con, &cur, &new));


Finally, we might want to do the check symmetric. I mean
that also nbcon_context_exit_unsafe() should not be called twice.
I mean something like:

	/*
	 * Unsafe sections are not reentrant except when an unsafe_takeover
	 * already happened.
	 *
	 * This check is valid only when "cur" contains the state when this
	 * context still owned the console, aka nbcon_context_can_proceed()
	 * succeeded.
	 */
	if (!cur.unsafe_takeover)
		WARN_ON_ONCE(unsafe == cur.unsafe);

But honestly, I haven't checked all callers. It is possible that some
code calls exit_unsafe() twice, like the kdb_msg_write() called
enter_unsafe() twice.

Best Regards,
Petr