Re: [RFC PATCH v1] panic: Flush unsafe consoles before panic reboot

[Petr Mladek <pmladek@suse.com>]

On Fri 2026-10-30 10:56:35, Breno Leitao wrote:
> On Thu, Oct 29, 2026 at 03:37:03PM +0100, Petr Mladek wrote:
> > Another question is where to call it. I guess the it should be called
> > in two code paths:
> > 
> >    + __crash_kexec() likely after stopping other CPUs
> > 
> >     + console_flush_on_panic() after flushing safe nbcon consoles
> > 	the safe way
> 
> What about something like:
> 
> 
> commit de57403441b601af01d92bc03bf0625338d39c80
> Author: Breno Leitao <leitao@debian.org>
> Date:   Fri Oct 30 10:08:01 2026 -0700
> 
>     panic: flush unsafe consoles before reboot and kdump
>     
>     netconsole registers with CON_NBCON_ATOMIC_UNSAFE since commit
>     7eab73b18630 ("netconsole: convert to NBCON console infrastructure"), so
>     its write_atomic() only runs from nbcon_atomic_flush_unsafe(). vpanic()
>     calls that right before its final busy loop, after emergency_restart()
>     and after __crash_kexec().
>     
>     A panic that reboots (panic_timeout != 0) or boots a crash kernel
>     therefore never reaches the netconsole target. Neither does an oops that
>     goes to kdump through crash_kexec(), which skips vpanic() entirely.
>     Before the conversion, netconsole was a legacy console, flushed by
>     console_flush_on_panic() ahead of the reboot.
>     
>     Add nbcon_atomic_unsafe_only_flush(), which flushes only the
>     CON_NBCON_ATOMIC_UNSAFE consoles and allows unsafe takeovers on them.
>     Call it on the panic CPU once the other CPUs are stopped, leaving the
>     consoles with a safe write_atomic() alone.
>     
>     In vpanic() it runs after the safe flush, when
>     panic_other_cpus_shutdown() has stopped the other CPUs. In
>     __crash_kexec() it runs after crash_smp_send_stop() and before
>     machine_crash_shutdown(), the order panic() already uses with
>     crash_kexec_post_notifiers.
>     
>     The call does not go into console_flush_on_panic(), because kgdb_panic()
>     calls that before the other CPUs are stopped. It does not go after
>     machine_crash_shutdown() either: on TDX guests tdx_kexec_finish() zaps
>     the shared PTEs there, and netconsole on a virtio NIC would touch that
>     memory.

This is the 1st time I hear about it. Does it mean that netconsole
should not be called after this point, please?

I wonder how this worked before the netconsole was converted to
the NBCON API. I guess that it "worked" because there was neither
printk() nor any explicit console flush after this point.

>     A write_atomic() that hangs now keeps the machine from rebooting or from
>     reaching the crash kernel.
>     
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -140,6 +140,13 @@ void __noclone __crash_kexec(struct pt_regs *regs)
>  
>  			crash_setup_regs(&fixed_regs, regs);
>  			crash_save_vmcoreinfo();
> +			/*
> +			 * Flush the unsafe consoles with the other CPUs
> +			 * stopped, before machine_crash_shutdown() tears
> +			 * down what they may need.
> +			 */
> +			crash_smp_send_stop();

AFAIK, the crashdump currently shows all CPUs online. And
the backtrace shows whatever they were doing last.

Stopping the CPUs earlier would change the existing behavior.
And we would need to discuss this with crashdump maintainers.

Well, I know that stopping other CPUs makes it more safe.
But is it strictly necessary? We did not stop the CPUs
when the netconsole was handled by the legacy loop.

> +			nbcon_atomic_unsafe_only_flush();
>  			machine_crash_shutdown(&fixed_regs);
>  			crash_cma_clear_pending_dma();
>  			machine_kexec(kexec_crash_image);
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 91e48d33b7dbbd..e14b1b60828130 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -723,6 +723,9 @@ void vpanic(const char *fmt, va_list args)
>  		panic_console_replay)
>  		console_flush_on_panic(CONSOLE_REPLAY_ALL);
>  
> +	/* The other CPUs are stopped, flush the unsafe consoles too.
> h /tmp/puf_post.sh kerneltest001.31.atn1 6791 atn1a &); (sh
> /tmp/puf_post.sh kerneltest005.19.atn2 6792 atn2a &)

The last two lines are some mess ;-)

> /
> +	nbcon_atomic_unsafe_only_flush();

This might be too early. Anyway, it is before printing the line:

		pr_emerg("Rebooting in %d seconds..\n", panic_timeout);

I have a crazy idea and do this:

--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1688,7 +1688,20 @@ static void __nbcon_atomic_flush_pending(u64 stop_seq)
  */
 void nbcon_atomic_flush_pending(void)
 {
-	__nbcon_atomic_flush_pending(prb_next_reserve_seq(prb));
+	u64 stop_seq = prb_next_reserve_seq(prb);
+
+	__nbcon_atomic_flush_pending(stop_seq, false);
+
+	/*
+	 * This is a compromise. Otherwise, NBCON consoles would never
+	 * have a chance to show messages in the panic paths where
+	 * the unsafe flush is not explicitely called.
+	 */
+	if (panic_on_this_cpu() && oops_in_progress) {
+		panic_nbcon_allow_unsafe_takeover = true;
+		__nbcon_atomic_flush_pending(stop_seq, true);
+		panic_nbcon_allow_unsafe_takeover = false;
+	}
 }

It would allow to flush the messages directly by printk().

I know that it would be a step back in the reliability. But showing
the messages looks more important to me than a reliable reboot.
And we could make it configurable when needed.

Also we could add more checks. For example, do the unsafe flush only
when "num_online_cpus == 1" or so.

> +
>  	if (!panic_blink)
>  		panic_blink = no_blink;
>  
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 5f8afaca886650..a43143eca1a015 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1653,8 +1653,9 @@ static void nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq)
>   * __nbcon_atomic_flush_pending - Flush all nbcon consoles using their
>   *					write_atomic() callback
>   * @stop_seq:			Flush up until this record
> + * @unsafe_only:		Flush only the CON_NBCON_ATOMIC_UNSAFE consoles
>   */
> -static void __nbcon_atomic_flush_pending(u64 stop_seq)
> +static void __nbcon_atomic_flush_pending(u64 stop_seq, bool unsafe_only)
>  {
>  	struct console *con;
>  	int cookie;
> @@ -1666,6 +1667,9 @@ static void __nbcon_atomic_flush_pending(u64 stop_seq)
>  		if (!(flags & CON_NBCON))
>  			continue;
>  
> +		if (unsafe_only && !(flags & CON_NBCON_ATOMIC_UNSAFE))
> +			continue;
> +
>  		if (!console_is_usable(con, flags, true))
>  			continue;
>  

This looks good to me.

> @@ -1701,7 +1705,21 @@ void nbcon_atomic_flush_pending(void)
>  void nbcon_atomic_flush_unsafe(void)
>  {
>  	panic_nbcon_allow_unsafe_takeover = true;
> -	__nbcon_atomic_flush_pending(prb_next_reserve_seq(prb));
> +	__nbcon_atomic_flush_pending(prb_next_reserve_seq(prb), false);
> +	panic_nbcon_allow_unsafe_takeover = false;
> +}
> +
> +/**
> + * nbcon_atomic_unsafe_only_flush - Flush only the CON_NBCON_ATOMIC_UNSAFE
> + *	consoles, allowing unsafe hostile takeovers
> + *
> + * Like nbcon_atomic_flush_unsafe(), but consoles with a safe write_atomic()
> + * are left alone. Meant for the panic CPU once the other CPUs are stopped.
> + */
> +void nbcon_atomic_unsafe_only_flush(void)
> +{
> +	panic_nbcon_allow_unsafe_takeover = true;
> +	__nbcon_atomic_flush_pending(prb_next_reserve_seq(prb), true);
>  	panic_nbcon_allow_unsafe_takeover = false;
>  }

Also the name nbcon_atomic_unsafe_only_flush() sounds good to me.

Best Regards,
Petr