linux-kernel - Re: [PATCH v1 1/3] kernel/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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aCbPOry4r_ac0zUL@U-2FWC9VHC-2323.local>
Date: Fri, 16 May 2025 13:38:02 +0800
From: Feng Tang <feng.tang@...ux.alibaba.com>
To: Petr Mladek <pmladek@...e.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Lance Yang <lance.yang@...ux.dev>, linux-kernel@...r.kernel.org,
	mhiramat@...nel.org, llong@...hat.com,
	"Paul E. McKenney" <paulmck@...nel.org>
Subject: Re: [PATCH v1 1/3] kernel/panic: generalize panic_print's function
 to show sys info

On Thu, May 15, 2025 at 12:32:04PM +0200, Petr Mladek wrote:
> On Tue 2025-05-13 21:23:25, Feng Tang wrote:
> > Hi Petr,
> > 
> > Thanks for the review!
> > 
> > On Tue, May 13, 2025 at 11:57:19AM +0200, Petr Mladek wrote:
> > > On Sun 2025-05-11 16:52:52, 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 help debugging cases
> > > > like task-hung, soft/hard lockup too, where user may need the snapshot
> > > > of system info at that time.
> > > > 
> > > The generic approach might deserve a separate source file,
> > > for example:
> > > 
> > >     include/linux/sys_info.h
> > >     lib/sys_info.c
> > 
> > Thanks for the suggestion! I'm really not good at naming. 
> > 
> > As panic.c is always built-in, I'll put sys_info.c as obj-y.
> 
> Makes sense.
> 
> > > Also I always considered the bitmask as a horrible user interface.
> > > It might be used internally. But it would be better to allow a human
> > > readable parameter, for example:
> > > 
> > > 	panic_sys_info=task,mem,timer,lock,ftrace,bt,all_bt,blocked_tasks
> > 
> > Yes, it's convenient for developers, as a cmdline parameter being parsed
> > at boot time.
> > 
> > But I think bitmask may be easier for runtime changing as a core parameter
> > under /proc/ or /sys/, or from sysctl interface. There are also some other
> > modules use debug bitmask controlling printking info for different
> > sub-components.
> 
> Good to know. Could you please give me a pointer to some other modules
> using the bitmask? I believe that they exist but I can't find any.
> I wonder how common it is...

Definitely not common :) I only know one: ACPI, which has 2 debug knobs,
'debug_layer' is a bigmap to control which sub-component's msg to be
dumped, and 'debug_level'.

> Anyway, I personally find words/names easier to use.

True. For me, I have some notes sticked on my monitor: one about characters
for /proc/sysrq-trigger, one for panic_print, one for struct page's flag :)

> For example,
> I like the following interfaces:
> 
>   #> cat /sys/power/pm_test
>   [none] core processors platform devices freezer
> 
>   #> cat /sys/kernel/debug/tracing/available_tracers
>   blk function_graph wakeup_dl wakeup_rt wakeup function nop
> 
>   #> cat /proc/sys/kernel/seccomp/actions_avail
>   kill_process kill_thread trap errno user_notif trace log allow
>   # cat /proc/sys/kernel/seccomp/actions_logged
>   kill_process kill_thread trap errno user_notif trace log

Thanks for the info, will check them.

> > And we have similar control knobs for hung, lockup etc.
> > 
> > Or should I change the name from 'xxx_print_mask' to 'xxx_sysinfo_flag'
> > in patch 2/3 ?
> > 
> > > 
> > > The console reply might be handled by a separate:
> > > 
> > > 	panic_console_reply=1
> > > 
> > > And it would obsolete the existing "panic_print" which is an
> > > ugly name and interface from my POV.
> > 
> > Agree it's ugly :). But besides a kernel parameter,  'panic_print' is
> > also a sysctl interface, I'm afraid renaming it might break user ABI.
> 
> A solution would be to keep it and create "panic_sys_info="
> with the human readable parameters in parallel. They would
> store the request in the same bitmap.
> 
> We could print a message that "panic_print" has been obsoleted
> by "panic_sys_info" when people use it.
> 
> Both parameters would override the current bitmap. So the later
> used parameter or procfs/sysfs write would win.

Reasonalbe.

> Note:
> 
> One question is whether to use sysctl or module parameters.
> 
> An advantage of sysctl is the "systcl" userspace tool. Some people
> might like it. But the API is very old and a bit cumbersome for
> implementing.
> 
> The sysfs, aka include/linux/moduleparam.h, API looks cleaner to me.
> But the parameters are hidden in the /sys/... jungle ;-)
> 
> I would slightly prefer "sysctl" because these parameters are easier
> to find.

I will think about the string parsing in sys_info.c, and in the backend,
a bitmap is still needed to save the parsing result, and as the parameter
for sys_show_info().

Also if we go 'sysctl' way, in the future, some exising interface like
'sysctl_hung_task_all_cpu_backtrace' could be deprecated and integrated
into the 'hung_task_sys_info'? 

Thanks,
Feng

> Best Regards,
> Petr