linux-kernel - Re: [PATCH v3 2/2] Handle flushing of CPU backtraces during panic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20240831082004.110975-1-takakura@valinux.co.jp>
Date: Sat, 31 Aug 2024 17:20:04 +0900
From: takakura@...inux.co.jp
To: john.ogness@...utronix.de
Cc: pmladek@...e.com,
	akpm@...ux-foundation.org,
	bhe@...hat.com,
	feng.tang@...el.com,
	j.granados@...sung.com,
	linux-kernel@...r.kernel.org,
	lukas@...ner.de,
	nishimura@...inux.co.jp,
	rostedt@...dmis.org,
	senozhatsky@...omium.org,
	stephen.s.brennan@...cle.com,
	taka@...inux.co.jp,
	takakura@...inux.co.jp,
	ubizjak@...il.com,
	wangkefeng.wang@...wei.com
Subject: Re: [PATCH v3 2/2] Handle flushing of CPU backtraces during panic

Hi John,

Thanks for checking into what I pointed out!

On 2024-08-26, John Ogness wrote:
>On 2024-08-21, takakura@...inux.co.jp wrote:
>>>> /**
>>>>  * console_try_or_trigger_flush - try to flush consoles directly when
>>>>  *   safe or the trigger deferred flush.
>>>>  *
>>>>  * Context: Any
>>>>  */
>>>> void console_try_or_trigger_flush(void)
>>>> {
>>>>      if (!is_printk_legacy_deferred() && console_trylock())
>>>>              console_unlock();
>>>>      else
>>>>              defer_console_output();
>>>> }
>>>>
>>>> and use it instead of printk_trigger_flush() in
>>>> nmi_trigger_cpumask_backtrace().
>>>
>>> Just to be clear, you are talking about removing
>>> printk_trigger_flush() entirely and instead provide the new
>>> console_try_or_trigger_flush()?  Which then also involves updating
>>> the call sites:
>>>
>>> lib/nmi_backtrace.c:nmi_trigger_cpumask_backtrace()
>>> arch/powerpc/kernel/watchdog.c:watchdog_timer_interrupt()
>>>
>>
>> Taking a look at [0], in addition to the mentioned call sites, 
>> nbcon_device_release() will also be calling printk_trigger_flush()?
>> For nbcon_device_release(), I thought its better not to be replaced as 
>> it calles for @legacy_off, in which case printk_trigger_flush() seems 
>> more suitable as it always defers printing.
>
>The @legacy_off logic would be within console_try_or_trigger_flush(),
>so the result would be the same.

I see.

>> Also taking a look at the [1], for nmi_trigger_cpumask_backtrace(), 
>> I thought that it will not comply with the syncing of 
>> legacy_allow_panic_sync. I believe it will allow flushing of legacy consoles 
>> before printk_legacy_allow_panic_sync() which is out of sync.
>
>But isn't your patch also causing that violation?

Yes, the patch I sent would be causing the violation...
Sorry I should have asked this earlier before sending patch. The question came up
after sending the patch.

>printk_legacy_allow_panic_sync() performs a trylock/unlock. Isn't that
>good enough?

Also sorry for being unclear here. I also think that 
printk_legacy_allow_panic_sync()'s trylock/unlock is good enough.
My question was that if we were to call console_try_or_trigger_flush() in
panic, wouldn't we need to check @legacy_direct to avoid flushing 
of legacy consoles.

Something like:
/**
 * console_try_or_trigger_flush - try to flush consoles directly when
 *	safe or the trigger deferred flush.
 *
 * Context: Any
 */
void console_try_or_trigger_flush(void)
{
        struct console_flush_type ft;
        printk_get_console_flush_type(&ft);
        if (ft.legacy_direct) {
                if (console_trylock())
                        console_unlock();
        } else {
                defer_console_output();
        }
}

>John

Sincerely,
Ryo Takakura