Message-ID: <neafilvcdq6btghci5ru7ouvcubvmt6udxuw2nwbaxitmcaqsb@aiud6hw3joj5>
Date: Fri, 10 Jan 2025 11:09:04 +0100
From: Joel Granados <joel.granados@...nel.org>
To: Petr Mladek <pmladek@...e.com>
Cc: Chris Down <chris@...isdown.name>, 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: Re: [PATCH v6 09/11] printk: Add sysctl interface to set global
 loglevels

On Thu, Nov 14, 2024 at 05:21:11PM +0100, Petr Mladek wrote:
> 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
The "copy the table" approach is used in sysctl_max_threads. And I'm
working on another for sysrq. In these two cases, it is used because
data is NULL; which is not applicable to your case. Just thought I
mention it as another case for its use.

> 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 is correct, But they all exist within kernel/sysctl.c; which we are
trying to make smaller by moving the non-sysctl logic out. I have not
gotten to these functions yet in my "move things away from
kerne/sysctl.c" proces, but I believe that your proposal of exporting
the do_proc_* functions is a good one for the users that are within the
kernel dir.

> 
> 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:
This seems like a good approach. Is there a new Version of this patch
with this in it?

I arrived a bit late to the review, hope it is still relevant.

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

-- 

Joel Granados

Powered by blists - more mailing lists