Message-ID: <ZzTMrFEcYZf58aqj@pathway.suse.cz>
Date: Wed, 13 Nov 2024 16:58:36 +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 06/11] printk: console: Introduce sysfs interface for
 per-console loglevels

On Mon 2024-10-28 16:45:46, Chris Down wrote:
> A sysfs interface under /sys/class/console/ is created that permits
> viewing and configuring per-console attributes. This is the main
> interface with which we expect users to interact with and configure
> per-console loglevels.
> 
> Each console device now has its own directory (for example,
> /sys/class/console/ttyS0/) containing the following attributes:
> 
> - effective_loglevel (ro): The effective loglevel for the console after
>   considering all loglevel authorities (e.g., global loglevel,
>   per-console loglevel).
> - effective_loglevel_source (ro): The source of the effective loglevel
>   (e.g., local, global, ignore_loglevel).
> - enabled (ro): Indicates whether the console is enabled (1) or disabled
>   (0).
> - loglevel (rw): The per-console loglevel. Writing a value between 0
>   (KERN_EMERG) and 8 (KERN_DEBUG + 1) sets the per-console loglevel.
>   Writing -1 disables the per-console loglevel.

The function clamp_loglevel() uses "minimum_console_loglevel"
as the minimal boundary. This variable is initialized to
CONSOLE_LOGLEVEL_MIN which is defined as 1.

And indeed, the value 0 is not accepted:

  # echo 0 >/sys/class/console/ttyS0/loglevel 
  -bash: echo: write error: Numerical result out of range

CONSOLE_LOGLEVEL_MIN has been used since ages. I am not completely
sure about the motivation. I guess that KERN_EMERG messages
should never get disabled.

I would keep "1" as the minimal allowed value and update
the documentation.


> In terms of technical implementation, we embed a device pointer in the
> console struct, and register each console using it so we can expose
> attributes in sysfs. We currently expose the following attributes:
> 
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-console
[...]
> +What:		/sys/class/console/<C>/loglevel
> +Date:		October 2024
> +Contact:	Chris Down <chris@...isdown.name>
> +Description:	Read write. The current per-console loglevel, which will take
> +		effect if not overridden by other non-sysfs controls (see
> +		Documentation/admin-guide/per-console-loglevel.rst). Bounds are
> +		0 (LOGLEVEL_EMERG) to 8 (LOGLEVEL_DEBUG + 1) inclusive. Also

The real minimal boundary is 1.

> +		takes the special value "-1" to indicate that no per-console
> +		loglevel is set, and we should defer to the global controls.
> diff --git a/Documentation/admin-guide/per-console-loglevel.rst b/Documentation/admin-guide/per-console-loglevel.rst
> index 1ec7608f94b0..41bf3befb2f3 100644
> --- a/Documentation/admin-guide/per-console-loglevel.rst
> +++ b/Documentation/admin-guide/per-console-loglevel.rst
> @@ -68,3 +68,44 @@ further:
>  The default value for ``kernel.console_loglevel`` comes from
>  ``CONFIG_CONSOLE_LOGLEVEL_DEFAULT``, or ``CONFIG_CONSOLE_LOGLEVEL_QUIET`` if
>  ``quiet`` is passed on the kernel command line.
> +
> +Console attributes
> +~~~~~~~~~~~~~~~~~~
> +
> +Registered consoles are exposed at ``/sys/class/console``. For example, if you
> +are using ``ttyS0``, the console backing it can be viewed at
> +``/sys/class/console/ttyS0/``. The following files are available:
> +
> +* ``effective_loglevel`` (r): The effective loglevel after considering all
> +  loglevel authorities. For example, it shows the value of the console-specific
> +  loglevel when a console-specific loglevel is defined, and shows the global
> +  console loglevel value when the console-specific one is not defined.
> +
> +* ``effective_loglevel_source`` (r): The loglevel authority which resulted in
> +  the effective loglevel being set. The following values can be present:
> +
> +    * ``local``: The console-specific loglevel is in effect.
> +
> +    * ``global``: The global loglevel (``kernel.console_loglevel``) is in
> +      effect. Set a console-specific loglevel to override it.
> +
> +    * ``ignore_loglevel``: ``ignore_loglevel`` was specified on the kernel
> +      command line or at ``/sys/module/printk/parameters/ignore_loglevel``.
> +      Disable it to use level controls.
> +
> +* ``enabled`` (r): Whether the console is enabled.

Please, remove the 'enabled' flag for now. All registered consoles are
enabled. It seems that only some serial consoles temporary disable
consoles during the suspend but the sysfs interface is not accessible
at this time anyway.

It has been discussed recently, see
https://lore.kernel.org/r/ZyoNZfLT6tlVAWjO@pathway.suse.cz

> +* ``loglevel`` (rw): The local, console-specific loglevel for this console.
> +  This will be in effect if no other global control overrides it. Look at
> +  ``effective_loglevel`` and ``effective_loglevel_source`` to verify that.
> +
> +Deprecated
> +~~~~~~~~~~
> +
> +* ``kernel.printk`` sysctl: this takes four values, setting
> +  ``kernel.console_loglevel``, ``kernel.default_message_loglevel``, the minimum
> +  console loglevel, and a fourth unused value. The interface is generally
> +  considered to quite confusing, doesn't perform checks on the values given,
> +  and is unaware of per-console loglevel semantics.
> +
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -77,6 +77,8 @@ extern bool ignore_per_console_loglevel;
>  
>  extern void console_verbose(void);
>  
> +int clamp_loglevel(int level);
> +

It is declared also in kernel/printk/internal.h. And it seems
that the internal header is enough.

>  /* strlen("ratelimit") + 1 */
>  #define DEVKMSG_STR_MAX_SIZE 10
>  extern char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE];
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -199,6 +199,12 @@ static int __init control_devkmsg(char *str)
>  }
>  __setup("printk.devkmsg=", control_devkmsg);
>  
> +int clamp_loglevel(int level)
> +{
> +	return clamp(level, minimum_console_loglevel,
> +		     CONSOLE_LOGLEVEL_MOTORMOUTH);

Documentation/ABI/testing/sysfs-class-console says:

   "Bounds are 1 (LOGLEVEL_ALERT) to 8 (LOGLEVEL_DEBUG + 1) inclusive."

I do not have strong opinion. But I would follow the documentation
because the limit is well defined there. On the contrary,
CONSOLE_LOGLEVEL_MOTORMOUTH is a randomly chosen value
and we should not leak it to the userspace ;-)

> +}
> +
>  char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE] = "ratelimit";
>  #if defined(CONFIG_PRINTK) && defined(CONFIG_SYSCTL)
>  int devkmsg_sysctl_set_loglvl(const struct ctl_table *table, int write,
> @@ -3894,6 +3900,8 @@ static int try_enable_preferred_console(struct console *newcon,
>  			// TODO: Will be configurable in a later patch
>  			newcon->level = -1;
>  
> +			newcon->classdev = NULL;
> +

The console can be enabled also by try_enable_default_console().
We need to initialize newcon->classdev there as well.

The same is true for newcon->level. I have missed this when
reviewing the 3rd patch.

>  			if (_braille_register_console(newcon, c))
>  				return 0;
>  
> --- /dev/null
> +++ b/kernel/printk/sysfs.c
> +void console_register_device(struct console *con)
> +{
> +	/*
> +	 * We might be called from register_console() before the class is
> +	 * registered. If that happens, we'll take care of it in
> +	 * printk_late_init.
> +	 */
> +	if (IS_ERR_OR_NULL(console_class))
> +		return;

This check is not enough to avoid race with printk_late_init():

CPU0					CPU1

register_console()			printk_late_init()
  [...]					[...]
  console_register_device()		console_register_device()
    if (IS_ERR_OR_NULL(console_class))    if (IS_ERR_OR_NULL(console_class))

BANG: Both CPUs see "console == NULL" and both CPUs try to
      register the device.

I suggest to avoid this race by taking console_list_lock() in
printk_late_init(), see below.

> +
> +	if (WARN_ON(con->classdev))
> +		return;
> +
> +	con->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
> +	if (!con->classdev)
> +		return;
> +
> +	device_initialize(con->classdev);
> +	dev_set_name(con->classdev, "%s%d", con->name, con->index);
> +	dev_set_drvdata(con->classdev, con);
> +	con->classdev->release = console_classdev_release;
> +	con->classdev->class = console_class;
> +	if (device_add(con->classdev))
> +		put_device(con->classdev);
> +}
> +
> +void console_setup_class(void)
> +{
> +	struct console *con;
> +	int cookie;
> +
> +	/*
> +	 * printk exists for the lifetime of the kernel, it cannot be unloaded,
> +	 * so we should never end up back in here.
> +	 */
> +	if (WARN_ON(console_class))
> +		return;
> +
> +	console_class = class_create("console");
> +	if (!IS_ERR(console_class))
> +		console_class->dev_groups = console_sysfs_groups;
> +
> +	cookie = console_srcu_read_lock();

We should take console_list_lock() here to avoid races with
register_console() and unregister_console(). Otherwise.
console_register_device(con) might be called twice.

> +	for_each_console_srcu(con)
> +		console_register_device(con);
> +	console_srcu_read_unlock(cookie);
> +}
> +#else /* CONFIG_PRINTK */
> +void console_register_device(struct console *new)
> +{
> +}
> +void console_setup_class(void)
> +{
> +}
> +#endif

Best Regards,
Petr

Powered by blists - more mailing lists