linux-kernel - Re: [PATCH v6 3/4] printk: Userspace format indexing support

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] [day] [month] [year] [list]

Date:   Fri, 4 Jun 2021 12:50:05 +0100
From:   Chris Down <chris@...isdown.name>
To:     Petr Mladek <pmladek@...e.com>
Cc:     linux-kernel@...r.kernel.org, Jessica Yu <jeyu@...nel.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        John Ogness <john.ogness@...utronix.de>,
        Steven Rostedt <rostedt@...dmis.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Kees Cook <keescook@...omium.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>, kernel-team@...com
Subject: Re: [PATCH v6 3/4] printk: Userspace format indexing support

Petr Mladek writes:
>> Petr Mladek writes:
>> > Note that we do not store the value defined by pr_fmt() which
>> > is yet another automatically added prefix.
>>
>> Hmm, we do actually store pr_fmt, since it's known at compile time (or we
>> wouldn't store it at all if it was dynamic due to the __builtin_constant_p
>> check). Or are you seeing something different?
>
>I see. Well, the prefix defined by pr_fmt() becomes part of entry.fmt.
>It is _not_ stored in entry.pre_fmt which confused me.
>
>As I mentioned in the previous mail. I think about using "subsys_fmt"
>or "subsys_prefix" instead of "pre_fmt".
>
>Anyway, we should improve the comment. For example, something like:
>
>	/*
>	 * The format string used by various subsystem specific printk()
>	 * wrappers to prefix the message.
>	 *
>	 * Note that the static prefix defined by pr_fmt() macro is handled
>	 * another way. It is stored directly in the message format (@fmt).
>	 */
>	const char *subsys_fmt;

Sounds totally reasonable, will do.

>> > Is is possible to update __printk_index_emit() to do not create
>> > entries with fmt = NULL ?
>> >
>> > We should either remove the above check or add a comment
>> > explaining why it is necessary.
>>
>> The trick is unfortunately necessary for the reason described in the comment
>> above the double check:
>>
>>     /*
>>      * The compiler may not be able to eliminate the
>>      * non-constant variants of _fmt and _level, so we need
>>      * to make sure that it doesn't see any hypothetical
>>      * assignment for non-constants even though this is
>>      * already inside the __builtin_constant_p guard.
>>      */
>>
>> Happy to add another comment if it helps.
>
>I re-read the discussion about this,
>namely https://lore.kernel.org/r/3Kf896Zt9O+/Yh@chrisdown.name
>
>Frankly, the comment above ftrace_vprintk() is easier to understand
>for me. I mean:
>
>/*
> * The double __builtin_constant_p is because gcc will give us an error
> * if we try to allocate the static variable to fmt if it is not a
> * constant. Even with the outer if statement.
> */

Oh yes, that does sound more clear. I'll update it :-)

Thanks!