[PATCH v2 0/2] printk: CPU backtrace not printing on panic

[PATCH v2 0/2] printk: CPU backtrace not printing on panic

[takakura]

From: Ryo Takakura <takakura@valinux.co.jp>

Hi! 

This patchset fixes 2 issues on CPU backtrace during panic. The first 
issue was pointed out by John [1].

(1) The usual(non-panic context) flushing of backtraces written into 
ringbuffer does not work during panic as non-panicked CPUs can't do the 
flushing themselves.

(2) CPU backtrace triggered during panic has stopped working since the 
commit 779dbc2e78d7 ("printk: Avoid non-panic CPUs writing to ringbuffer") 
as it disabled non-panicked cpus writing into ringbuffer after panic.

Sincerely,
Ryo Takakura

---

Changes since V1:
[1] https://lore.kernel.org/all/20240729114601.176047-1-takakura@valinux.co.jp/T/

- Thanks for the feedbacks by Petr and John.
- Fix the issue (1) seperately from original patch.
- Fix the issue (2) by allowing non-panicked CPUs to write into ringbuffer 
while CPU backtrace is triggered as suggested by Petr.

---

Ryo Takakura (2):
  Handle flushing of CPU backtraces during panic
  Allow cpu backtraces to be written into ringbuffer during panic

 include/linux/panic.h  | 1 +
 kernel/panic.c         | 9 ++++++++-
 kernel/printk/printk.c | 2 +-
 3 files changed, 10 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] Handle flushing of CPU backtraces during panic

[takakura]

From: Ryo Takakura <takakura@valinux.co.jp>

After panic, non-panicked CPU's has been unable to flush ringbuffer 
while they can still write into it. This can affect CPU backtrace 
triggered in panic only able to write into ringbuffer incapable of 
flushing them.

Fix the issue by letting the panicked CPU handle the flushing of 
ringbuffer right after non-panicked CPUs finished writing their
backtraces.

Signed-off-by: Ryo Takakura <takakura@valinux.co.jp>
---
 kernel/panic.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 7e2070925..f94923a63 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -252,8 +252,10 @@ void check_panic_on_warn(const char *origin)
  */
 static void panic_other_cpus_shutdown(bool crash_kexec)
 {
-	if (panic_print & PANIC_PRINT_ALL_CPU_BT)
+	if (panic_print & PANIC_PRINT_ALL_CPU_BT) {
 		trigger_all_cpu_backtrace();
+		console_flush_on_panic(CONSOLE_FLUSH_PENDING);
+	}
 
 	/*
 	 * Note that smp_send_stop() is the usual SMP shutdown function,
-- 
2.34.1

[PATCH v2 2/2] Allow cpu backtraces to be written into ringbuffer during panic

[takakura]

From: Ryo Takakura <takakura@valinux.co.jp>

commit 779dbc2e78d7 ("printk: Avoid non-panic CPUs writing
to ringbuffer") disabled non-panic CPUs to further write messages to
ringbuffer after panicked.

Since the commit, non-panicked CPU's are not allowed to write to
ring buffer after panicked and CPU backtrace which is triggered
after panicked to sample non-panicked CPUs' backtrace no longer
serves its function as it has nothing to print.

Fix the issue by allowing non-panicked CPUs to write into ringbuffer
while CPU backtrace is in flight.

Fixes: 779dbc2e78d7 ("printk: Avoid non-panic CPUs writing to ringbuffer")
Signed-off-by: Ryo Takakura <takakura@valinux.co.jp>
---
 include/linux/panic.h  | 1 +
 kernel/panic.c         | 5 +++++
 kernel/printk/printk.c | 2 +-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/linux/panic.h b/include/linux/panic.h
index 6717b15e7..556b4e2ad 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -16,6 +16,7 @@ extern void oops_enter(void);
 extern void oops_exit(void);
 extern bool oops_may_print(void);
 
+extern bool panic_triggering_all_cpu_backtrace;
 extern int panic_timeout;
 extern unsigned long panic_print;
 extern int panic_on_oops;
diff --git a/kernel/panic.c b/kernel/panic.c
index f94923a63..d7ed93567 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -63,6 +63,8 @@ unsigned long panic_on_taint;
 bool panic_on_taint_nousertaint = false;
 static unsigned int warn_limit __read_mostly;
 
+bool panic_triggering_all_cpu_backtrace;
+
 int panic_timeout = CONFIG_PANIC_TIMEOUT;
 EXPORT_SYMBOL_GPL(panic_timeout);
 
@@ -253,7 +255,10 @@ void check_panic_on_warn(const char *origin)
 static void panic_other_cpus_shutdown(bool crash_kexec)
 {
 	if (panic_print & PANIC_PRINT_ALL_CPU_BT) {
+		/* Temporary allow non-panic CPUs to write their backtraces. */
+		panic_triggering_all_cpu_backtrace = true;
 		trigger_all_cpu_backtrace();
+		panic_triggering_all_cpu_backtrace = false;
 		console_flush_on_panic(CONSOLE_FLUSH_PENDING);
 	}
 
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index d0bff0b0a..fa2580dd3 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2354,7 +2354,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 	 * non-panic CPUs are generating any messages, they will be
 	 * silently dropped.
 	 */
-	if (other_cpu_in_panic())
+	if (other_cpu_in_panic() && !panic_triggering_all_cpu_backtrace)
 		return 0;
 
 	if (level == LOGLEVEL_SCHED) {
-- 
2.34.1

Re: [PATCH v2 1/2] Handle flushing of CPU backtraces during panic

[Petr Mladek]

On Sat 2024-08-03 17:12:30, takakura@valinux.co.jp wrote:
> From: Ryo Takakura <takakura@valinux.co.jp>
> 
> After panic, non-panicked CPU's has been unable to flush ringbuffer 
> while they can still write into it. This can affect CPU backtrace 
> triggered in panic only able to write into ringbuffer incapable of 
> flushing them.
> 
> Fix the issue by letting the panicked CPU handle the flushing of 
> ringbuffer right after non-panicked CPUs finished writing their
> backtraces.
> 
> Signed-off-by: Ryo Takakura <takakura@valinux.co.jp>
> ---
>  kernel/panic.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 7e2070925..f94923a63 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -252,8 +252,10 @@ void check_panic_on_warn(const char *origin)
>   */
>  static void panic_other_cpus_shutdown(bool crash_kexec)
>  {
> -	if (panic_print & PANIC_PRINT_ALL_CPU_BT)
> +	if (panic_print & PANIC_PRINT_ALL_CPU_BT) {
>  		trigger_all_cpu_backtrace();
> +		console_flush_on_panic(CONSOLE_FLUSH_PENDING);

Hmm, this is too dangerous.

console_flush_on_panic() is supposed to be called at the end on
panic() as the final desperate attempt to flush consoles.
It does not take console_lock(). It must not be called before
stopping non-panic() CPUs.

We would need to implement something like:

/**
 * console_try_flush - try to flush consoles when safe
 *
 * Context: Any, except for NMI.
 */
void console_try_flush(void)
{
	if (is_printk_legacy_deferred())
		return;

	if (console_trylock())
		console_unlock();
}

, where is_printk_legacy_deferred() is not yet in the mainline. It is a new
API proposed by the last version of a patchset adding adding write_atomic() callback,
see https://lore.kernel.org/all/20240804005138.3722656-24-john.ogness@linutronix.de/

Best Regards,
Petr

Re: [PATCH v2 2/2] Allow cpu backtraces to be written into ringbuffer during panic

[Petr Mladek]

On Sat 2024-08-03 17:16:49, takakura@valinux.co.jp wrote:
> From: Ryo Takakura <takakura@valinux.co.jp>
> 
> commit 779dbc2e78d7 ("printk: Avoid non-panic CPUs writing
> to ringbuffer") disabled non-panic CPUs to further write messages to
> ringbuffer after panicked.
> 
> Since the commit, non-panicked CPU's are not allowed to write to
> ring buffer after panicked and CPU backtrace which is triggered
> after panicked to sample non-panicked CPUs' backtrace no longer
> serves its function as it has nothing to print.
> 
> Fix the issue by allowing non-panicked CPUs to write into ringbuffer
> while CPU backtrace is in flight.
> 
> Fixes: 779dbc2e78d7 ("printk: Avoid non-panic CPUs writing to ringbuffer")
> Signed-off-by: Ryo Takakura <takakura@valinux.co.jp>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v2 1/2] Handle flushing of CPU backtraces during panic

[John Ogness]

On 2024-08-03, takakura@valinux.co.jp wrote:
> From: Ryo Takakura <takakura@valinux.co.jp>
>
> After panic, non-panicked CPU's has been unable to flush ringbuffer 
> while they can still write into it. This can affect CPU backtrace 
> triggered in panic only able to write into ringbuffer incapable of 
> flushing them.

Right now, they cannot write to it. If you apply your second patch
before this one, then the above statement is true.

Perhaps the ordering of the two patches should be reversed?

Either way, for the series:

Reviewed-by: John Ogness <john.ogness@linutronix.de>

Re: [PATCH v2 1/2] Handle flushing of CPU backtraces during panic

[John Ogness]

On 2024-08-05, Petr Mladek <pmladek@suse.com> wrote:
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index 7e2070925..f94923a63 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -252,8 +252,10 @@ void check_panic_on_warn(const char *origin)
>>   */
>>  static void panic_other_cpus_shutdown(bool crash_kexec)
>>  {
>> -	if (panic_print & PANIC_PRINT_ALL_CPU_BT)
>> +	if (panic_print & PANIC_PRINT_ALL_CPU_BT) {
>>  		trigger_all_cpu_backtrace();
>> +		console_flush_on_panic(CONSOLE_FLUSH_PENDING);
>
> Hmm, this is too dangerous.
>
> console_flush_on_panic() is supposed to be called at the end on
> panic() as the final desperate attempt to flush consoles.

Thanks for catching this. I keep forgetting that
console_flush_on_panic() is the legacy variant of
nbcon_atomic_flush_unsafe(), but is called much earlier.

John

Re: [PATCH v2 1/2] Handle flushing of CPU backtraces during panic

[takakura]

Hi Petr and John,

On 2024-08-05, Petr Mladek wrote:
>On Sat 2024-08-03 17:12:30, takakura@valinux.co.jp wrote:
>> From: Ryo Takakura <takakura@valinux.co.jp>
>> 
>> After panic, non-panicked CPU's has been unable to flush ringbuffer 
>> while they can still write into it. This can affect CPU backtrace 
>> triggered in panic only able to write into ringbuffer incapable of 
>> flushing them.
>> 
>> Fix the issue by letting the panicked CPU handle the flushing of 
>> ringbuffer right after non-panicked CPUs finished writing their
>> backtraces.
>> 
>> Signed-off-by: Ryo Takakura <takakura@valinux.co.jp>
>> ---
>>  kernel/panic.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index 7e2070925..f94923a63 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -252,8 +252,10 @@ void check_panic_on_warn(const char *origin)
>>   */
>>  static void panic_other_cpus_shutdown(bool crash_kexec)
>>  {
>> -	if (panic_print & PANIC_PRINT_ALL_CPU_BT)
>> +	if (panic_print & PANIC_PRINT_ALL_CPU_BT) {
>>  		trigger_all_cpu_backtrace();
>> +		console_flush_on_panic(CONSOLE_FLUSH_PENDING);
>
>Hmm, this is too dangerous.
>
>console_flush_on_panic() is supposed to be called at the end on
>panic() as the final desperate attempt to flush consoles.
>It does not take console_lock(). It must not be called before
>stopping non-panic() CPUs.

I see, yes, it is dangerous...  Thanks for pointing this out!

Just out of curiosity, if we only had nbcon consoles which disables 
acquiring console lock after panic as pointed out by John on [0], 
I guess in that case we will be able to call
console_flush_on_panic(CONSOLE_FLUSH_PENDING) in this context.

>We would need to implement something like:
>
>/**
> * console_try_flush - try to flush consoles when safe
> *
> * Context: Any, except for NMI.
> */
>void console_try_flush(void)
>{
>	if (is_printk_legacy_deferred())
>		return;
>
>	if (console_trylock())
>		console_unlock();
>}
>
>, where is_printk_legacy_deferred() is not yet in the mainline. It is a new
>API proposed by the last version of a patchset adding adding write_atomic() callback,
>see https://lore.kernel.org/all/20240804005138.3722656-24-john.ogness@linutronix.de/

Also thanks for potinting the direction.
I'll send the next version with console_try_flush() for flushing backtraces 
as suggested!

>Best Regards,
>Petr

Sincerely,
Ryo Takakura

[0] https://lore.kernel.org/lkml/875xsofl7i.fsf@jogness.linutronix.de/

Re: [PATCH v2 1/2] Handle flushing of CPU backtraces during panic

[takakura]

Hi John and Petr,

On 2024-08-05, John Ogness wrote:
>On 2024-08-03, takakura@valinux.co.jp wrote:
>> From: Ryo Takakura <takakura@valinux.co.jp>
>>
>> After panic, non-panicked CPU's has been unable to flush ringbuffer 
>> while they can still write into it. This can affect CPU backtrace 
>> triggered in panic only able to write into ringbuffer incapable of 
>> flushing them.
>
>Right now, they cannot write to it. If you apply your second patch
>before this one, then the above statement is true.
>
>Perhaps the ordering of the two patches should be reversed?

Yes, that is true. Thanks!
I'll send the patches in the reverse order for next version. 

>Either way, for the series:
>
>Reviewed-by: John Ogness <john.ogness@linutronix.de>

Sincerely,
Ryo Takakura