Re: [PATCH V2 3/5] sys_info: add help to translate sys_info string to bitmap

[Petr Mladek <pmladek@suse.com>]

On Mon 2025-06-16 09:08:38, Feng Tang wrote:
> Bitmap definition is hard to remember and decode. Add sysctl interface
> to translate human readable string like "tasks,mem,timer,lock,ftrace,..."
> to bitmap.
> 
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com>
> ---
>  include/linux/sys_info.h |  7 +++
>  lib/sys_info.c           | 97 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/include/linux/sys_info.h b/include/linux/sys_info.h
> index 79bf4a942e5f..0b49863cd414 100644
> --- a/include/linux/sys_info.h
> +++ b/include/linux/sys_info.h
> @@ -16,5 +16,12 @@
>  #define SYS_SHOW_BLOCKED_TASKS		0x00000080
>  
>  extern void sys_show_info(unsigned long info_mask);
> +extern unsigned long sys_info_parse_param(char *str);
>  
> +#ifdef CONFIG_SYSCTL
> +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
>  #endif	/* _LINUX_SYS_INFO_H */
> diff --git a/lib/sys_info.c b/lib/sys_info.c
> index 90a79b5164c9..9693542435ba 100644
> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> @@ -5,6 +5,103 @@
>  #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";
> +
> +static const struct sys_info_name  si_names[] = {
> +	{ SYS_SHOW_TASK_INFO,	"tasks" },
> +	{ SYS_SHOW_MEM_INFO,	"mem" },
> +	{ SYS_SHOW_TIMER_INFO,	"timer" },

I see that the naming is a bit inconsistent in using singulars
vs. plurals. I suggest to use plulars when it makes sense.
It means here:

	{ SYS_SHOW_TIMERS_INFO,	"timers" },

> +	{ SYS_SHOW_LOCK_INFO,	"lock" },

and here

	{ SYS_SHOW_LOCKS_INFO,	"locks" },

> +	{ SYS_SHOW_FTRACE_INFO, "ftrace" },
> +	{ SYS_SHOW_ALL_CPU_BT,	"all_bt" },
> +	{ SYS_SHOW_BLOCKED_TASKS, "blocked_tasks" },
> +};

[...]

> +#ifdef CONFIG_SYSCTL
> +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];
> +	struct ctl_table table;
> +	unsigned long *si_bits_global;
> +	int i, ret, len;
> +
> +	si_bits_global = ro_table->data;
> +
> +	if (write) {
> +		unsigned long si_bits;
> +
> +		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.
> +		 */

Nit, the comment fits on a single line. I would use:

		/* The access to the global value is not synchronized. */

> +		WRITE_ONCE(*si_bits_global, si_bits);
> +		return 0;
> +	} else {
> +		/* for 'read' operation */
> +		bool first = true;
> +		char *buf;
> +
> +		buf = names;
> +		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> +			if (*si_bits_global & si_names[i].bit) {
> +
> +				if (first) {
> +					first = false;
> +				} else {
> +					*buf = ',';
> +					buf++;
> +				}
> +
> +				len = strlen(si_names[i].name);
> +				strncpy(buf, si_names[i].name, len);

I am afraid of a buffer overflow when people add new entry into
si_names[] table but they forget to update sys_info_avail[] string.
I would prefer to limit the write to the buffer by the size of the buffer.

Something like (on top of this patch):

--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -50,7 +50,7 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 	char names[sizeof(sys_info_avail) + 1];
 	struct ctl_table table;
 	unsigned long *si_bits_global;
-	int i, ret, len;
+	int i, ret;
 
 	si_bits_global = ro_table->data;
 
@@ -72,27 +72,18 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
 		return 0;
 	} else {
 		/* for 'read' operation */
-		bool first = true;
-		char *buf;
+		char *delim = "";
+		int len = 0;
 
-		buf = names;
+		names[0] = '\0';
 		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
 			if (*si_bits_global & 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;
+				len += scnprintf(names + len, sizeof(names) - len,
+						 "%s%s", delim, si_names[i].name);
+				delim = ",";
 			}
 
 		}
-		*buf = '\0';
 
 		table = *ro_table;
 		table.data = names;

Best Regards,
Petr