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: <Y3PZEbx+40ZyN/79@alley>
Date:   Tue, 15 Nov 2022 19:23:13 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org
Subject: Re: replay log: Re: [PATCH printk v4 38/39] printk: relieve
 console_lock of list synchronization duties

On Tue 2022-11-15 17:47:12, John Ogness wrote:
> On 2022-11-15, Petr Mladek <pmladek@...e.com> wrote:
> > On Mon 2022-11-14 17:35:31, John Ogness wrote:
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -3334,6 +3330,11 @@ void register_console(struct console *newcon)
> >>  		 * boot console that is the furthest behind.
> >>  		 */
> >>  		if (bootcon_registered && !keep_bootcon) {
> >> +			/*
> >> +			 * Hold the console_lock to guarantee safe access to
> >> +			 * console->seq.
> >> +			 */
> >> +			console_lock();
> >>  			for_each_console(con) {
> >>  				if ((con->flags & CON_BOOT) &&
> >>  				    (con->flags & CON_ENABLED) &&
> >> @@ -3341,6 +3342,7 @@ void register_console(struct console *newcon)
> >>  					newcon->seq = con->seq;
> >>  				}
> >>  			}
> >> +			console_unlock();
> >
> > Thinking more about it. This console_unlock() will actually cause
> > flushing the boot consoles. A solution would be to call
> > console_flush_all() here.
> 
> console_flush_all() requires the console_lock, so I don't think it would
> be different.

I meant to keep the console_lock(), something like:

		if (bootcon_registered && !keep_bootcon) {
			/*
			 * Hold the console_lock to guarantee safe access to
			 * console->seq.
			 */
			console_lock();
			/* Try to sync all consoles. */
			console_flush_all();
			/* Check if some boot console is still behind. */
			for_each_console(con) {
				if ((con->flags & CON_BOOT) &&
				    (con->flags & CON_ENABLED) &&
				    con->seq < newcon->seq) {
					newcon->seq = con->seq;
				}
			}
			console_unlock();
		}

It is not that easy because console_flush_all() might handover the
console_lock(). Also some new messages might appear so that we
should re-read prb_next_seq().

Maybe, the best solution would be to call pr_flush():

		if (bootcon_registered && !keep_bootcon) {
			/*
			 * Try to sync all consoles because we do not
			 * know which one is going to be replaced
			 */
			pr_flush();
			/*
			 * Hold the console_lock to guarantee safe access to
			 * console->seq.
			 */
			console_lock();
			/* Check if some boot console is still behind. */
			for_each_console(con) {
				if ((con->flags & CON_BOOT) &&
				    (con->flags & CON_ENABLED) &&
				    con->seq < newcon->seq) {
					newcon->seq = con->seq;
				}
			}
			console_unlock();
		}

It was always just the best effort. It does not need to be perfect.
On the other hand, we should not make it much worse because people
report duplicated messages from time to time.

> The correct solution would be to recognize if nextcon is taking over a
> bootcon. If yes, that bootcon could be unregistered right here with
> unregister_console_locked() and then seq for nextcon set appropriately
> to perfectly take over.
> 
> But we will need to think about how we could recognize the same
> device. I was thinking about if consoles hat some attribute showing
> their io-membase or something so that it could be clear that the two are
> the same hardware.

Interesting idea. Well, it is out of scope of this patchset.

Best Regards,
Petr

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ