Message-ID: <YxB845DNwLTEUVgw@alley>
Date:   Thu, 1 Sep 2022 11:35:31 +0200
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>, kernel-team@...com
Subject: Re: [PATCH v3 2/2] printk: console: Support console-specific
 loglevels

On Wed 2022-07-20 18:48:16, Chris Down wrote:
> In terms of technical implementation, this patch embeds a device pointer
> in the console struct, and registers each console using it so we can
> expose attributes in sysfs.
>
> For information on the precedence and application of the new controls,
> see Documentation/ABI/testing/sysfs-class-console and
> Documentation/admin-guide/per-console-loglevel.rst.

The overall logic looks good to me. I finally have good feeling that
the behavior is "easy" to understand.

The detailed documentation is very nice!

See below for comments about various implementation details.

> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -137,6 +138,22 @@ static inline int con_debug_leave(void)
>  #define CON_BRL		(32) /* Used for a braille device */
>  #define CON_EXTENDED	(64) /* Use the extended output format a la /dev/kmsg */
>  
> +/*
> + * The loglevel for a console can be set in many places:
> + *
> + * 1. It can be forced to emit everything (ignore_loglevel);
> + * 2. It can be set globally (sysctls kernel.printk (deprecated),
> + *    kernel.console_loglevel, magic sysrq, loglevel= on kernel command line);
> + * 3. It can be locally set for this specific console (console=...,loglevel:N on
> + *    kernel command line, /sys/class/console/.../loglevel);
> + * 4. It can be set by a compile-time default
> + *    (CONFIG_CONSOLE_LOGLEVEL_{DEFAULT,QUIET})
> + *
> + * If case 3 happens, even if another global value in effect, CON_LOGLEVEL will
> + * be set.

The last sentence is not clear to me.

Well, I suggest to keep it simple and remove this comment
completely. The meaning of the flag is simple. It is set when
the local (per-console) loglevel is set.

The precedence of the various loglevel setting is explained above
console_effective_loglevel() where it belongs.

> + */
> +#define CON_LOGLEVEL	(128) /* Level set locally for this console */

I would write:

#define CON_LOGLEVEL	(128) /* Local (per-console) loglevel is set. */

Alternatively we could avoid the flag completely. The per-console
loglevel is set when con->level > 0. A valid value must never
be below CONSOLE_MIN_LOGLEVEL which is 1. And it is perfectly fine
to say that 0 or -1 is not a valid loglevel. The same effect could
be achieved by disabling the console completely.

I do not have strong opinion. The flag has obvious meaning and might
make the code better readable. On the other hand, it adds an extra
code and complexity.

I slightly prefer to do it without the flag.

Anyway, if we add the new flag, we should also show it in
/proc/consoles, see fs/proc/consoles.c.

> +
>  struct console {
>  	char	name[16];
>  	void	(*write)(struct console *, const char *, unsigned);
> @@ -155,8 +172,15 @@ struct console {
>  	unsigned long dropped;
>  	void	*data;
>  	struct	 console *next;
> +	int	level;
> +	struct	device *classdev;
>  };
>  
> +static inline struct console *classdev_to_console(struct device *dev)
> +{
> +	return dev_get_drvdata(dev);
> +}

Please, open code this in the _show()/_store() callbacks. dev_get_drvdata()
seems to be the standard way how it is done. Hiding it into a custom
function just adds an extra step when reading the code.

> +
>  /*
>   * for_each_console() allows you to iterate on each console
>   */
> diff --git a/kernel/printk/console_cmdline.h b/kernel/printk/console_cmdline.h
> index 3ca74ad391d6..40f1a1ff0965 100644
> --- a/kernel/printk/console_cmdline.h
> +++ b/kernel/printk/console_cmdline.h
> @@ -6,6 +6,8 @@ struct console_cmdline
>  {
>  	char	name[16];			/* Name of the driver	    */
>  	int	index;				/* Minor dev. to use	    */
> +	int	level;				/* Log level to use */
> +	short	flags;				/* Initial flags */
>  	bool	user_specified;			/* Specified by command line vs. platform */
>  	char	*options;			/* Options for the driver   */
>  #ifdef CONFIG_A11Y_BRAILLE_CONSOLE
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 6094f773ad4a..6f5e29b60875 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1202,9 +1217,72 @@ module_param(ignore_loglevel, bool, S_IRUGO | S_IWUSR);
>  MODULE_PARM_DESC(ignore_loglevel,
>  		 "ignore loglevel setting (prints all kernel messages to the console)");
>  
> -static bool suppress_message_printing(int level)
> +static bool __read_mostly ignore_per_console_loglevel;
> +
> +static int __init ignore_per_console_loglevel_setup(char *str)
> +{
> +	ignore_per_console_loglevel = true;
> +	return 0;
> +}
> +
> +early_param("ignore_per_console_loglevel", ignore_per_console_loglevel_setup);
> +module_param(ignore_per_console_loglevel, bool, 0644);
> +MODULE_PARM_DESC(ignore_per_console_loglevel,
> +		 "ignore per-console loglevel setting (only respect global console loglevel)");
> +
> +/*
> + * Hierarchy of loglevel authority:
> + *
> + * 1. con->level. The locally set, console-specific loglevel. Optional, only
> + *    valid if the CON_LOGLEVEL flag is set.
> + * 2. console_loglevel. The default global console loglevel, always present.
> + *
> + * The behaviour can be further changed by the following printk module
> + * parameters:
> + *
> + * 1. ignore_loglevel. Can be set at boot or at runtime with
> + *    /sys/module/printk/parameters/ignore_loglevel. Overrides absolutely
> + *    everything since it's used to debug.
> + * 2. ignore_per_console_loglevel. Existing per-console loglevel values are left
> + *    intact, but are ignored in favour of console_loglevel as long as this is
> + *    true.
> + *
> + * Callers typically only need the level _or_ the source, but they're both
> + * emitted from this function so that the effective loglevel logic can be
> + * kept in one place.
> + */
> +static int console_effective_loglevel(const struct console *con,
> +				      enum loglevel_source *source)
> +{
> +	enum loglevel_source lsource;
> +	int level;
> +
> +	if (ignore_loglevel) {
> +		lsource = LLS_IGNORE_LOGLEVEL;
> +		level = CONSOLE_LOGLEVEL_MOTORMOUTH;
> +		goto out;
> +	}
> +
> +	if (!ignore_per_console_loglevel &&
> +	    (con && (con->flags & CON_LOGLEVEL))) {
> +		lsource = LLS_LOCAL;
> +		level = con->level;
> +		goto out;
> +	}
> +
> +	lsource = LLS_GLOBAL;
> +	level = console_loglevel;
> +
> +out:
> +	*source = lsource;
> +	return level;
> +}

It might be a matter of taste. But I would probably do it the
following way (note that these would not be used in
boot_delay_msec()):

static int console_effective_loglevel(const struct console *con)
{
	enum loglevel_source source;
	int level;

	if (WARN_ON_ONCE(!con))
		return;

	source = console_effective_loglevel_source(con);

	switch (source) {
	case LLS_IGNORE_LOGLEVEL:
		level = CONSOLE_LOGLEVEL_MOTORMOUTH;
		break;
	case LSS_LOCAL:
		level = con->level;
		break;
	case LSS_GLOBAL:
		level = console_loglevel;
		break;
	default:
		pr_warn("Unhandled console loglevel source: %d,	source);
		level = default_console_loglevel;
		break;
	}

	return level;
}

static const char *console_effective_loglevel_source_str(const struct *con)
{
	enum loglevel_source source;
	const char *str;

	if (WARN_ON_ONCE(!con))
		return;

	source = console_effective_loglevel_source(con);

	switch (source) {
	case LLS_IGNORE_LOGLEVEL:
		str = "ignore_loglevel";
		break;
	case LSS_LOCAL:
		str = "local"
		break;
	case LSS_GLOBAL:
		str = "global";
		break;
	default:
		pr_warn("Unhandled console loglevel source: %d,	source);
		str = "unknown";
		break;
	}

	return str;
}

static enum loglevel_source
console_effective_loglevel_source(const struct console *con)
{
	if (WARN_ON_ONCE(!con))
		return;

	if (ignore_loglevel)
		return LLS_IGNORE_LOGLEVEL;

	if (con->flags & CON_LOGLEVEL && !ignore_per_console_loglevel))
		return LLS_LOCAL;

	return LLS_GLOBAL;
}

It looks like a bit cleaner and better separated (layered) logic.

There is no need to define "enum loglevel_source" variable when
the caller is interested only into the loglevel value.

The advantage of console_effective_loglevel_source_str() is that it
always returns a valid string. It prevents a potential out-of-bound
access to loglevel_source_names[].

> +
> +static bool suppress_message_printing(int level, struct console *con)
>  {
> -	return (level >= console_loglevel && !ignore_loglevel);
> +	enum loglevel_source source;
> +
> +	return level >= console_effective_loglevel(con, &source);
>  }
>  
>  #ifdef CONFIG_BOOT_PRINTK_DELAY
> @@ -1236,7 +1314,7 @@ static void boot_delay_msec(int level)
>  	unsigned long timeout;
>  
>  	if ((boot_delay == 0 || system_state >= SYSTEM_RUNNING)
> -		|| suppress_message_printing(level)) {
> +		|| suppress_message_printing(level, NULL)) {

This does not take into account per-console loglevels.
And we could not check them in vprintk_emit() because
we could not take console_lock() there.

AFAIK, the purpose of this call is to allow reading the messages
on consoles when they do not support scrolling.

A solution would be to call boot_delay_msec() in
console_flush_all(). It would need adding parameter
into console_emit_next_record() that pass information
whether it emitted or suppressed the message.
Something like:

/*
[...]
 * @emitted will be set to "true" when the message was really emitted to the
 * console. It means that it was not suppressed because of console loglevel.
[...]
*/
static bool console_emit_next_record(struct console *con, char *text, char *ext_text,
				     char *dropped_text, bool *emitted, bool *handover)


>  		return;
>  	}
>  
> @@ -1701,12 +1806,14 @@ int do_syslog(int type, char __user *buf, int len, int source)
>  		break;
>  	/* Disable logging to console */
>  	case SYSLOG_ACTION_CONSOLE_OFF:
> +		warn_on_local_loglevel();
>  		if (saved_console_loglevel == LOGLEVEL_DEFAULT)
>  			saved_console_loglevel = console_loglevel;
>  		console_loglevel = minimum_console_loglevel;
>  		break;

We actually could disable logging on all consoles by setting
ignore_per_console_loglevel. Something like:

	case SYSLOG_ACTION_CONSOLE_OFF:
		if (saved_console_loglevel == LOGLEVEL_DEFAULT) {
			saved_console_loglevel = console_loglevel;
			saved_ignore_per_console_loglevel = ignore_per_console_loglevel;
		}
		console_loglevel = minimum_console_loglevel;
		ignore_per_console_loglevel = true;
		break;



>  	/* Enable logging to console */
>  	case SYSLOG_ACTION_CONSOLE_ON:
> +		warn_on_local_loglevel();
>  		if (saved_console_loglevel != LOGLEVEL_DEFAULT) {
>  			console_loglevel = saved_console_loglevel;
>  			saved_console_loglevel = LOGLEVEL_DEFAULT;

and here:

	case SYSLOG_ACTION_CONSOLE_ON:
		if (saved_console_loglevel != LOGLEVEL_DEFAULT) {
			console_loglevel = saved_console_loglevel;
			ignore_per_console_loglevel = saved_ignore_per_console_loglevel;
			saved_console_loglevel = LOGLEVEL_DEFAULT;
		}


> @@ -1714,6 +1821,7 @@ int do_syslog(int type, char __user *buf, int len, int source)
>  		break;
>  	/* Set level of messages printed to console */
>  	case SYSLOG_ACTION_CONSOLE_LEVEL:
> +		warn_on_local_loglevel();

I would keep it simple:

		if (!ignore_per_console_loglevel)
			pr_warn_once("SYSLOG_ACTION_CONSOLE_LEVEL is ignored by consoles with explicitely set per-console loglevel, see Documentation/admin-guide/per-console-loglevel\n");

People should know that this API has limits. The check of
ignore_per_console_loglevel allows to prevent the warning
when the users are not interested into the per-console
loglevels feature.

I think about opening discussion about obsoleting the syslog
syscall. It has many drawbacks. Everything is stored in global
variables. As a result, it supports only one reader and one
writer.


>  		if (len < 1 || len > 8)
>  			return -EINVAL;
>  		if (len < minimum_console_loglevel)
> @@ -3030,6 +3149,145 @@ static int __init keep_bootcon_setup(char *str)
>  
>  early_param("keep_bootcon", keep_bootcon_setup);
>  
> +#ifdef CONFIG_PRINTK
> +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct console *con = classdev_to_console(dev);
> +
> +	if (con->flags & CON_LOGLEVEL)
> +		return sysfs_emit(buf, "%d\n", con->level);
> +	else
> +		return sysfs_emit(buf, "unset\n");

I can't find any other sysfs interface using this style (number or
"unset"). Or any other interface combining number or a string values.

I feel a bit uneasy to introduce a completely new semantic. Instead I
prefer using either -1 or 0 when the per-console is not set. It might
be slightly less obvious but it is a standard way so it might be
easier for users in the end.

Both 0 and -1 values are usable as explained above. I personally
prefer -1 because the meaning is more obvious. I would even
use it instead of CON_LOGLEVEL flag.


> +}
> +
> +static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t size)
> +{
> +	struct console *con = classdev_to_console(dev);
> +	ssize_t ret;
> +	int tmp;
> +
> +	if (!strcmp(buf, "unset") || !strcmp(buf, "unset\n")) {
> +		con->flags &= ~CON_LOGLEVEL;
> +		return size;
> +	}
> +
> +	ret = kstrtoint(buf, 10, &tmp);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (tmp < LOGLEVEL_EMERG || tmp > LOGLEVEL_DEBUG + 1)
> +		return -ERANGE;
> +
> +	if (tmp < minimum_console_loglevel)
> +		return -EINVAL;

This looks superfluous. Please, use minimum_console_loglevel
instead of LOGLEVEL_EMERG in the above range check.

Also, we should make a helper function for this. The same range check
is useful also for the global console_loglevel.

> +
> +	con->level = tmp;
> +	con->flags |= CON_LOGLEVEL;
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR_RW(loglevel);
> +
[...]
> +static struct attribute *console_sysfs_attrs[] = {
> +	&dev_attr_loglevel.attr,
> +	&dev_attr_effective_loglevel_source.attr,
> +	&dev_attr_effective_loglevel.attr,
> +	&dev_attr_enabled.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(console_sysfs);
> +
> +static void console_classdev_release(struct device *dev)
> +{
> +	kfree(dev);
> +}
> +
> +static void console_register_device(struct console *new)

Please use "con" instead of "new" like it is done in the other
API manipulating struct console. It helps when reading the code.

> +{
> +	/*
> +	 * 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;
> +

We should check whether new->classdev is NULL to prevent double
initialization. It should not happen but better be on the safe side.


> +	new->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
> +	if (!new->classdev)
> +		return;
> +
> +	device_initialize(new->classdev);
> +	dev_set_name(new->classdev, "%s", new->name);

This should be:

	dev_set_name(new->classdev, "%s%d", con->name, con->index);

It should match console names defined on the command line
and shown by /proc/consoles. See how the name is printed in
fs/proc/consoles.c.

> +	dev_set_drvdata(new->classdev, new);
> +	new->classdev->release = console_classdev_release;
> +	new->classdev->class = console_class;
> +	if (device_add(new->classdev))
> +		put_device(new->classdev);
> +}
> +
> +static void console_setup_class(void)
> +{
> +	struct console *con;
> +
> +	/*
> +	 * 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(THIS_MODULE, "console");
> +	if (!IS_ERR(console_class))
> +		console_class->dev_groups = console_sysfs_groups;
> +
> +	for_each_console(con)
> +		console_register_device(con);

This should be done under console_lock so that the list of registered
console could not get manipulated.

> +}
> +#else /* CONFIG_PRINTK */
> +static void console_register_device(struct console *new) {}
> +static void console_setup_class(void) {}
> +#endif
> +
>  /*
>   * This is called by register_console() to try to match
>   * the newly registered console with any of the ones selected
> --- a/kernel/printk/sysctl.c
> +++ b/kernel/printk/sysctl.c
> @@ -7,10 +7,14 @@
>  #include <linux/printk.h>
>  #include <linux/capability.h>
>  #include <linux/ratelimit.h>
> +#include <linux/console.h>
>  #include "internal.h"
>  
>  static const int ten_thousand = 10000;
>  
> +static int min_loglevel = LOGLEVEL_EMERG;

We should use minimum_console_loglevel instead.

> +static int max_loglevel = LOGLEVEL_DEBUG + 1;

Please, define maximum_console_loglevel in kernel/printk/printk.c
after the console_printk[4] array.

This will allow to create a function for the range check that might
be used for both sysfs and proc interface.

Also I would set the maximal value to CONSOLE_LOGLEVEL_MOTORMOUTH.

> +
>  static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
>  				void *buffer, size_t *lenp, loff_t *ppos)
>  {
> @@ -76,6 +122,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	= printk_console_loglevel,
> +	},
> +	{
> +		.procname	= "default_message_loglevel",
> +		.data		= &default_message_loglevel,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &min_loglevel,
> +		.extra2		= &max_loglevel,
> +	},

Is there any chance to add this into /sys/class/console instead?
I mean:

	/sys/class/console/loglevel
	/sys/class/console/default_message_loglevel

It would be nice to have the things are on the same place.
Especially it would be nice to have the global loglevel there.

Best Regards,
Petr

Powered by blists - more mailing lists