linux-kernel - Re: [PATCH printk v2 18/18] 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <ZoQPLe-VogelU4Jm@pathway.suse.cz>
Date: Tue, 2 Jul 2024 16:31:09 +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 v2 18/18] printk: nbcon: Add function for printers
 to reacquire ownership

On Tue 2024-06-04 01:30:53, John Ogness wrote:
> Since ownership can be lost at any time due to handover or
> takeover, a printing context _must_ be prepared to back out
> immediately and carefully. However, there are scenarios where
> the printing context must reacquire ownership in order to
> finalize or revert hardware changes.
> 
> One such example is when interrupts are disabled during
> printing. No other context will automagically re-enable the
> interrupts. For this case, the disabling context _must_
> reacquire nbcon ownership so that it can re-enable the
> interrupts.
> 
> Provide nbcon_reacquire() for exactly this purpose. It allows a
> printing context to reacquire ownership using the same priority
> as its previous ownership.
> 
> Note that after a successful reacquire the printing context
> will have no output buffer because that has been lost. This
> function cannot be used to resume printing.
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -838,6 +838,38 @@ bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt)
>  }
>  EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
>  
> +/**
> + * nbcon_reacquire - Reacquire a console after losing ownership while printing
> + * @wctxt:	The write context that was handed to the write callback
> + *
> + * Since ownership can be lost at any time due to handover or takeover, a
> + * printing context _must_ be prepared to back out immediately and
> + * carefully. However, there are scenarios where the printing context must
> + * reacquire ownership in order to finalize or revert hardware changes.
> + *
> + * This function allows a printing context to reacquire ownership using the
> + * same priority as its previous ownership.
> + *
> + * Note that after a successful reacquire the printing context will have no
> + * output buffer because that has been lost. This function cannot be used to
> + * resume printing.
> + */
> +void nbcon_reacquire(struct nbcon_write_context *wctxt)

I think about calling it "nbcon_reacquire_nobuf" to make it clear
that it is not a complete recovery.

> +{
> +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> +	struct console *con = ctxt->console;
> +	struct nbcon_state cur;
> +
> +	while (!nbcon_context_try_acquire(ctxt))
> +		cpu_relax();
> +
> +	wctxt->outbuf = NULL;
> +	wctxt->len = 0;
> +	nbcon_state_read(con, &cur);
> +	wctxt->unsafe_takeover = cur.unsafe_takeover;

The nbcon_state_read() makes it a bit tricky. I would hide it into
a helper script:

void nbcon_write_context_set_buf(struct nbcon_write_context *wctxt,
				 char *buf, unsigned int len)
{
	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
	struct console *con = ctxt->console;
	struct nbcon_state cur;

	wctxt->outbuf = buf;
	wctxt->len = len;
	nbcon_state_read(con, &cur);
	wctxt->unsafe_takeover = cur.unsafe_takeover;
}

It would help to keep it in sync with nbcon_emit_next_record().

> +}
> +EXPORT_SYMBOL_GPL(nbcon_reacquire);

Otherwise, it looks good. I do not resist on the proposed changes.

Best Regards,
Petr