KDB can interrupt any console to execute the "mirrored printing" at any
time, so add an exception to nbcon_context_try_acquire_direct to allow
to get the context if the current CPU is the same as kdb_printf_cpu.
This change will be necessary for the next patch, which fixes
kdb_msg_write to work with NBCON consoles by calling ->write_atomic on
such consoles. But to print it first needs to acquire the ownership of
the console, so nbcon_context_try_acquire_direct is fixed here.
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
kernel/printk/nbcon.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index ff218e95a505fd10521c2c4dfb00ad5ec5773953..352235a0eb4a484caccf86d3a57d1a149218ecec 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -10,6 +10,7 @@
#include <linux/export.h>
#include <linux/init.h>
#include <linux/irqflags.h>
+#include <linux/kdb.h>
#include <linux/kthread.h>
#include <linux/minmax.h>
#include <linux/percpu.h>
@@ -247,6 +248,8 @@ static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
* Panic does not imply that the console is owned. However,
* since all non-panic CPUs are stopped during panic(), it
* is safer to have them avoid gaining console ownership.
+ * The only exception is if kdb is active, which may print
+ * from multiple CPUs during a panic.
*
* If this acquire is a reacquire (and an unsafe takeover
* has not previously occurred) then it is allowed to attempt
@@ -255,6 +258,7 @@ static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
* interrupted by the panic CPU while printing.
*/
if (other_cpu_in_panic() &&
+ READ_ONCE(kdb_printf_cpu) != raw_smp_processor_id() &&
(!is_reacquire || cur->unsafe_takeover)) {
return -EPERM;
}
--
2.50.0On Tue 2025-09-02 15:33:54, Marcos Paulo de Souza wrote: > KDB can interrupt any console to execute the "mirrored printing" at any > time, so add an exception to nbcon_context_try_acquire_direct to allow > to get the context if the current CPU is the same as kdb_printf_cpu. > > This change will be necessary for the next patch, which fixes > kdb_msg_write to work with NBCON consoles by calling ->write_atomic on > such consoles. But to print it first needs to acquire the ownership of > the console, so nbcon_context_try_acquire_direct is fixed here. > > --- a/kernel/printk/nbcon.c > +++ b/kernel/printk/nbcon.c > @@ -247,6 +248,8 @@ static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt, > * Panic does not imply that the console is owned. However, > * since all non-panic CPUs are stopped during panic(), it > * is safer to have them avoid gaining console ownership. > + * The only exception is if kdb is active, which may print > + * from multiple CPUs during a panic. Strictly speaking this is not the only exception. The reacquire is another one. I would put this into a separate paragraph and write: * One exception is when kdb is active, which may print * from multiple CPUs during a panic. > * If this acquire is a reacquire (and an unsafe takeover And here start the paragrah with * Second exception is a reacquire (and an usafe ... > * has not previously occurred) then it is allowed to attempt Best Regards, Petr
On 2025-09-02, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index ff218e95a505fd10521c2c4dfb00ad5ec5773953..352235a0eb4a484caccf86d3a57d1a149218ecec 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -255,6 +258,7 @@ static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
> * interrupted by the panic CPU while printing.
> */
> if (other_cpu_in_panic() &&
> + READ_ONCE(kdb_printf_cpu) != raw_smp_processor_id() &&
This needs some sort of ifdef CONFIG_KGDB_KDB wrapped around it. Maybe
it would be cleaner to have some macro.
#ifdef CONFIG_KGDB_KDB
#define KGDB_IS_ACTIVE() (READ_ONCE(kdb_printf_cpu) == raw_smp_processor_id())
#else
#define KGDB_IS_ACTIVE() false
#endif
and then something like:
if (other_cpu_in_panic() &&
!KGDB_IS_ACTIVE() &&
(!is_reacquire || cur->unsafe_takeover)) {
return -EPERM;
}
Or however you prefer. We need to compile for !CONFIG_KGDB_KDB.
John
On Fri 2025-09-05 16:58:34, John Ogness wrote:
> On 2025-09-02, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
> > diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> > index ff218e95a505fd10521c2c4dfb00ad5ec5773953..352235a0eb4a484caccf86d3a57d1a149218ecec 100644
> > --- a/kernel/printk/nbcon.c
> > +++ b/kernel/printk/nbcon.c
> > @@ -255,6 +258,7 @@ static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
> > * interrupted by the panic CPU while printing.
> > */
> > if (other_cpu_in_panic() &&
> > + READ_ONCE(kdb_printf_cpu) != raw_smp_processor_id() &&
>
> This needs some sort of ifdef CONFIG_KGDB_KDB wrapped around it. Maybe
> it would be cleaner to have some macro.
Great catch!
> #ifdef CONFIG_KGDB_KDB
> #define KGDB_IS_ACTIVE() (READ_ONCE(kdb_printf_cpu) == raw_smp_processor_id())
> #else
> #define KGDB_IS_ACTIVE() false
> #endif
I like this. It would fit into include/linux/kdb.h which already
contains the #ifdef CONFIG_KGDB_KDB / #else / #endif sections.
> and then something like:
>
> if (other_cpu_in_panic() &&
> !KGDB_IS_ACTIVE() &&
> (!is_reacquire || cur->unsafe_takeover)) {
> return -EPERM;
> }
>
> Or however you prefer. We need to compile for !CONFIG_KGDB_KDB.
Best Regards,
Petr
On 2025-09-08, Petr Mladek <pmladek@suse.com> wrote: > On Fri 2025-09-05 16:58:34, John Ogness wrote: >> On 2025-09-02, Marcos Paulo de Souza <mpdesouza@suse.com> wrote: >> > diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c >> > index ff218e95a505fd10521c2c4dfb00ad5ec5773953..352235a0eb4a484caccf86d3a57d1a149218ecec 100644 >> > --- a/kernel/printk/nbcon.c >> > +++ b/kernel/printk/nbcon.c >> > @@ -255,6 +258,7 @@ static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt, >> > * interrupted by the panic CPU while printing. >> > */ >> > if (other_cpu_in_panic() && >> > + READ_ONCE(kdb_printf_cpu) != raw_smp_processor_id() && >> >> This needs some sort of ifdef CONFIG_KGDB_KDB wrapped around it. Maybe >> it would be cleaner to have some macro. > > Great catch! > >> #ifdef CONFIG_KGDB_KDB >> #define KGDB_IS_ACTIVE() (READ_ONCE(kdb_printf_cpu) == raw_smp_processor_id()) >> #else >> #define KGDB_IS_ACTIVE() false >> #endif > > I like this. It would fit into include/linux/kdb.h which already > contains the #ifdef CONFIG_KGDB_KDB / #else / #endif sections. BTW, if there is such a macro created, it should be KDB_IS_ACTIVE() rather than KGDB_IS_ACTIVE(). John
On Fri, 2025-09-05 at 16:58 +0206, John Ogness wrote:
> On 2025-09-02, Marcos Paulo de Souza <mpdesouza@suse.com> wrote:
> > diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> > index
> > ff218e95a505fd10521c2c4dfb00ad5ec5773953..352235a0eb4a484caccf86d3a
> > 57d1a149218ecec 100644
> > --- a/kernel/printk/nbcon.c
> > +++ b/kernel/printk/nbcon.c
> > @@ -255,6 +258,7 @@ static int
> > nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
> > * interrupted by the panic CPU while printing.
> > */
> > if (other_cpu_in_panic() &&
> > + READ_ONCE(kdb_printf_cpu) !=
> > raw_smp_processor_id() &&
>
> This needs some sort of ifdef CONFIG_KGDB_KDB wrapped around it.
> Maybe
> it would be cleaner to have some macro.
Ouch.. that's true!
>
> #ifdef CONFIG_KGDB_KDB
> #define KGDB_IS_ACTIVE() (READ_ONCE(kdb_printf_cpu) ==
> raw_smp_processor_id())
> #else
> #define KGDB_IS_ACTIVE() false
> #endif
>
> and then something like:
>
> if (other_cpu_in_panic() &&
> !KGDB_IS_ACTIVE() &&
> (!is_reacquire || cur->unsafe_takeover)) {
> return -EPERM;
> }
>
> Or however you prefer. We need to compile for !CONFIG_KGDB_KDB.
I like the idea! Let me prepare it for v4.
>
> John
© 2016 - 2025 Red Hat, Inc.