Function kdb_msg_write was calling con->write for the any found console,
but it won't work on NBCON ones. In this case we should acquire the
ownership of the console using NBCON_PRIO_EMERGENCY, since printing
kdb messages should only be interrupted by a panic.
At this point, the console is required to use the atomic callback. The
console is skipped if the write_atomic callback is not set or if the
context could not be acquired. The validation of NBCON is done by the
console_is_usable helper. The context is released right after
write_atomic finishes.
Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
kernel/debug/kdb/kdb_io.c | 46 ++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 42 insertions(+), 4 deletions(-)
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 9b11b10b120cf07e451a7a4d92ce50f9a6c066b2..3b7365c11d06b01d487767fd89f1081da10dd2ed 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -558,6 +558,25 @@ static int kdb_search_string(char *searched, char *searchfor)
return 0;
}
+static struct nbcon_context *nbcon_acquire_ctxt(struct console *con,
+ struct nbcon_write_context *wctxt,
+ char *msg, int msg_len)
+{
+ struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
+
+ ctxt->console = con;
+ ctxt->spinwait_max_us = 0;
+ ctxt->prio = NBCON_PRIO_EMERGENCY;
+ ctxt->allow_unsafe_takeover = false;
+ wctxt->outbuf = msg;
+ wctxt->len = msg_len;
+
+ if (!nbcon_context_try_acquire(ctxt))
+ return NULL;
+
+ return ctxt;
+}
+
static void kdb_msg_write(const char *msg, int msg_len)
{
struct console *c;
@@ -589,12 +608,26 @@ static void kdb_msg_write(const char *msg, int msg_len)
*/
cookie = console_srcu_read_lock();
for_each_console_srcu(c) {
- if (!(console_srcu_read_flags(c) & CON_ENABLED))
+ struct nbcon_write_context wctxt = { };
+ struct nbcon_context *ctxt;
+ short flags = console_srcu_read_flags(c);
+
+ if (!console_is_usable(c, flags, true))
continue;
if (c == dbg_io_ops->cons)
continue;
- if (!c->write)
- continue;
+
+ /*
+ * Do not continue if the console is NBCON and the context
+ * can't be acquired.
+ */
+ if (flags & CON_NBCON) {
+ ctxt = nbcon_acquire_ctxt(c, &wctxt, (char *)msg,
+ msg_len);
+ if (!ctxt)
+ continue;
+ }
+
/*
* Set oops_in_progress to encourage the console drivers to
* disregard their internal spin locks: in the current calling
@@ -605,7 +638,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
* for this calling context.
*/
++oops_in_progress;
- c->write(c, msg, msg_len);
+ if (flags & CON_NBCON) {
+ c->write_atomic(c, &wctxt);
+ nbcon_context_release(ctxt);
+ } else {
+ c->write(c, msg, msg_len);
+ }
--oops_in_progress;
touch_nmi_watchdog();
}
--
2.50.0On 2025-07-13, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 9b11b10b120cf07e451a7a4d92ce50f9a6c066b2..3b7365c11d06b01d487767fd89f1081da10dd2ed 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -558,6 +558,25 @@ static int kdb_search_string(char *searched, char *searchfor)
> return 0;
> }
>
> +static struct nbcon_context *nbcon_acquire_ctxt(struct console *con,
> + struct nbcon_write_context *wctxt,
> + char *msg, int msg_len)
> +{
> + struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> +
> + ctxt->console = con;
> + ctxt->spinwait_max_us = 0;
> + ctxt->prio = NBCON_PRIO_EMERGENCY;
> + ctxt->allow_unsafe_takeover = false;
> + wctxt->outbuf = msg;
> + wctxt->len = msg_len;
> +
> + if (!nbcon_context_try_acquire(ctxt))
> + return NULL;
> +
> + return ctxt;
This function is grabbing a reference to a private member and returning
it, thus exposing internals. Can we instead create a proper API in
kernel/printk/nbcon.c for kdb?
For example, take a look at:
nbcon_device_try_acquire() and nbcon_device_release()
We could have something similar for kdb, such as:
bool *nbcon_kdb_try_acquire(struct nbcon_write_context *wctxt,
struct console *con, char *msg, int msg_len);
void nbcon_kdb_release(struct nbcon_write_context *wctxt);
> +}
> +
> static void kdb_msg_write(const char *msg, int msg_len)
> {
> struct console *c;
> @@ -589,12 +608,26 @@ static void kdb_msg_write(const char *msg, int msg_len)
> */
> cookie = console_srcu_read_lock();
> for_each_console_srcu(c) {
> - if (!(console_srcu_read_flags(c) & CON_ENABLED))
> + struct nbcon_write_context wctxt = { };
> + struct nbcon_context *ctxt;
With the above suggestion we do not need @ctxt.
> + short flags = console_srcu_read_flags(c);
> +
> + if (!console_is_usable(c, flags, true))
> continue;
> if (c == dbg_io_ops->cons)
> continue;
> - if (!c->write)
> - continue;
> +
> + /*
> + * Do not continue if the console is NBCON and the context
> + * can't be acquired.
> + */
> + if (flags & CON_NBCON) {
> + ctxt = nbcon_acquire_ctxt(c, &wctxt, (char *)msg,
> + msg_len);
> + if (!ctxt)
> + continue;
And this becomes:
if (!nbcon_kdb_try_acquire(&wctxt, c, (char *)msg, msg_len))
continue;
> + }
> +
> /*
> * Set oops_in_progress to encourage the console drivers to
> * disregard their internal spin locks: in the current calling
> @@ -605,7 +638,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
> * for this calling context.
> */
> ++oops_in_progress;
> - c->write(c, msg, msg_len);
> + if (flags & CON_NBCON) {
> + c->write_atomic(c, &wctxt);
> + nbcon_context_release(ctxt);
And this becomes:
nbcon_kdb_release(&wctxt);
> + } else {
> + c->write(c, msg, msg_len);
> + }
> --oops_in_progress;
> touch_nmi_watchdog();
> }
John Ogness
On Sun, 2025-07-13 at 22:42 +0206, John Ogness wrote:
> On 2025-07-13, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
> > diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> > index
> > 9b11b10b120cf07e451a7a4d92ce50f9a6c066b2..3b7365c11d06b01d487767fd8
> > 9f1081da10dd2ed 100644
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -558,6 +558,25 @@ static int kdb_search_string(char *searched,
> > char *searchfor)
> > return 0;
> > }
> >
> > +static struct nbcon_context *nbcon_acquire_ctxt(struct console
> > *con,
> > + struct nbcon_write_context
> > *wctxt,
> > + char *msg, int msg_len)
> > +{
> > + struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> > +
> > + ctxt->console = con;
> > + ctxt->spinwait_max_us = 0;
> > + ctxt->prio = NBCON_PRIO_EMERGENCY;
> > + ctxt->allow_unsafe_takeover = false;
> > + wctxt->outbuf = msg;
> > + wctxt->len = msg_len;
> > +
> > + if (!nbcon_context_try_acquire(ctxt))
> > + return NULL;
> > +
> > + return ctxt;
>
> This function is grabbing a reference to a private member and
> returning
> it, thus exposing internals. Can we instead create a proper API in
> kernel/printk/nbcon.c for kdb?
>
> For example, take a look at:
>
> nbcon_device_try_acquire() and nbcon_device_release()
>
> We could have something similar for kdb, such as:
>
> bool *nbcon_kdb_try_acquire(struct nbcon_write_context *wctxt,
> struct console *con, char *msg, int
> msg_len);
>
> void nbcon_kdb_release(struct nbcon_write_context *wctxt);
Makes sense John! Thanks for the quick review and suggestion! I'll work
on it in the next few days, but also wait for more people to take a
look as well.
>
> > +}
> > +
> > static void kdb_msg_write(const char *msg, int msg_len)
> > {
> > struct console *c;
> > @@ -589,12 +608,26 @@ static void kdb_msg_write(const char *msg,
> > int msg_len)
> > */
> > cookie = console_srcu_read_lock();
> > for_each_console_srcu(c) {
> > - if (!(console_srcu_read_flags(c) & CON_ENABLED))
> > + struct nbcon_write_context wctxt = { };
> > + struct nbcon_context *ctxt;
>
> With the above suggestion we do not need @ctxt.
>
> > + short flags = console_srcu_read_flags(c);
> > +
> > + if (!console_is_usable(c, flags, true))
> > continue;
> > if (c == dbg_io_ops->cons)
> > continue;
> > - if (!c->write)
> > - continue;
> > +
> > + /*
> > + * Do not continue if the console is NBCON and the
> > context
> > + * can't be acquired.
> > + */
> > + if (flags & CON_NBCON) {
> > + ctxt = nbcon_acquire_ctxt(c, &wctxt, (char
> > *)msg,
> > + msg_len);
> > + if (!ctxt)
> > + continue;
>
> And this becomes:
>
> if (!nbcon_kdb_try_acquire(&wctxt, c, (char
> *)msg, msg_len))
> continue;
Agreed.
> > + }
> > +
> > /*
> > * Set oops_in_progress to encourage the console
> > drivers to
> > * disregard their internal spin locks: in the
> > current calling
> > @@ -605,7 +638,12 @@ static void kdb_msg_write(const char *msg, int
> > msg_len)
> > * for this calling context.
> > */
> > ++oops_in_progress;
> > - c->write(c, msg, msg_len);
> > + if (flags & CON_NBCON) {
> > + c->write_atomic(c, &wctxt);
> > + nbcon_context_release(ctxt);
>
> And this becomes:
>
> nbcon_kdb_release(&wctxt);
Much better and cleaner!
>
> > + } else {
> > + c->write(c, msg, msg_len);
> > + }
> > --oops_in_progress;
> > touch_nmi_watchdog();
> > }
>
Thanks for the quick review!
> John Ogness
© 2016 - 2025 Red Hat, Inc.