linux-kernel - Re: [PATCH v3] panic: add an option to replay all the printk message in buffer

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190425110517.cafkeexdr6r4b25v@pathway.suse.cz>
Date:   Thu, 25 Apr 2019 13:05:17 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Feng Tang <feng.tang@...el.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        linux-kernel@...r.kernel.org,
        Aaro Koskinen <aaro.koskinen@...ia.com>,
        Kees Cook <keescook@...omium.org>, Borislav Petkov <bp@...e.de>
Subject: Re: [PATCH v3] panic: add an option to replay all the printk message
 in buffer

On Wed 2019-04-24 16:51:12, Feng Tang wrote:
> Currently on panic, kernel will lower the loglevel and print out
> pending printk msg only with console_flush_on_panic().
> 
> Add an option for users to configure the "panic_print" to replay
> all dmesg in buffer, some of which they may have never seen due
> to the loglevel setting, which will help panic debugging .
> 
> diff --git a/kernel/panic.c b/kernel/panic.c
> index b9f004e..b9c0eb3 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -53,6 +53,7 @@ int panic_suppress_printk;
>  #define PANIC_PRINT_TIMER_INFO		0x00000004
>  #define PANIC_PRINT_LOCK_INFO		0x00000008
>  #define PANIC_PRINT_FTRACE_INFO		0x00000010
> +#define PANIC_PRINT_ALL_PRINTK_MSG	0x00000020
>  unsigned long panic_print;
>  
>  ATOMIC_NOTIFIER_HEAD(panic_notifier_list);
> @@ -136,6 +137,11 @@ EXPORT_SYMBOL(nmi_panic);
>  
>  static void panic_print_sys_info(void)
>  {
> +	if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
> +		console_flush_on_panic(CONSOLE_REPLAY_ALL);
> +	else
> +		console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> +
>  	if (panic_print & PANIC_PRINT_TASK_INFO)
>  		show_state();
>  
> @@ -279,8 +285,6 @@ void panic(const char *fmt, ...)
>  	 * panic() is not being callled from OOPS.
>  	 */
>  	debug_locks_off();
> -	console_flush_on_panic();

I would prefer if we keep it here. It is a very important
operation that plays a role in many panic/printk related
bugs. We should not hide it in an "unrelated"
function.

The name "panic_print_sys_info" suggests that it
just shows some additional information.

I guess that you wanted to avoid flushing some messages
twice. IMHO, it is not a big deal. In many situations,
console_flush_on_panic(CONSOLE_FLUSH_PENDING) will
do nothing or it would handle just few lines.

Also people might decide to do a forced reboot when
the console is slow and it takes ages to print
everything. Then it might be useful to handle
the few "most" important lines before starting
from beginning.

Other than that I am fine with the patch. I give up
on bike shedding about the header line ;-)

Best Regards,
Petr