If legacy and nbcon consoles are registered and the nbcon
consoles are allowed to flush (i.e. no boot consoles
registered), the legacy consoles will no longer perform
direct printing on the panic CPU until after the backtrace
has been stored. This will give the safe nbcon consoles a
chance to print the panic messages before allowing the
unsafe legacy consoles to print.

If no nbcon consoles are registered or they are not allowed
to flush, there is no change in behavior (i.e. legacy
consoles will always attempt to print from the printk()
caller context).

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/printk.h   |  5 ++++
 kernel/panic.c           |  2 ++
 kernel/printk/internal.h |  2 ++
 kernel/printk/printk.c   | 54 ++++++++++++++++++++++++++++++++++------
 4 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 2e083f01f8a3..eca9bb2ee637 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -200,6 +200,7 @@ extern asmlinkage void dump_stack_lvl(const char *log_lvl) __cold;
 extern asmlinkage void dump_stack(void) __cold;
 void printk_trigger_flush(void);
 void console_try_replay_all(void);
+void printk_legacy_allow_panic_sync(void);
 extern bool nbcon_device_try_acquire(struct console *con);
 extern void nbcon_device_release(struct console *con);
 void nbcon_atomic_flush_unsafe(void);
@@ -286,6 +287,10 @@ static inline void console_try_replay_all(void)
 {
 }
 
+static inline void printk_legacy_allow_panic_sync(void)
+{
+}
+
 static inline bool nbcon_device_try_acquire(struct console *con)
 {
 	return false;
diff --git a/kernel/panic.c b/kernel/panic.c
index a58631f42e79..84488daa14ce 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -368,6 +368,8 @@ void panic(const char *fmt, ...)
 
 	panic_other_cpus_shutdown(_crash_kexec_post_notifiers);
 
+	printk_legacy_allow_panic_sync();
+
 	/*
 	 * Run any panic handlers, including those that might need to
 	 * add information to the kmsg dump output.
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 7679e18f24b3..dbda90f0dc08 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -130,6 +130,8 @@ static inline bool console_is_usable(struct console *con, short flags)
 #define PRINTK_MESSAGE_MAX	0
 #define PRINTKRB_RECORD_MAX	0
 
+#define legacy_allow_panic_sync	false
+
 /*
  * In !PRINTK builds we still export console_sem
  * semaphore and some of console functions (console_unlock()/etc.), so
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 2277871e1f12..480c0993abd5 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -471,7 +471,9 @@ static DEFINE_MUTEX(syslog_lock);
 static bool have_legacy_console;
 
 /*
- * Specifies if an nbcon console is registered.
+ * Specifies if an nbcon console is registered. If nbcon consoles are present,
+ * synchronous printing of legacy consoles will not occur during panic until
+ * the backtrace has been stored to the ringbuffer.
  */
 static bool have_nbcon_console;
 
@@ -2330,12 +2332,30 @@ int vprintk_store(int facility, int level,
 	return ret;
 }
 
+static bool legacy_allow_panic_sync;
+
+/*
+ * This acts as a one-way switch to allow legacy consoles to print from
+ * the printk() caller context on a panic CPU. It also attempts to flush
+ * the legacy consoles in this context.
+ */
+void printk_legacy_allow_panic_sync(void)
+{
+	legacy_allow_panic_sync = true;
+
+	if (printing_via_unlock && !is_printk_legacy_deferred()) {
+		if (console_trylock())
+			console_unlock();
+	}
+}
+
 asmlinkage int vprintk_emit(int facility, int level,
 			    const struct dev_printk_info *dev_info,
 			    const char *fmt, va_list args)
 {
+	bool do_trylock_unlock = printing_via_unlock;
+	bool defer_legacy = !do_trylock_unlock;
 	int printed_len;
-	bool in_sched = false;
 
 	/* Suppress unimportant messages after panic happens */
 	if (unlikely(suppress_printk))
@@ -2349,17 +2369,35 @@ asmlinkage int vprintk_emit(int facility, int level,
 	if (other_cpu_in_panic())
 		return 0;
 
+	/* If called from the scheduler, we can not call up(). */
 	if (level == LOGLEVEL_SCHED) {
 		level = LOGLEVEL_DEFAULT;
-		in_sched = true;
+		do_trylock_unlock = false;
+		defer_legacy = true;
 	}
 
 	printk_delay(level);
 
 	printed_len = vprintk_store(facility, level, dev_info, fmt, args);
 
-	/* If called from the scheduler, we can not call up(). */
-	if (!in_sched && printing_via_unlock) {
+	if (have_nbcon_console && !have_boot_console) {
+		nbcon_atomic_flush_pending();
+
+		/*
+		 * In panic, the legacy consoles are not allowed to print from
+		 * the printk calling context unless explicitly allowed. This
+		 * gives the safe nbcon consoles a chance to print out all the
+		 * panic messages first. This restriction only applies if
+		 * there are nbcon consoles registered and they are allowed to
+		 * flush.
+		 */
+		if (this_cpu_in_panic() && !legacy_allow_panic_sync) {
+			do_trylock_unlock = false;
+			defer_legacy = false;
+		}
+	}
+
+	if (do_trylock_unlock) {
 		/*
 		 * The caller may be holding system-critical or
 		 * timing-sensitive locks. Disable preemption during
@@ -2379,7 +2417,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 		preempt_enable();
 	}
 
-	if (in_sched && printing_via_unlock)
+	if (defer_legacy)
 		defer_console_output();
 	else
 		wake_up_klogd();
@@ -3292,7 +3330,9 @@ void console_flush_on_panic(enum con_flush_mode mode)
 	if (!have_boot_console)
 		nbcon_atomic_flush_pending();
 
-	console_flush_all(false, &next_seq, &handover);
+	/* Flush legacy consoles once allowed, even when dangerous. */
+	if (legacy_allow_panic_sync)
+		console_flush_all(false, &next_seq, &handover);
 }
 
 /*
-- 
2.39.2

On Sun 2024-08-04 02:57:32, John Ogness wrote:
> If legacy and nbcon consoles are registered and the nbcon
> consoles are allowed to flush (i.e. no boot consoles
> registered), the legacy consoles will no longer perform
> direct printing on the panic CPU until after the backtrace
> has been stored. This will give the safe nbcon consoles a
> chance to print the panic messages before allowing the
> unsafe legacy consoles to print.
> 
> If no nbcon consoles are registered or they are not allowed
> to flush, there is no change in behavior (i.e. legacy
> consoles will always attempt to print from the printk()
> caller context).
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2330,12 +2332,30 @@ int vprintk_store(int facility, int level,
>  	return ret;
>  }
>  
> +static bool legacy_allow_panic_sync;
> +
> +/*
> + * This acts as a one-way switch to allow legacy consoles to print from
> + * the printk() caller context on a panic CPU. It also attempts to flush
> + * the legacy consoles in this context.
> + */
> +void printk_legacy_allow_panic_sync(void)
> +{
> +	legacy_allow_panic_sync = true;
> +
> +	if (printing_via_unlock && !is_printk_legacy_deferred()) {
> +		if (console_trylock())
> +			console_unlock();
> +	}
> +}
> +
>  asmlinkage int vprintk_emit(int facility, int level,
>  			    const struct dev_printk_info *dev_info,
>  			    const char *fmt, va_list args)
>  {
> +	bool do_trylock_unlock = printing_via_unlock;
> +	bool defer_legacy = !do_trylock_unlock;

This should be:

	bool defer_legacy = false;

If we do not need to call the legacy loop then we do not need
to do defer it. The nbcon consoles are flushed directly, see below.


>  	int printed_len;
> -	bool in_sched = false;
>  
>  	/* Suppress unimportant messages after panic happens */
>  	if (unlikely(suppress_printk))
> @@ -2349,17 +2369,35 @@ asmlinkage int vprintk_emit(int facility, int level,
>  	if (other_cpu_in_panic())
>  		return 0;
>  
> +	/* If called from the scheduler, we can not call up(). */
>  	if (level == LOGLEVEL_SCHED) {
>  		level = LOGLEVEL_DEFAULT;
> -		in_sched = true;
> +		do_trylock_unlock = false;
> +		defer_legacy = true;

And this should be:

		defer_legacy = do_trylock_unlock;
		do_trylock_unlock = false;

>  	}
>  
>  	printk_delay(level);
>  
>  	printed_len = vprintk_store(facility, level, dev_info, fmt, args);
>  
> -	/* If called from the scheduler, we can not call up(). */
> -	if (!in_sched && printing_via_unlock) {
> +	if (have_nbcon_console && !have_boot_console) {
> +		nbcon_atomic_flush_pending();

The nbcon consoles will always be flushed directly. At least at this
stage of the patchset. I guess that a later patch will offload it to
the kthread when possible.

> +		/*
> +		 * In panic, the legacy consoles are not allowed to print from
> +		 * the printk calling context unless explicitly allowed. This
> +		 * gives the safe nbcon consoles a chance to print out all the
> +		 * panic messages first. This restriction only applies if
> +		 * there are nbcon consoles registered and they are allowed to
> +		 * flush.
> +		 */
> +		if (this_cpu_in_panic() && !legacy_allow_panic_sync) {
> +			do_trylock_unlock = false;
> +			defer_legacy = false;
> +		}
> +	}
> +
> +	if (do_trylock_unlock) {
>  		/*
>  		 * The caller may be holding system-critical or
>  		 * timing-sensitive locks. Disable preemption during
> @@ -3292,7 +3330,9 @@ void console_flush_on_panic(enum con_flush_mode mode)
>  	if (!have_boot_console)
>  		nbcon_atomic_flush_pending();
>  
> -	console_flush_all(false, &next_seq, &handover);
> +	/* Flush legacy consoles once allowed, even when dangerous. */
> +	if (legacy_allow_panic_sync)
> +		console_flush_all(false, &next_seq, &handover);
>  }

This is a good change. It make the console_flush_on_panic()
API more safe to use. console_flush_all() should not be called
before stopping other CPUs.

Best Regards,
Petr


>  
>  /*
> -- 
> 2.39.2

Hi Linus,

On Sun 2024-08-04 02:57:32, John Ogness wrote:
> If legacy and nbcon consoles are registered and the nbcon
> consoles are allowed to flush (i.e. no boot consoles
> registered), the legacy consoles will no longer perform
> direct printing on the panic CPU until after the backtrace
> has been stored. This will give the safe nbcon consoles a
> chance to print the panic messages before allowing the
> unsafe legacy consoles to print.
> 
> If no nbcon consoles are registered or they are not allowed
> to flush, there is no change in behavior (i.e. legacy
> consoles will always attempt to print from the printk()
> caller context).

I want to be sure that this is acceptable to you.

This behavior has already been in the rejected pull request
for 6.11, see https://lore.kernel.org/r/Zp-_7R49fIHgIhaq@pathway.suse.cz

You did not complain about this particular change. But it is
yet another buffering in critical situation which you do not
like in general.

It is a bit different from the buffering during Oops. In this case,
the new nbcon consoles will still be flushed immediately. And
the legacy consoles will be blocked only when there is a nbcon
console.

The buffering here should increase the chance to see the messages
on the more safe (nbcon) consoles. The legacy consoles are less
safe primary because of the bust_spinlocks(1) called earlier.

I personally do not have strong opinion. The change makes sense.
It looks like it should make more good than harm. But it is not
a clear win.

Best Regards,
Petr

On Tue, 6 Aug 2024 at 02:59, Petr Mladek <pmladek@suse.com> wrote:
>
> I want to be sure that this is acceptable to you.

Just going by the superficial code, it's perfectly fine to delay other
consoles, as long as *some* consoles don't get delayed.

So yes:

> It is a bit different from the buffering during Oops. In this case,
> the new nbcon consoles will still be flushed immediately.

that sounds fine to me.

In general, I think delaying flushing is fine if

 (a) re-entrancy issues with already active (ie non-buffered) printing
on a console

 (b) there are higher-priority consoles that did flush it so that
something made it out

and obviously the whole "nbcon existing" will match that (b) case.

(In a perfect world the (a) case would be per-cpu, ie a "avoid obvious
deadlock", but honestly, I think any oops while the console is already
busy synchronously printing is understandable - and any concurrent
synchronous printing may be the more immediate cause of the oops -
we've certainly seen lots of cases where a bug on one CPU will then
immediately trigger on another one).

            Linus