Re: [PATCH 3/5] panic: add 'panic_sys_info' sysctl to take human readable string parameter

[Feng Tang <feng.tang@linux.alibaba.com>]

On Tue, Aug 12, 2025 at 12:23:07PM +0200, Petr Mladek wrote:
> On Thu 2025-07-03 10:10:02, Feng Tang wrote:
> > Bitmap definition for 'panic_print' is hard to remember and decode.
> > Add 'panic_sys_info='sysctl to take human readable string like
> > "tasks,mem,timers,locks,ftrace,..." and translate it into bitmap.
> > 
> > The detailed mapping is:
> > 	SYS_INFO_TASKS		"tasks"
> > 	SYS_INFO_MEM		"mem"
> > 	SYS_INFO_TIMERS		"timers"
> > 	SYS_INFO_LOCKS		"locks"
> > 	SYS_INFO_FTRACE		"ftrace"
> > 	SYS_INFO_ALL_CPU_BT	"all_bt"
> > 	SYS_INFO_BLOCKED_TASKS	"blocked_tasks"
> > 
> > --- a/lib/sys_info.c
> > +++ b/lib/sys_info.c
> > +static const char sys_info_avail[] = "tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks";
> > +
> > +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];
> 
> The "+ 1" looks superfluous.
> 
> I guess that it is for the trailing '\0'. But sys_info_avail[] already
> includes the trailing '\0' so it should be already counted by the sizeof().
> 
> Note that it would be needed with strlen(). But it should not be
> needed with sizeof().

Yes, you are right. Will remove it.

> > +	struct ctl_table table;
> > +	unsigned long *si_bits_global;
> > +
> > +	si_bits_global = ro_table->data;
> > +
> > +	if (write) {
> > +		unsigned long si_bits;
> > +		int ret;
> > +
> > +		table = *ro_table;
> > +		table.data = names;
> > +		table.maxlen = sizeof(names);
> > +		ret = proc_dostring(&table, write, buffer, lenp, ppos);
> > +		if (ret)
> > +			return ret;
> > +
> > +		si_bits = sys_info_parse_param(names);
> > +		/* The access to the global value is not synchronized. */
> > +		WRITE_ONCE(*si_bits_global, si_bits);
> > +		return 0;
> > +	} else {
> > +		/* for 'read' operation */
> > +		char *delim = "";
> > +		int i, len = 0;
> > +
> 
> It looks to me that names[] can later be used non-initialized when
> *si_bits_global == 0. We should initialized it here, something like:
> 
> 		names[0] = '\0';

Good catch! Will add this.

> > +		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> > +			if (*si_bits_global & si_names[i].bit) {
> > +				len += scnprintf(names + len, sizeof(names) - len,
> > +					"%s%s", delim, si_names[i].name);
> > +				delim = ",";
> > +			}
> > +		}
> > +
> > +		table = *ro_table;
> > +		table.data = names;
> > +		table.maxlen = sizeof(names);
> > +		return proc_dostring(&table, write, buffer, lenp, ppos);
> > +	}
> > +}
> > +#endif
> 
> Otherwise, it looks good.
 
Thanks for reviewing the patchset!

- Feng

> Best Regards,
> Petr