linux-kernel - Re: [PATCH v2 1/3] printk: introduce new macros pr_<level>

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: <42950773-aac6-4ec6-8cbe-543489afe316@t-8ch.de>
Date:   Wed, 30 Nov 2022 15:50:55 +0100
From:   Thomas Weißschuh <linux@...ssschuh.net>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Joe Perches <joe@...ches.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>,
        linux-pm@...r.kernel.org,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Andy Whitcroft <apw@...onical.com>,
        linux-kernel@...r.kernel.org, Steven Rostedt <rostedt@...dmis.org>,
        Dwaipayan Ray <dwaipayanray1@...il.com>,
        Lukas Bulwahn <lukas.bulwahn@...il.com>
Subject: Re: [PATCH v2 1/3] printk: introduce new macros pr_<level>_cont()

On 2022-11-30 14:59+0100, Petr Mladek wrote:
> On Fri 2022-11-25 21:33:40, Thomas Weißschuh wrote:
>> On 2022-11-25 12:18-0800, Joe Perches wrote:
>>> On Fri, 2022-11-25 at 20:09 +0100, Thomas Weißschuh wrote:
>>>> These macros emit continuation messages with explicit levels.
>>>> In case the continuation is logged separately from the original message
>>>> it will retain its level instead of falling back to KERN_DEFAULT.
>>>> 
>>>> This remedies the issue that logs filtered by level contain stray
>>>> continuation messages without context.
>>>> 
>>>> --- a/include/linux/printk.h
>>>> +++ b/include/linux/printk.h
>>>> @@ -701,6 +703,27 @@ do {									\
>>>>  	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
>>>>  #endif
>>>>  
>>>> +/*
>>>> + * Print a continuation message with level. In case the continuation is split
>>>> + * from the main message it preserves the level.
>>>> + */
>>>> +
>>>> +#define pr_emerg_cont(fmt, ...)					\
>>>> +	printk(KERN_EMERG KERN_CONT pr_fmt(fmt), ##__VA_ARGS__)
>>> 
>>> Aren't this rather backwards?
>>> KERN_CONT KERN_<LEVEL> seems to make more sense to me.
>> 
>> If nobody else disagrees I'll do this for v3.
> 
> I slightly prefer the way how it is now. IMHO, it makes it easier
> to check the related levels in /sys/kernel/debug/printk/index/vmlinux [*]:
> 
> <6> kernel/power/process.c:227 thaw_kernel_threads "Restarting kernel threads ... "
> <6,c> kernel/power/process.c:218 thaw_processes "done.\n"
> <6> kernel/power/process.c:197 thaw_processes "Restarting tasks ... "
> <6,c> kernel/power/process.c:176 freeze_kernel_threads "\n"
> <6,c> kernel/power/process.c:174 freeze_kernel_threads "done."
> <6> kernel/power/process.c:169 freeze_kernel_threads "Freezing remaining freezable tasks ... "
> <6,c> kernel/power/process.c:140 freeze_processes "\n"
> <6,c> kernel/power/process.c:138 freeze_processes "done."
> <6> kernel/power/process.c:133 freeze_processes "Freezing user space processes ... "
> <6,c> kernel/power/process.c:105 try_to_freeze_tasks "(elapsed %d.%03d seconds) "

I did not test it (will do so later) but it seems to me that the code in
kernel/printk/index.c should do this correctly in either case. At least it
tries to:

if (flags & LOG_CONT) {
	/*
	 * LOGLEVEL_DEFAULT here means "use the same level as the
	 * message we're continuing from", not the default message
	 * loglevel, so don't display it as such.
	 */
	if (level == LOGLEVEL_DEFAULT)
		seq_puts(s, "<c>");
		else
		seq_printf(s, "<%d,c>", level);
	} else
		seq_printf(s, "<%d>", level);
	}
}

I'll try to validate it.

If it doesn't work it may make more sense to fix the index file generation.
What do you think

> That said, I do not want to fight over it. It is hidden behind the
> API. The only really visible place is the printk index.
> 
> [*] The index is available only when CONFIG_PRINTK_INDEX is enabled.
> 
> Best Regards,
> Petr

Thomas