linux-kernel - Re: [PATCH v5 2/2] printk: console: Support console-specific loglevels

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]

Date:   Thu, 27 Apr 2023 17:47:26 +0206
From:   John Ogness <john.ogness@...utronix.de>
To:     Petr Mladek <pmladek@...e.com>, 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>,
        Geert Uytterhoeven <geert@...ux-m68k.org>, kernel-team@...com
Subject: Re: [PATCH v5 2/2] printk: console: Support console-specific loglevels

On 2023-04-27, Petr Mladek <pmladek@...e.com> wrote:
>> @@ -2764,7 +2982,8 @@ static void console_prepend_dropped(struct printk_message *pmsg, unsigned long d
>>   * of @pmsg are valid. (See the documentation of struct printk_message
>>   * for information about the @pmsg fields.)
>>   */
>> -static bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>> +static bool printk_get_next_message(struct printk_message *pmsg,
>> +				    struct console *con, u64 seq,
>>  				    bool is_extended, bool may_suppress)
>
> @is_extended has to match @con->flags & CON_EXTENDED. It would be
> ugly if the API is not used correctly and they do not match.
>
> Also the "may_suppress" value might be guessed from @con.
>
> I see two solutions:
>
> 1. If we pass @con then @is_extended and @may_suppress parameters
>    are not needed. The values might be guessed. The logic is
>    the following:
>
> 	if (con) {
> 		is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
> 		may_suppress = true;
> 	} else {
> 		/* Used only by devkmsg_read(). */
> 		is_extended = true;
> 		may_suppress = false;
> 	}

This is correct, but we also need to keep in mind that the caller of
printk_get_next_message() also needs to know if it is extended, so that
it can know if it should prepend the dropped messages. Although I guess
it doesn't matter if the caller is also reading @con->flags to get the
extended status.

I don't want to move console_prepend_dropped() inside
printk_get_next_message() because then printk_get_next_message() would
be modifying @dropped. The main purpose of printk_get_next_message() was
to locklessly get formatted text, thus simplifying the atomic console
implementation.

>    The drawback is that @con->seq and @seq values might still
>    be inconsistent. But the @seq value must be passed explicitly
>    when called from dev_kmsg() and @con is NULL. Also the explicit
>    @seq parameter will most likely useful also for the
>    con->atomic_write() callback added by John's patchset.
>    The atomic consoles will not use con->seq at all.

Correct. @flags and @loglevel are the _only_ fields that may be read
locklessly.

> 2. We could pass @con_loglevel instead of @con. And pass it to
>    suppress_message_printing() instead of @con as well.
>
>    It is probably cleaner solution but the many parameters suck.

We would need to pass console_effective_loglevel(). But yes, 3 separate
parameters when they could be determined from 1 sucks.

> I would personally use the 1st proposal and live with the fact
> that the function would ignore con->seq and use the passed
> @seq instead.

Whitelist: only @flags and @level may be read

I agree that we should go with your 1st proposal. But I would like to
see some comments added into printk_get_next_message() that only @flags
and @level may be considered valid.

John