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]
Message-ID: <CALqELGwd1CiRAYNBVWsrgb5T3eJ9ugP+0wG2WKZGvSfowqgaaQ@mail.gmail.com>
Date: Tue, 30 Sep 2025 16:14:19 +0100
From: Andrew Murray <amurray@...goodpenguin.co.uk>
To: John Ogness <john.ogness@...utronix.de>
Cc: Petr Mladek <pmladek@...e.com>, Steven Rostedt <rostedt@...dmis.org>, 
	Sergey Senozhatsky <senozhatsky@...omium.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] printk: console_flush_one_record() code cleanup

On Tue, 30 Sept 2025 at 13:59, John Ogness <john.ogness@...utronix.de> wrote:
>
> On 2025-09-27, Andrew Murray <amurray@...goodpenguin.co.uk> wrote:
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 060d4919de320fe21fd7aca73ba497e27c4ff334..e2c1cacdb4164489c60fe38f1e2837eb838107d6 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3193,6 +3194,7 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
> >       bool any_progress;
> >       int cookie;
> >
> > +     *any_usable = false;
>
> Since it is expected that @next_seq and @handover are initialized by
> their callers (if their callers are interested in the values), then I
> would expect @any_usable to be initialized by the
> caller. console_flush_one_record() never reads this variable.

Yes, that's correct. Perhaps the comments for the parameters should
indicate otherwise?


>
> > @@ -3280,21 +3284,16 @@ static bool console_flush_one_record(bool do_cond_resched, u64 *next_seq, bool *
> >   */
> >  static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handover)
> >  {
> > -     bool any_usable = false;
> > +     bool any_usable;
>
> Since console_flush_all() does read @any_usable, I would expect it to
> initialize @any_usable. So I would not remove this definition initialization.

Prior to this series, console_flush_all would set any_usable to false.
It would be set to true if at any point a usable console is found,
that value would be returned, or otherwise false if handover or panic.
When the first patch split out this function, any_usable was kept as
it was, leading to any_usable being true, even if a handover or panic
had happened (hence additional checks needed, which are removed in
this patch).

By setting any_usable at the start of flush_one_record, it allows for
any_usable to revert back to false, in the case where a once usable
console is no longer usable. Thus representing the situation for the
last record printed. It also makes console_flush_one_record easier to
understand, as the any_usable flag will always be set, rather than
only changing from false to true.

At least that was the intent discussed here [1].

Alternatively, it may be possible for console_flush_one_record to
return any_usable, thus dropping it as an argument and removing the
return of any_progress. Instead the caller could keep calling
console_flush_one_record until it returns false or until next_seq
stops increasing? Thus semantically, the return value of
console_flush_one_record tells you that nothing bad happened and you
can call it again, and the benefit of calling it again depends on if
progress is being made (as determined by the caller through the
existing seq argument).

>
> >       bool any_progress;
> >
> >       *next_seq = 0;
> >       *handover = false;
> >
> >       do {
> > -             any_progress = console_flush_one_record(do_cond_resched, next_seq, handover,
> > -                                                     &any_usable);
> > +             any_progress = console_flush_one_record(do_cond_resched, next_seq,
> > +                                                     handover, &any_usable);
> >
>
> Since the second line of the call to console_flush_one_record() already
> has a ton of whitespace, I would remove the above blank line.

Sure.


>
> John


[1] https://lkml.org/lkml/2025/9/27/200

Andrew Murray

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ