linux-kernel - Re: [PATCH printk v3 03/19] printk: nbcon: Add function for printers to reacquire ownership

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <874j76hhrt.fsf@jogness.linutronix.de>
Date: Tue, 27 Aug 2024 03:38:54 +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 03/19] printk: nbcon: Add function for
 printers to reacquire ownership

On 2024-07-30, Petr Mladek <pmladek@...e.com> wrote:
> My idea was: "If we still own the context that we have owned it all
> 	      the time and con-write_atomic() succeeded."
>
> The race is is not important. If we lose the ownership before updating
> nbcon_seq then the line will get written again anyway.
>
>> Once a reacquire has occurred, the driver is allowed to proceed. It just
>> is not allowed to print (because its buffer is gone).
>
> I see. My idea does not work because the driver is going to reacquire
> the ownership. It means that nbcon_can_proceed() would return true
> even when con->atomic_write() failed.
>
> But it is not documented anywhere. And what if the driver has a bug
> and does not call reacquire. Or what if the driver does not need
> to restore anything.
>
> IMHO, nbcon_emit_next_record() should check both:
>
> 	if (use_atomic)
> 		con->write_atomic(con, wctxt);
> 	else
> 		con->write_thread(con, wctxt);
>
> 	/* Still owns the console? */
> 	if (!nbcon_can_proceed(wctxt)
> 		return false;
>
> 	if (!wctxt->outbuf) {
> 		/*
> 		 * Ownership was lost and reacquired by the driver.
> 		 * Handle it as if ownership was lost.
> 		 */
> 		nbcon_context_release(ctxt);
> 		return false;
> 	}

Except that the next thing nbcon_emit_next_record() does is
nbcon_context_enter_unsafe(), which checks ownership. So your suggested
nbcon_can_proceed() is redundant.

For v4 I can add comments explaining this. It would look like this (at
this point in the series):

	/* Initialize the write context for driver callbacks. */
	nbcon_write_context_set_buf(wctxt, &pmsg.pbufs->outbuf[0], pmsg.outbuf_len);

	if (con->write_atomic) {
		con->write_atomic(con, wctxt);
	} else {
		/*
		 * This function should never be called for legacy consoles.
		 * Handle it as if ownership was lost and try to continue.
		 */
		WARN_ON_ONCE(1);
		nbcon_context_release(ctxt);
		return false;
	}

	if (!wctxt->outbuf) {
		/*
		 * Ownership was lost and reacquired by the driver. Handle it
		 * as if ownership was lost.
		 */
		nbcon_context_release(ctxt);
		return false;
	}

	/*
	 * Ownership may have been lost but _not_ reacquired by the driver.
	 * This case is detected and handled when entering unsafe to update
	 * dropped/seq values.
	 */

	/*
	 * Since any dropped message was successfully output, reset the
	 * dropped count for the console.
	 */
	dropped = 0;
update_con:
	/*
	 * The dropped count and the sequence number are updated within an
	 * unsafe section. This limits update races to the panic context and
	 * allows the panic context to win.
	 */

	if (!nbcon_context_enter_unsafe(ctxt))
		return false;

John Ogness