linux-kernel - Re: [PATCH v6 03/11] printk: console: Implement core per-console loglevel infrastructure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZzMtCVlLVLnwghQB@pathway.suse.cz>
Date: Tue, 12 Nov 2024 11:25:38 +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 03/11] printk: console: Implement core per-console
 loglevel infrastructure

On Fri 2024-11-08 17:10:31, Petr Mladek wrote:
> On Mon 2024-10-28 16:45:37, Chris Down wrote:
> It would be nice to describe the basic logic in the commit message.
> And also the handling when the global console_loglevel is modified
> via sysrq. Something like:
> 
> <proposal>
> This commit adds the internal infrastructure to support per-console
> log levels, which will be configurable through sysfs and the kernel
> command line in future commits.
> 
> The global `console_loglevel` is preserved and used as the default log
> level for all consoles. Each console can override this global level
> with its own specific log level stored in `struct console`. To
> override the global level, the per-console log level must be greater
> than 0; otherwise, the default value of -1 ensures the global level
> is used.
> 
> The existing `ignore_loglevel` command line parameter will override
> both the global and per-console log levels.
> </proposal>
> 
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -1287,9 +1287,62 @@ module_param(ignore_loglevel, bool, S_IRUGO | S_IWUSR);
> > +enum loglevel_source
> > +console_effective_loglevel_source(const struct console *con)
> > +{
> > +	if (WARN_ON_ONCE(!con))
> > +		return LLS_GLOBAL;
> > +
> > +	if (ignore_loglevel)
> > +		return LLS_IGNORE_LOGLEVEL;
> > +
> > +	if (per_console_loglevel_is_set(con))
> > +		return LLS_LOCAL;
> > +
> > +	return LLS_GLOBAL;
> > +}
> 
> The @con parameter is nice. But it makes it hard to add lockdep
> checks. So, I suggest to pass the console specific loglevel
> as the parameter, like:
> 
> enum loglevel_source
> console_effective_loglevel_source(int con_level)
> {
> 	if (ignore_loglevel)
> 		return LLS_IGNORE_LOGLEVEL;
> 
> 	if (is_valid_per_console_loglevel(con_level))
> 		return LLS_LOCAL;
> 
> 	return LLS_GLOBAL;
> }
> 
> int console_effective_loglevel(int con_level)
> {
> 	enum loglevel_source source;
> 	int level;
> 
> 	source = console_effective_loglevel_source(con_level);
> 
> 	switch (source) {
> 	case LLS_IGNORE_LOGLEVEL:
> 		level = CONSOLE_LOGLEVEL_MOTORMOUTH;
> 		break;
> 	case LLS_LOCAL:
> 		level = con_level;
> 		break;
> 	case LLS_GLOBAL:
> 		level = console_level;
> 		break;
> 	default:
> 		pr_warn("Unhandled console loglevel source: %d", source);
> 		level = console_level;
> 		break;
> 	}
> 
> 	return level;
> }
> 
> static bool suppress_message_printing(int level, int con_level)
> {
> 	return level >= console_effective_loglevel(con_level);
> }
> 
> >  
> >  #ifdef CONFIG_BOOT_PRINTK_DELAY
> > @@ -2975,7 +3042,7 @@ bool printk_get_next_message(struct printk_message *pmsg, struct console *con,
> >  	pmsg->dropped = r.info->seq - seq;
> >  
> >  	/* Never suppress when used in devkmsg_read() */
> > -	if (con && suppress_message_printing(r.info->level))
> > +	if (con && suppress_message_printing(r.info->level, con))
> 
> We could read the con_level at the beginning of the funcion. We
> already read there the CON_EXTENDED attribute:
> 
> 	int con_level:
> 
> 	if (con) {
> 		is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
> 		con_level = console_srcu_read_loglevel(con);
> 	} else {
> 		/* Used only by devkmsg_read(). */
> 		is_extended = true;
> 		con_level = LOGLEVEL_DEFAULT;

Or we could do:

	} else {
		/* Used only by devkmsg_read(). Show all messages there. */
		is_extended = true;
		con_level = CONSOLE_LOGLEVEL_MOTORMOUTH;

The LOGLEVE_DEFAULT would cause using the global loglevel.
But we want to force showing the message.


> 	}
> 
> Note that I have used LOGLEVEL_DEFAULT instead of the hardcoded -1.
> IMHO, it is better to use a NAME and LOGLEVEL_DEFAULT is defined as -1
> and the name more or less fits here.
> 
> and then do:
> 
> 	if (con && suppress_message_printing(r.info->level, con_level))

and then we do not need to check the @con here.

This idea came when I was looking at resolving conflicts against the patchset
which removed the hack in sysrq code, see
https://lore.kernel.org/r/20241105-printk-loud-con-v2-0-bd3ecdf7b0e4@suse.com

Best Regards,
Petr