Message-ID: <aEmH5JbdwFopOU4g@U-2FWC9VHC-2323.local>
Date: Wed, 11 Jun 2025 21:43:00 +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 Tue, Jun 10, 2025 at 05:44:34PM +0200, Petr Mladek wrote:
[...]
> > > > 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...

My bad. It could be another my local change which follows your suggestion
of using a panic_console_replay() cleanup.

> > 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.

OK.

> 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.

Aha, right.

> Also I am not sure why there is the leading and
> trailing space.

I tried to give more space to avoid 'char names[strlen()+1]' 

> I would expect:
> 
> static const char sys_info_avail[] = "tasks,mem,timer,lock,ftrace,all_bt,blocked_tasks";

OK.

> > +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.

Yes, in 0001 patch, they are 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 forgot to remove it in code clean :)

> 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;

Good catch! thanks

> 			}
> 		}
> 	}
> 
> 	return si_bits;
> }

Will do.

> > +
> > +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.
 
Makes sense.

> 
> > +	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:

Will do.

> > +		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);

Thanks for the suggestion.


> > +		/* 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().

Yes, I referred the seccomp.c in many places.

> 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.

OK.

> 
> > +		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.

I think it's obviously better :). will change.

> 
> > +	}
> > +}
> > +
> >  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.

No problem! I really appreciate your review and suggestions, like
your help on reviewing my early 'panic_print' patches years ago.

Thanks,
Feng

Powered by blists - more mailing lists