Message-ID: <aEhS4iybzwYYMkJF@pathway.suse.cz>
Date: Tue, 10 Jun 2025 17:44:34 +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>, 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 Wed 2025-05-21 13:31:20, Feng Tang wrote:
> On Fri, May 16, 2025 at 01:38:02PM +0800, Feng Tang wrote:
> > On Thu, May 15, 2025 at 12:32:04PM +0200, Petr Mladek wrote:
> [...]
> > > > > 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().
> 
> Hi Petr
> 
> I tried further this way, and with below patch on top of current 1/3
> patch, the 'panic_sys_info' sysctl interface basically works, as parsing
> user-input, and save it in 'panic_print' bitmap. 

It does not apply. It seems that it depends on another change which
crated lib/sys_info.c...

> It has  one problem that it doesn't support the string parsing as a the
> kernel command line parameter (auto-derived from sysctl interface), I'm
> not sure if we should add a __setup() or early_param() for it, or it's
> fine?

Ah, I was not aware of this. We need to make it working from the
command line, definitely. I would go with __setup() for now. We could
always switch it to early_param() when anyone requires it.

See some more comments, below.

> ---
> diff --git a/include/linux/sys_info.h b/include/linux/sys_info.h
> index 79bf4a942e5f..d6d55646e25a 100644
> --- a/include/linux/sys_info.h
> +++ b/include/linux/sys_info.h
> @@ -17,4 +17,8 @@
>  
>  extern void sys_show_info(unsigned long info_mask);
>  
> +struct ctl_table;
> +extern int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
> +					  void *buffer, size_t *lenp,
> +					  loff_t *ppos);
>  #endif	/* _LINUX_SYS_INFO_H */
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 3d9cf8063242..8ca9b30f0fe4 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -88,6 +88,13 @@ static const struct ctl_table kern_panic_table[] = {
>  		.extra2         = SYSCTL_ONE,
>  	},
>  #endif
> +	{
> +		.procname	= "panic_sys_info",
> +		.data		= &panic_print,
> +		.maxlen         = sizeof(panic_print),
> +		.mode		= 0644,
> +		.proc_handler	= sysctl_sys_info_handler,
> +	},
>  	{
>  		.procname       = "warn_limit",
>  		.data           = &warn_limit,
> diff --git a/lib/sys_info.c b/lib/sys_info.c
> index 4090b2e0515e..27de6f0d0a4d 100644
> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> @@ -4,6 +4,121 @@
>  #include <linux/console.h>
>  #include <linux/nmi.h>
>  
> +struct sys_info_name {
> +	unsigned long bit;
> +	const char *name;
> +};
> +
> +static const char sys_info_avail[] = " tasks mem timer lock ftrace all_bt blocked_tasks ";

It is a bit confusing to have it space-separated when the parameter
is comma-separated. Also I am not sure why there is the leading and
trailing space.

I would expect:

static const char sys_info_avail[] = "tasks,mem,timer,lock,ftrace,all_bt,blocked_tasks";

> +static const struct sys_info_name  si_names[] = {
> +	{ SYS_SHOW_TASK_INFO,	"tasks" },
> +	{ SYS_SHOW_MEM_INFO,	"mem" },
> +	{ SYS_SHOW_TIMER_INFO,	"timer" },
> +	{ SYS_SHOW_LOCK_INFO,	"lock" },
> +	{ SYS_SHOW_FTRACE_INFO, "ftrace" },
> +	{ SYS_SHOW_ALL_CPU_BT,	"all_bt" },
> +	{ SYS_SHOW_BLOCKED_TASKS, "blocked_tasks" },
> +};

I guess that this is just an RFC. Anyway, I would expect that
SYS_SHOW_* values would be defined in sys_info.h.

> +
> +/* Expecting string like "xxx_sys_info=tasks,mem,timer,lock" */
> +static int write_handler(const struct ctl_table *ro_table, void *buffer,
> +				size_t *lenp, loff_t *ppos)
> +{
> +	char names[sizeof(sys_info_avail)];
> +	char *buf, *name;
> +	struct ctl_table table;
> +	unsigned long *si_flag;
> +	int i, len, ret;
> +
> +	si_flag = ro_table->data;
> +
> +	/* Clear it first */
> +	*si_flag = 0;
> +
> +	table = *ro_table;
> +	table.data = names;
> +	table.maxlen = sizeof(names);
> +	ret = proc_dostring(&table, 1, buffer, lenp, ppos);
> +	if (ret)
> +		return ret;
> +
> +	buf = names;
> +	while ((name = strsep(&buf, ",")) && *name) {
> +		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> +			if (!strcmp(name, si_names[i].name))
> +				*si_flag |= si_names[i].bit;
> +		}
> +	}
> +
> +	return 0;
> +}

The above function is defined but not used. The same code is
copy&pasted in the if (write) section below.

I think that we would need a helper function which could be used
in both sysctl_sys_info_handler() and in the __setup() callback.

Something like:

static unsigned long sys_info_parse_flags(char *str)
{
	unsigned long si_bits = 0;
	char *s, *name;
	int i;

	s = str;
	while ((name = strsep(&s, ",")) && *name) {
		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
			if (!strcmp(name, si_names[i].name)) {
				*si_bits |= si_names[i].bit;
				break;
			}
		}
	}

	return si_bits;
}

> +
> +int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
> +					  void *buffer, size_t *lenp,
> +					  loff_t *ppos)
> +{
> +	char names[sizeof(sys_info_avail) + 1];
> +	char *buf, *name;
> +	struct ctl_table table;
> +	unsigned long *si_flag;

Nit: I would call this "si_bits_global" to make it more clear that
     this is pointer to the global bitmask.


> +	int i, ret, len;
> +
> +	si_flag = ro_table->data;
> +
> +	if (write) {
> +		/* Clear it first */
> +		*si_flag = 0;

There is no synchronization against readers. IMHO, it is not worth it.
But we should at least update the global value only once.

We should define a local variable, e.g.

		unsigned long si_bits;

and do the following:

> +		table = *ro_table;
> +		table.data = names;
> +		table.maxlen = sizeof(names);
> +		ret = proc_dostring(&table, 1, buffer, lenp, ppos);

I would pass the "write" parameter here instead of the hard-coded "1".
Do we know that it should be exactly '1'?

> +		if (ret)
> +			return ret;

		si_bits = sys_info_parse_param(flags);
		/*
		 * The access to the global value is not synchronized.
		 * Update it at once at least.
		 */
		WRITE_ONCE(*si_bits_global, si_bits);

> +		/* Expecting string like "xxx_sys_info=tasks,mem,timer,lock" */
> +		buf = names;
> +		while ((name = strsep(&buf, ",")) && *name) {
> +			for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> +				if (!strcmp(name, si_names[i].name))
> +					*si_flag |= si_names[i].bit;
> +			}
> +		}
> +
> +		return 0;
> +	} else {
> +		bool first = true;
> +
> +		memset(names, 0, sizeof(names));

I guess that you took this from read_actions_logged().

It looks too paranoid to me. I do not see it anywhere else.
IMHO, if the proc_dostring() does not stop at the trailing '\0'
then most interfaces would leak data.

> +		buf = names;
> +		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> +			if (*si_flag & si_names[i].bit) {
> +
> +				if (first) {
> +					first = false;
> +				} else {
> +					*buf = ',';
> +					buf++;
> +				}
> +
> +				len = strlen(si_names[i].name);
> +				strncpy(buf, si_names[i].name, len);
> +				buf += len;
> +			}
> +
> +		}
> +		*buf = '\0';

IMHO, always adding this trailing '\0' should be enough.

> +		table = *ro_table;
> +		table.data = names;
> +		table.maxlen = sizeof(names);
> +		return proc_dostring(&table, 0, buffer, lenp, ppos);

I would pass the "write" parameter here instead of the hard coded 0.
But it is a matter of taste.

> +	}
> +}
> +
>  void sys_show_info(unsigned long info_flag)
>  {
>  	if (info_flag & SYS_SHOW_TASK_INFO)

Best Regards,
Petr

PS: I am sorry for the late reply. Too many things have accumulated
over the few last weeks.

Powered by blists - more mailing lists