Re: [patch RFC 28/29] printk: Provide functions for atomic write enforcement

From: John Ogness <john.ogness@linutronix.de>
To: Thomas Gleixner <tglx@linutronix.de>,
	LKML <linux-kernel@vger.kernel.org>
Cc: Petr Mladek <pmladek@suse.com>,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Linus Torvalds <torvalds@linuxfoundation.org>,
	Peter Zijlstra <peterz@infradead.org>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Daniel Vetter <daniel@ffwll.ch>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Helge Deller <deller@gmx.de>,
	Jason Wessel <jason.wessel@windriver.com>,
	Daniel Thompson <daniel.thompson@linaro.org>
Subject: Re: [patch RFC 28/29] printk: Provide functions for atomic write enforcement
Date: Tue, 27 Sep 2022 16:46:46 +0206	[thread overview]
Message-ID: <87mtaklw29.fsf@jogness.linutronix.de> (raw)
In-Reply-To: <20220910222301.995758602@linutronix.de>

Below is a fix that was used for the LPC2022 demo so that an
interrupting context does not clobber the console state when the
interrupted context was the console owner.

On 2022-09-11, Thomas Gleixner <tglx@linutronix.de> wrote:
> --- a/kernel/printk/printk_nobkl.c
> +++ b/kernel/printk/printk_nobkl.c
> +/**
> + * cons_atomic_flush_one - Flush one console in atomic mode
> + * @con:	The console to flush
> + * @prio:	The priority of the current context
> + */
> +static void cons_atomic_flush_one(struct console *con, enum cons_prio prio)
> +{
> +	struct cons_write_context *wctxt = cons_get_wctxt(con, prio);
> +	struct cons_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);

@ctxt is per-console, per-cpu, and per-prio. But it is *not*
per-context. If a CPU is in EMERGENCY priority and is interrupted by
another context that calls into printk() while the first context had the
console locked, the following cons_atomic_try_acquire() will use the
same ctxt and clobber the values used by the first context. This
corrupts the state information for the first context and results in
situations where the console is never unlocked because the owner's state
was corrupt.

> +	if (!cons_atomic_try_acquire(con, ctxt, prio))
> +		return;
> +
> +	do {
> +		/*
> +		 * cons_emit_record() returns false when the console was
> +		 * handed over or taken over. In both cases the context is
> +		 * not longer valid.
> +		 */
> +		if (!cons_emit_record(wctxt))
> +			return;
> +	} while (ctxt->backlog);
> +
> +	cons_release(ctxt);
> +}

Since it is not desirable to actively print from nested contexts
of the same priority, add an @in_use flag to wctxt, allowing to
detect the situation and avoid active printing and corrupting
the state.

Applying the following patch on top does just that:

diff --git a/include/linux/console.h b/include/linux/console.h
index e6bde0e879fc..1b3028cce3f3 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -317,6 +317,7 @@ struct cons_context {
  * @len:	Length to write
  * @pos:	Current write position in @outbuf
  * @unsafe:	Invoked in unsafe state due to force takeover
+ * @in_use:	The context is in use
  */
 struct cons_write_context {
 	struct cons_context __private	ctxt;
@@ -324,6 +325,7 @@ struct cons_write_context {
 	unsigned int			len;
 	unsigned int			pos;
 	bool				unsafe;
+	bool				in_use;
 };
 
 #define CONS_MAX_NEST_LVL	8
diff --git a/kernel/printk/printk_nobkl.c b/kernel/printk/printk_nobkl.c
index 736f71361799..b513b2fb2c2d 100644
--- a/kernel/printk/printk_nobkl.c
+++ b/kernel/printk/printk_nobkl.c
@@ -1338,8 +1338,12 @@ static void cons_atomic_flush_one(struct console *con, enum cons_prio prio)
 	struct cons_write_context *wctxt = cons_get_wctxt(con, prio);
 	struct cons_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
 
-	if (!cons_atomic_try_acquire(con, ctxt, prio))
+	if (wctxt->in_use)
 		return;
+	wctxt->in_use = true;
+
+	if (!cons_atomic_try_acquire(con, ctxt, prio))
+		goto out;
 
 	do {
 		/*
@@ -1348,10 +1352,12 @@ static void cons_atomic_flush_one(struct console *con, enum cons_prio prio)
 		 * not longer valid.
 		 */
 		if (!cons_emit_record(wctxt))
-			return;
+			goto out;
 	} while (ctxt->backlog);
 
 	cons_release(ctxt);
+out:
+	wctxt->in_use = false;
 }
 
 /**

John Ogness
