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: <871q3bkkgw.fsf@jogness.linutronix.de>
Date: Tue, 30 Jul 2024 16:50:47 +0206
From: John Ogness <john.ogness@...utronix.de>
To: Petr Mladek <pmladek@...e.com>
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 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.

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.

> +	}
> +
> +	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.

Thoughts?

John Ogness