* Re: [PATCH printk v2] printk/nbcon: WARN on unsafe reentrance
2026-07-31 14:39 [PATCH printk v2] printk/nbcon: WARN on unsafe reentrance John Ogness
@ 2026-08-19 15:52 ` Petr Mladek
0 siblings, 0 replies; 2+ messages in thread
From: Petr Mladek @ 2026-08-19 15:52 UTC (permalink / raw)
To: John Ogness; +Cc: Sergey Senozhatsky, Steven Rostedt, linux-kernel
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
^ permalink raw reply [flat|nested] 2+ messages in thread