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: <CALqELGw8wtbbihLsOcNgnV2vGoSR7kD8_tHmt7ESY4d3buwrLQ@mail.gmail.com>
Date: Wed, 1 Oct 2025 17:26:27 +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 Wed, 1 Oct 2025 at 10:53, John Ogness <john.ogness@...utronix.de> wrote:
>
> On 2025-09-30, Andrew Murray <amurray@...goodpenguin.co.uk> 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?
>
> They should clarify. For example, @next_seq is "valid only when this
> function returns true" but @handover validitiy is not specified and is
> also not related to the return value.
>
> I would like to see the helper function provide clear usage. If the
> caller is expected to initialize the parameters (which IMHO it should be
> the case for all of the pointer parameters), then that should be
> specified.

OK.


>
> >> > @@ -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.
>
> OK. But then just have console_flush_all() set @any_usable in the loop:
>
> static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handover)
> {
>         bool any_progress;
>         bool any_usable;
>
>         *next_seq = 0;
>         *handover = false;
>
>         do {
>                 any_usable = false;
>                 any_progress = console_flush_one_record(do_cond_resched, next_seq,
>                                                         handover, &any_usable);
>         } while (any_progress);
>
>         return any_usable;
> }

Yes, that seems like common sense, I have no idea why I didn't think of that :|


>
> > 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).
>
> Sorry, I could not follow how this would work. It sounds like a
> simplification. If you can make it work, go for it.

Against my patches, something like this:

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 2c9b9383df76..f38295cc3645 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3186,16 +3186,13 @@ static inline void
printk_kthreads_check_locked(void) { }
  *
  * Requires the console_lock.
  */
-static bool console_flush_one_record(bool do_cond_resched, u64
*next_seq, bool *handover,
-                                    bool *any_usable)
+static bool console_flush_one_record(bool do_cond_resched, u64
*next_seq, bool *handover)
 {
        struct console_flush_type ft;
        struct console *con;
-       bool any_progress;
        int cookie;

-       *any_usable = false;
-       any_progress = false;
+       bool any_usable = false;

        printk_get_console_flush_type(&ft);

@@ -3215,7 +3212,7 @@ static bool console_flush_one_record(bool
do_cond_resched, u64 *next_seq, bool *

                if (!console_is_usable(con, flags, !do_cond_resched))
                        continue;
-               *any_usable = true;
+               any_usable = true;

                if (flags & CON_NBCON) {
                        progress = nbcon_legacy_emit_next_record(con,
handover, cookie,
@@ -3239,7 +3236,6 @@ static bool console_flush_one_record(bool
do_cond_resched, u64 *next_seq, bool *

                if (!progress)
                        continue;
-               any_progress = true;

                /* Allow panic_cpu to take over the consoles safely. */
                if (other_cpu_in_panic())
@@ -3250,12 +3246,11 @@ static bool console_flush_one_record(bool
do_cond_resched, u64 *next_seq, bool *
        }
        console_srcu_read_unlock(cookie);

-       return any_progress;
+       return any_usable;

 unusable_srcu:
        console_srcu_read_unlock(cookie);
 unusable:
-       *any_usable = false;
        return false;
 }

@@ -3286,15 +3281,15 @@ static bool console_flush_all(bool
do_cond_resched, u64 *next_seq, bool *handove
 {
        bool any_usable;
        bool any_progress;
+       u64 last_seq;

        *next_seq = 0;
        *handover = false;

        do {
-               any_progress =
console_flush_one_record(do_cond_resched, next_seq,
-                                                       handover, &any_usable);
-
-       } while (any_progress);
+               last_seq = *next_seq;
+               any_usable = console_flush_one_record(do_cond_resched,
next_seq, handover);
+       } while (*next_seq > last_seq);

        return any_usable;
 }
@@ -3674,21 +3669,20 @@ static int legacy_kthread_func(void *unused)
        wait_event_interruptible(legacy_wait,
                                 legacy_kthread_should_wakeup());

+       u64 last_seq, next_seq = 0;
        do {
-               bool any_usable;
                bool handover = false;
-               u64 next_seq;

                if (kthread_should_stop())
                        return 0;

                console_lock();
-               any_progress = console_flush_one_record(true, &next_seq,
-                                               &handover, &any_usable);
+               last_seq = next_seq;
+               console_flush_one_record(true, &next_seq, &handover);
                if (!handover)
                        __console_unlock();

-       } while (any_progress);
+       } while (next_seq > last_seq);

        goto wait_for_event;
 }
-- 
2.34.1

This also has the benefit of removing code (and more could be removed
if the progress variable in console_flush_one_record is removed - i.e.
we could unconditionally bail on panic and resched each time).

Thanks,

Andrew Murray

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ