linux-kernel - Re: [PATCH v6 02/11] printk: Use struct console for suppression and extended console state

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: <Zz3P_vDLiNNCLCQR@pathway.suse.cz>
Date: Wed, 20 Nov 2024 13:03:48 +0100
From: Petr Mladek <pmladek@...e.com>
To: Chris Down <chris@...isdown.name>
Cc: linux-kernel@...r.kernel.org, John Ogness <john.ogness@...utronix.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Tony Lindgren <tony.lindgren@...ux.intel.com>, kernel-team@...com
Subject: Re: [PATCH v6 02/11] printk: Use struct console for suppression and
 extended console state

On Wed 2024-11-20 04:17:51, Chris Down wrote:
> John Ogness writes:
> > On 2024-10-28, Chris Down <chris@...isdown.name> wrote:
> > > In preparation for supporting per-console loglevels, modify
> > > printk_get_next_message() to accept the console itself instead of
> > > individual arguments that mimic its fields.
> > 
> > Sorry, this is not allowed. printk_get_next_message() was created
> > specifically to locklessly retrieve and format arbitrary records. It
> > must never be tied to a console because it has nothing to do with
> > consoles (as can bee seen with the devkmsg_read() hack you added in the
> > function).
> > 
> > I recommend adding an extra argument specifying the level.
> > 
> > The extra argument would be redundant if may_suppress=false. So perhaps
> > as an alternative change "bool may_suppress" to "u32 supress_level". The
> > loglevels are only 3 bits. So you could easily define a special value
> > NO_SUPPRESS to represent the may_suppress=false case.
> > 
> > #define NO_SUPPRESS BIT(31)
> > 
> > bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
> >                             bool is_extended, u32 suppress_level);
> > 
> > Then in devkmsg_read():
> > 
> > printk_get_next_message(&pmsg, atomic64_read(&user->seq), true, NO_SUPRRESS)
> 
> Petr, what do you think about this? I remember when we discussed this before
> we talked about either determining state via `struct console` (which seems
> to turn out not to be feasible) or passing another argument.
> 
> Do you prefer to have another argument or do the bit dance?
> 
> Personally I prefer the simpler solution with more arguments instead of bit
> stuffing if we have to go this way, but I'm up for whichever sounds good to
> you.

Ah, I though that John's proposal was reasonable. But it is true that
the meaning of @supress_level is not clear.

I see two possibilities:

  1. printk_get_next_message() and console_emit_next_record()
     could pass con->level.

     But then we would need to create the extra value for devkmsg_read().


 2. printk_get_next_message() and console_emit_next_record()
    could pass console_effective_loglevel().

    devkmsg_read() could pass CONSOLE_LOGLEVEL_MOTORMOUTH.


Sigh, it seems that any solution is hairy, including the one which
passed @con.

I personally think that the 2nd variant, passing the effective
loglevel, is least ugly. I am just not sure about a good name
for the parameter. What about?

    bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
				 bool is_extended, int con_eff_level);

Note that this would require passing the effective loglevel also
to suppress_message_printing() so that we would get:

static bool suppress_message_printing(int level, int con_eff_level)
{
	return level >= con_eff_level;
}

Best Regards,
Petr