linux-kernel - Re: [PATCH printk v3 09/19] printk: nbcon: Introduce printer kthreads

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: <ZqoK77CDoqNll7_M@pathway.suse.cz>
Date: Wed, 31 Jul 2024 11:59:11 +0200
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,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk v3 09/19] printk: nbcon: Introduce printer kthreads

On Tue 2024-07-30 16:50:47, John Ogness wrote:
> On 2024-07-22, John Ogness <john.ogness@...utronix.de> wrote:
> > +/**
> > + * nbcon_kthread_should_wakeup - Check whether a printer thread should wakeup
> > + * @con:	Console to operate on
> > + * @ctxt:	The nbcon context from nbcon_context_try_acquire()
> > + *
> > + * Return:	True if the thread should shutdown or if the console is
> > + *		allowed to print and a record is available. False otherwise.
> > + *
> > + * After the thread wakes up, it must first check if it should shutdown before
> > + * attempting any printing.
> > + */
> > +static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_context *ctxt)
> > +{
> > +	bool ret = false;
> > +	short flags;
> > +	int cookie;
> > +
> > +	if (kthread_should_stop())
> > +		return true;
> > +
> > +	cookie = console_srcu_read_lock();
> > +
> > +	flags = console_srcu_read_flags(con);
> > +	if (console_is_usable(con, flags, false)) {
> > +		/* Bring the sequence in @ctxt up to date */
> > +		ctxt->seq = nbcon_seq_read(con);
> > +
> > +		ret = prb_read_valid(prb, ctxt->seq, NULL);
> 
> With this v3 series, the kthreads could be started before @nbcon_seq has
> been set to the correct initial value. This will cause it to start
> printing immediately from 0.

Great catch!

> To fix this, I would change nbcon_alloc() to initialize @nbcon_seq to
> the highest possible value:
> 
> 	/*
> 	 * Initialize @nbcon_seq to the highest possible sequence number so
> 	 * that practically speaking it will have nothing to print until a
> 	 * desired initial sequence number has been set via nbcon_seq_force().
> 	 */
> 	atomic_long_set(&ACCESS_PRIVATE(con, nbcon_seq), ULSEQ_MAX(prb));
> 
> With the following ULSEQ_MAX macro added to printk_ringbuffer.h:
> 
> #ifdef CONFIG_64BIT
> #define ULSEQ_MAX(rb) (-1)
> #else
> #define ULSEQ_MAX(rb) (prb_first_seq(rb) + 0x80000000)
> #endif
> 
> This will allow the prb_read_valid() in nbcon_kthread_should_wakeup() to
> return false and the kthread will sleep until @nbcon_seq has been
> correctly setup and the kthread is woken when the "enabled" printk() is
> called.

It looks good.

Alternative solution would be to delay setting of the CON_ENABLED bit.
Well, there is one catch, some console drivers pre-set CON_ENABLED to
enforce the registration, see the comment in
try_enable_preferred_console(). We would need to handle this somehow.

So the trick with ULSEQ_MAX is a better way to go.

> > +	}
> > +
> > +	console_srcu_read_unlock(cookie);
> > +	return ret;
> > +}
> 
> Other options would be to add extra checks to
> nbcon_kthread_should_wakeup() or add some wait/notify code before
> entering the kthread main loop. But both are overkill for this simple
> startup scenario.

Yeah, the trick with ULSEQ_MAX is easier and looks reliable. I am fine
with it.

Best Regards,
Petr