Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 10 Aug 2023 14:56:58 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH printk v2 6/8] printk: nbcon: Add ownership state
 functions
Message-ID: <ZNTemiTUI38f11ek@alley>
References: <20230728000233.50887-1-john.ogness@linutronix.de>
 <20230728000233.50887-7-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230728000233.50887-7-john.ogness@linutronix.de>
Precedence: bulk

On Fri 2023-07-28 02:08:31, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Provide functions that are related to the safe handover mechanism
> and allow console drivers to dynamically specify unsafe regions:
> --- a/kernel/printk/printk_nbcon.c
> +++ b/kernel/printk/printk_nbcon.c
> @@ -650,6 +649,118 @@ static void nbcon_context_release(struct nbcon_context *ctxt)
>  	ctxt->pbufs = NULL;
>  }
>  
> +/**
> + * nbcon_context_can_proceed - Check whether ownership can proceed
> + * @ctxt:	The nbcon context from nbcon_context_try_acquire()
> + * @cur:	The current console state
> + *
> + * Return:	True if the state is correct. False if ownership was
> + *		handed over or taken.
> + *
> + * Must be invoked after the record was dumped into the assigned buffer
> + * and at appropriate safe places in the driver.
> + *
> + * When this function returns false then the calling context is no longer
> + * the owner and is no longer allowed to go forward. In this case it must
> + * back out immediately and carefully. The buffer content is also no longer
> + * trusted since it no longer belongs to the calling context.
> + */
> +static bool nbcon_context_can_proceed(struct nbcon_context *ctxt, struct nbcon_state *cur)
> +{
[...]
> +	/*
> +	 * A console owner within an unsafe region is always allowed to
> +	 * proceed, even if there are waiters. It can perform a handover
> +	 * when exiting the unsafe region. Otherwise the waiter will
> +	 * need to perform an unsafe hostile takeover.
> +	 */
> +	if (cur->unsafe) {
> +		debug_store(cur->req_prio > cur->prio,
> +			    "handover: cpu%d IGNORING HANDOVER prio%d -> prio%d (unsafe)\n",
> +			    cpu, cur->prio, cur->req_prio);
> +		return true;
> +	}
[...]
> +}
> +
> +/**
> + * nbcon_context_update_unsafe - Update the unsafe bit in @con->nbcon_state
> + * @ctxt:	The nbcon context from nbcon_context_try_acquire()
> + * @unsafe:	The new value for the unsafe bit
> + *
> + * Return:	True if the state is correct. False if ownership was
> + *		handed over or taken.
> + *
> + * Typically the unsafe bit is set during acquire. This function allows
> + * modifying the unsafe status without releasing ownership.
> + *
> + * When this function returns false then the calling context is no longer
> + * the owner and is no longer allowed to go forward. In this case it must
> + * back out immediately and carefully. The buffer content is also no longer
> + * trusted since it no longer belongs to the calling context.
> + *
> + * Internal helper to avoid duplicated code
> + */
> +__maybe_unused
> +static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
> +{
> +	struct console *con = ctxt->console;
> +	struct nbcon_state cur;
> +	struct nbcon_state new;
> +
> +	nbcon_state_read(con, &cur);
> +
> +	/* The unsafe bit must not be cleared if @hostile_unsafe is set. */
> +	if (!unsafe && cur.hostile_unsafe)
> +		return nbcon_context_can_proceed(ctxt, &cur);
> +
> +	do {
> +		if (!nbcon_context_can_proceed(ctxt, &cur))
> +			return false;

nbcon_context_can_proceed() returns "true" even when there
is a pending request. It happens when the current state is "unsafe".
But see below.

> +
> +		new.atom = cur.atom;
> +		new.unsafe = unsafe;
> +	} while (!nbcon_state_try_cmpxchg(con, &cur, &new));

If the new state is "safe" and there is a pending request
then we should release the lock and return false here.

It does not make sense to block the waiter just to realize
that we can't enter "unsafe" state again.

> +	ctxt->unsafe = unsafe;
> +
> +	return true;

An easy solution would be to do here:

	ctxt->unsafe = unsafe;
	return nbcon_context_can_proceed(ctxt, &cur);

> +}

But maybe, we can change the logic a bit. Something like:

/**
 * nbcon_context_update_unsafe - Update the unsafe bit in @con->nbcon_state
 * @ctxt:	The nbcon context from nbcon_context_try_acquire()
 * @unsafe:	The new value for the unsafe bit
 *
 * Return:	True if the state is correct. False if ownership was
 *		handed over or taken.
 *
 * When this function returns false then the calling context is no longer
 * the owner and is no longer allowed to go forward. In this case it must
 * back out immediately and carefully. The buffer content is also no longer
 * trusted since it no longer belongs to the calling context.
 *
 * Internal helper to avoid duplicated code
 */
static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
{
	struct console *con = ctxt->console;
	struct nbcon_state cur;
	struct nbcon_state new;
	bool updated, can_proceed;

	if (!nbcon_context_can_proceed(ctxt, &cur))
		return false;

	/* The unsafe bit must not be cleared if @hostile_unsafe is set. */
	if (cur.hostile_unsafe)
		unsafe = true;

	if (cur.unsafe == unsafe)
		return true;

	do {
		new.atom = cur.atom;
		new.unsafe = unsafe;

		updated = nbcon_state_try_cmpxchg(con, &cur, &new));
		/*
		 * The state has changed. Either there is a new
		 * request lor there was a hostile takeover.
		 */
		can_proceed = nbcon_context_can_proceed(ctxt, &cur);
	} while (!updated && can_proceed);

	if (updated)
		ctxt->unsafe = unsafe;

	return can_proceed;
}

Best Regards,
Petr