linux-kernel - Re: [PATCH V2 2/5] panic: generalize 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: <aFV8ZFZ0Qy715hdR@pathway.suse.cz>
Date: Fri, 20 Jun 2025 17:21:08 +0200
From: Petr Mladek <pmladek@...e.com>
To: Feng Tang <feng.tang@...ux.alibaba.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Lance Yang <lance.yang@...ux.dev>, Jonathan Corbet <corbet@....net>,
	linux-kernel@...r.kernel.org, paulmck@...nel.org,
	john.ogness@...utronix.de
Subject: Re: [PATCH V2 2/5] panic: generalize panic_print's function to show
 sys info

On Mon 2025-06-16 09:08:37, Feng Tang wrote:
> 'panic_print' was introduced to help debugging kernel panic by dumping
> different kinds of system information like tasks' call stack, memory,
> ftrace buffer, etc. Acutually this function could also help debugging
> cases like task-hung, soft/hard lockup, and other cases , where user
> may need the snapshot of system info at that time.
> 
> Extract sys_show_info() function out of panic code to be used by other
> kernel parts for debugging.
> 
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -27,6 +27,7 @@
>  #include <linux/math.h>
>  #include <linux/minmax.h>
>  #include <linux/typecheck.h>
> +#include <linux/sys_info.h>

There will be only few users of this API. There is no need to
include it via this generic header which is included almost
everywhere.

Some people are working hard on getting rid of this header file,
see the comment at the beginning:

 * This header has combined a lot of unrelated to each other stuff.
 * The process of splitting its content is in progress while keeping
 * backward compatibility. That's why it's highly recommended NOT to
 * include this header inside another header file, especially under
 * generic or architectural include/ directory.

Instead, please include the new linux/sys_info.h in panic.c directly.

>  #include <linux/panic.h>
>  #include <linux/printk.h>
>  #include <linux/build_bug.h>

> --- /dev/null
> +++ b/include/linux/sys_info.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_SYS_INFO_H
> +#define _LINUX_SYS_INFO_H
> +
> +/*
> + * SYS_SHOW_ALL_PRINTK_MSG is for panic case only, as it needs special
> + * handling which only fits panic case.

This flags is really special. I would even rename it to match
the function where it is used:

#define PANIC_CONSOLE_REPLAY		0x00000020

And it would be better to do the rename (ALL_PRINTK_MSG ->
CONSOLE_REPLAY) already in the 1st patch where panic_console_replay()
was introduced.

Also it would make sense to update the documentation (in 1st patch),
something like:

--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4533,7 +4533,7 @@
 			bit 2: print timer info
 			bit 3: print locks info if CONFIG_LOCKDEP is on
 			bit 4: print ftrace buffer
-			bit 5: print all printk messages in buffer
+			bit 5: replay all messages on consoles at the end of panic
 			bit 6: print all CPUs backtrace (if available in the arch)
 			bit 7: print only tasks in uninterruptible (blocked) state
 			*Be aware* that this option may print a _lot_ of lines,

> + */
> +#define SYS_SHOW_TASK_INFO		0x00000001
> +#define SYS_SHOW_MEM_INFO		0x00000002
> +#define SYS_SHOW_TIMER_INFO		0x00000004
> +#define SYS_SHOW_LOCK_INFO		0x00000008
> +#define SYS_SHOW_FTRACE_INFO		0x00000010
> +#define SYS_SHOW_ALL_PRINTK_MSG		0x00000020
> +#define SYS_SHOW_ALL_CPU_BT		0x00000040
> +#define SYS_SHOW_BLOCKED_TASKS		0x00000080
> +
> +extern void sys_show_info(unsigned long info_mask);

Please, do not use "extern" in new header files. This is from
Documentation/process/coding-style.rst:

    Do not use the ``extern`` keyword with function declarations as this makes
    lines longer and isn't strictly necessary.

Also the header file is named "sys_info" but the API is "sys_show_*info".
It would be more user friendly to consistently use the same prefix
for the entire API, for example:

#define SYS_INFO_TASK		0x00000001
#define SYS_INFO_MEM		0x00000002
#define SYS_INFO_TIMER		0x00000004

void sys_info(unsigned long si_mask);

I am sorry that I did not tell your this in the RFC.
I focused on the bigger picture at that time.

> +#endif	/* _LINUX_SYS_INFO_H */

Best Regards,
Petr