Message-ID: <ZzYjd5TOvQjGNd1n@pathway.suse.cz>
Date: Thu, 14 Nov 2024 17:21:11 +0100
From: Petr Mladek <pmladek@...e.com>
To: Chris Down <chris@...isdown.name>
Cc: linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	John Ogness <john.ogness@...utronix.de>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Tony Lindgren <tony.lindgren@...ux.intel.com>, kernel-team@...com
Subject: Re: [PATCH v6 09/11] printk: Add sysctl interface to set global
 loglevels

On Mon 2024-10-28 16:45:55, Chris Down wrote:
> Introduce two new sysctl interfaces for configuring global loglevels:
> 
> - kernel.console_loglevel: Sets the global console loglevel, determining
>   the minimum priority of messages printed to consoles. Messages with a
>   loglevel lower than this value will be printed.
> - kernel.default_message_loglevel: Sets the default loglevel for
>   messages that do not specify an explicit loglevel.
> 
> The kernel.printk sysctl was previously used to set multiple loglevel
> parameters simultaneously, but it was confusing and lacked proper
> validation. By introducing these dedicated sysctl interfaces, we provide
> a clearer and more granular way to configure the loglevels.
> 
> Signed-off-by: Chris Down <chris@...isdown.name>
> ---
>  Documentation/admin-guide/sysctl/kernel.rst | 17 ++++++++-
>  kernel/printk/sysctl.c                      | 42 +++++++++++++++++++++
>  2 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index f8bc1630eba0..8019779b27f6 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -1044,6 +1044,20 @@ otherwise the 'doze' mode will be used.
>  
>  ==============================================================
>  
> +Some of these settings may be overridden per-console, see
> +Documentation/admin-guide/per-console-loglevel.rst. See ``man 2 syslog`` for
> +more information on the different loglevels.
> +
> +console_loglevel
> +================
> +
> +Messages with a higher priority than this will be printed to consoles.
> +
> +default_message_loglevel
> +========================
> +
> +Messages without an explicit priority will be printed with this priority.
> +
>  printk
>  ======
>  
> @@ -1052,8 +1066,7 @@ The four values in printk denote: ``console_loglevel``,
>  ``default_console_loglevel`` respectively.
>  
>  These values influence printk() behavior when printing or
> -logging error messages. See '``man 2 syslog``' for more info on
> -the different loglevels.
> +logging error messages.
>  
>  ======================== =====================================
>  console_loglevel         messages with a higher priority than
> diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
> index f5072dc85f7a..3bce8b89dacc 100644
> --- a/kernel/printk/sysctl.c
> +++ b/kernel/printk/sysctl.c
> @@ -11,6 +11,9 @@
>  
>  static const int ten_thousand = 10000;
>  
> +static int min_msg_loglevel = LOGLEVEL_EMERG;
> +static int max_msg_loglevel = LOGLEVEL_DEBUG;
> +
>  static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int write,
>  				void *buffer, size_t *lenp, loff_t *ppos)
>  {
> @@ -20,6 +23,29 @@ static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int writ
>  	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>  }
>  
> +static int printk_console_loglevel(const struct ctl_table *table, int write,
> +				   void *buffer, size_t *lenp, loff_t *ppos)

I would make it more clear that this function is using the procfs
based API and call it "proc_dointvec_console_loglevel()".

> +{
> +	struct ctl_table ltable = *table;
> +	int ret, level;
> +
> +	if (!write)
> +		return proc_dointvec(table, write, buffer, lenp, ppos);
> +
> +	ltable.data = &level;

Ah, I have missed that this is a copy and spent quite some time
wondering why it worked ;-) I remember that the same happened
last time I saw this trick.

It would deserve a comment for people like me. Or maybe, rename
the variable from ltable to table_copy.

Or I think about another solution, see below.

> +
> +	ret = proc_dointvec(&ltable, write, buffer, lenp, ppos);
> +	if (ret)
> +		return ret;
> +
> +	if (level != -1 && level != clamp_loglevel(level))
> +		return -ERANGE;
> +
> +	console_loglevel = level;

There is no locking. It seems that the original proc_dointvec code
handle this by using WRITE_ATOMIC(). It prevents compiler
optimizations. In particular, it makes sure that the entire value
will be updated in a single operation (atomically).

> +	return 0;
> +}
> +

I have mixed feelings. On one hand, the copy of the table entry looks
like a nice trick. On the other hand, I guess that this is the only
code using such a trick. It might make it more error prone when
some of the API internals change.

It seems that other users handle similar situations by
passing a custom @conv callback to do_proc_dointvec(),
for example, proc_dointvec_minmax(), proc_dointvec_jiffies().

This approach would require exporing do_proc_dointvec()
and do_proc_dointvec_conv(). But there already is a precedent
when do_proc_douintvec() is used in proc_dopipe_max_size().

I have tried to implement it to see how it looks. And I would
prefer to use it. Here is the updated patch:


>From 05a75d464276da24b7f4b7b97b982041607bbae2 Mon Sep 17 00:00:00 2001
From: Chris Down <chris@...isdown.name>
Date: Mon, 28 Oct 2024 16:45:55 +0000
Subject: [POC 09/11] printk: Add sysctl interface to set global loglevels

Introduce two new sysctl interfaces for configuring global loglevels:

- kernel.console_loglevel: Sets the global console loglevel, determining
  the minimum priority of messages printed to consoles. Messages with a
  loglevel lower than this value will be printed.
- kernel.default_message_loglevel: Sets the default loglevel for
  messages that do not specify an explicit loglevel.

The kernel.printk sysctl was previously used to set multiple loglevel
parameters simultaneously, but it was confusing and lacked proper
validation. By introducing these dedicated sysctl interfaces, we provide
a clearer and more granular way to configure the loglevels.

Signed-off-by: Chris Down <chris@...isdown.name>
Signed-off-by: Petr Mladek <pmladek@...e.com>
---
 Documentation/admin-guide/sysctl/kernel.rst | 17 ++++++-
 include/linux/sysctl.h                      |  7 +++
 kernel/printk/sysctl.c                      | 51 +++++++++++++++++++++
 kernel/sysctl.c                             |  4 +-
 4 files changed, 75 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index f8bc1630eba0..8019779b27f6 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -1044,6 +1044,20 @@ otherwise the 'doze' mode will be used.
 
 ==============================================================
 
+Some of these settings may be overridden per-console, see
+Documentation/admin-guide/per-console-loglevel.rst. See ``man 2 syslog`` for
+more information on the different loglevels.
+
+console_loglevel
+================
+
+Messages with a higher priority than this will be printed to consoles.
+
+default_message_loglevel
+========================
+
+Messages without an explicit priority will be printed with this priority.
+
 printk
 ======
 
@@ -1052,8 +1066,7 @@ The four values in printk denote: ``console_loglevel``,
 ``default_console_loglevel`` respectively.
 
 These values influence printk() behavior when printing or
-logging error messages. See '``man 2 syslog``' for more info on
-the different loglevels.
+logging error messages.
 
 ======================== =====================================
 console_loglevel         messages with a higher priority than
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index aa4c6d44aaa0..a297ca0d4096 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -237,6 +237,13 @@ extern struct ctl_table_header *register_sysctl_mount_point(const char *path);
 
 void do_sysctl_args(void);
 bool sysctl_is_alias(char *param);
+int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, int *valp,
+			  int write, void *data);
+int do_proc_dointvec(const struct ctl_table *table, int write,
+		     void *buffer, size_t *lenp, loff_t *ppos,
+		     int (*conv)(bool *negp, unsigned long *lvalp, int *valp,
+				 int write, void *data),
+		     void *data);
 int do_proc_douintvec(const struct ctl_table *table, int write,
 		      void *buffer, size_t *lenp, loff_t *ppos,
 		      int (*conv)(unsigned long *lvalp,
diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
index f5072dc85f7a..749e3575f2d1 100644
--- a/kernel/printk/sysctl.c
+++ b/kernel/printk/sysctl.c
@@ -11,6 +11,9 @@
 
 static const int ten_thousand = 10000;
 
+static int min_msg_loglevel = LOGLEVEL_EMERG;
+static int max_msg_loglevel = LOGLEVEL_DEBUG;
+
 static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int write,
 				void *buffer, size_t *lenp, loff_t *ppos)
 {
@@ -20,6 +23,38 @@ static int proc_dointvec_minmax_sysadmin(const struct ctl_table *table, int writ
 	return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 }
 
+static int do_proc_dointvec_console_loglevel(bool *negp, unsigned long *lvalp,
+					int *valp,
+					int write, void *data)
+{
+	int level, ret;
+
+	/*
+	 * If writing, first do so via a temporary local int so we can
+	 * bounds-check it before touching *valp.
+	 */
+	int *intp = write ? &level : valp;
+
+	ret = do_proc_dointvec_conv(negp, lvalp, intp, write, data);
+	if (ret)
+		return ret;
+
+	if (write) {
+		if (level != -1 && level != clamp_loglevel(level))
+			return -ERANGE;
+		WRITE_ONCE(*valp, level);
+	}
+
+	return 0;
+}
+
+static int proc_dointvec_console_loglevel(const struct ctl_table *table,
+		int write, void *buffer, size_t *lenp, loff_t *ppos)
+{
+	return do_proc_dointvec(table, write, buffer, lenp, ppos,
+				do_proc_dointvec_console_loglevel, NULL);
+}
+
 static struct ctl_table printk_sysctls[] = {
 	{
 		.procname	= "printk",
@@ -76,6 +111,22 @@ static struct ctl_table printk_sysctls[] = {
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_TWO,
 	},
+	{
+		.procname	= "console_loglevel",
+		.data		= &console_loglevel,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_console_loglevel,
+	},
+	{
+		.procname	= "default_message_loglevel",
+		.data		= &default_message_loglevel,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &min_msg_loglevel,
+		.extra2		= &max_msg_loglevel,
+	},
 };
 
 void __init printk_sysctl_init(void)
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 79e6cb1d5c48..225ef261d2fb 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -424,7 +424,7 @@ static void proc_put_char(void **buf, size_t *size, char c)
 	}
 }
 
-static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
+int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
 				 int *valp,
 				 int write, void *data)
 {
@@ -541,7 +541,7 @@ static int __do_proc_dointvec(void *tbl_data, const struct ctl_table *table,
 	return err;
 }
 
-static int do_proc_dointvec(const struct ctl_table *table, int write,
+int do_proc_dointvec(const struct ctl_table *table, int write,
 		  void *buffer, size_t *lenp, loff_t *ppos,
 		  int (*conv)(bool *negp, unsigned long *lvalp, int *valp,
 			      int write, void *data),
-- 
2.47.0

Powered by blists - more mailing lists