linux-kernel - Re: [PATCH printk v4 03/27] printk: nbcon: Remove return value for write

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: <ZhPuep7cMc3i6wro@localhost.localdomain>
Date: Mon, 8 Apr 2024 15:17:46 +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 v4 03/27] printk: nbcon: Remove return value for
 write_atomic()

On Wed 2024-04-03 00:17:05, John Ogness wrote:
> The return value of write_atomic() does not provide any useful
> information. On the contrary, it makes things more complicated
> for the caller to appropriately deal with the information.
> 
> Change write_atomic() to not have a return value. If the
> message did not get printed due to loss of ownership, the
> caller will notice this on its own. If ownership was not lost,
> it will be assumed that the driver successfully printed the
> message and the sequence number for that console will be
> incremented.
> 
> Signed-off-by: John Ogness <john.ogness@...utronix.de>

Reviewed-by: Petr Mladek <pmladek@...e.com>

> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -889,16 +888,16 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
>  	wctxt->unsafe_takeover = cur.unsafe_takeover;
>  
>  	if (con->write_atomic) {
> -		done = con->write_atomic(con, wctxt);
> +		con->write_atomic(con, wctxt);
>  	} else {
> -		nbcon_context_release(ctxt);
> +		/*
> +		 * This function should never be called for legacy consoles.
> +		 * Handle it as if ownership was lost and try to continue.
> +		 */
>  		WARN_ON_ONCE(1);
> -		done = false;
> -	}
> -
> -	/* If not done, the emit was aborted. */
> -	if (!done)
> +		nbcon_context_release(ctxt);

I thought a bit whether it is better to release the context before
or after the WARN(). My conclusion is that it does not really matter.

Anyway, we must make sure that it is safe to call WARN_ON_ONCE()
when nbcon context is acquired. People will use it. And I believe
that it _is_ safe.

Best Regards,
Petr