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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <87cymwfvd7.fsf@jogness.linutronix.de>
Date: Mon, 29 Jul 2024 10:42:04 +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-26, Petr Mladek <pmladek@...e.com> wrote:
> On Mon 2024-07-22 19:25:23, 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.
>
> I am still not sure how this is going to be used. It is suspicious.
> If the context lost the ownership than another started flushing
> higher priority messages.
>
> Is it really safe to manipulate the HW at this point?
> Won't it break the higher priority context?

Why would it break anything? It spins until it normally and safely
acquires ownership again. The commit message provides a simple example
of why it is necessary. With a threaded printer, this situation happens
almost every time a warning occurs.

>> --- a/kernel/printk/nbcon.c
>> +++ b/kernel/printk/nbcon.c
>> @@ -911,6 +948,15 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
>>  		return false;
>>  	}
>>  
>> +	if (!wctxt->outbuf) {
>
> This check works only when con->write_atomic() called
> nbcon_reacquire_nobuf().

Exactly. That is what it is for.

> At least, we should clear the buffer also in nbcon_enter_unsafe() and
> nbcon_exit_unsafe() when they realize that they do own the context.

OK.

> Even better would be to add a check whether we still own the context.
> Something like:
>
> bool nbcon_can_proceed(struct nbcon_write_context *wctxt)
> {
> 	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> 	struct nbcon_state cur;
>
> 	nbcon_state_read(con, &cur);
>
> 	return nbcon_context_can_proceed(ctxt, &cur);
> }

nbcon_can_proceed() is meant to check ownership. And it only makes sense
to use it within an unsafe section. Otherwise it is racy.

Once a reacquire has occurred, the driver is allowed to proceed. It just
is not allowed to print (because its buffer is gone).

>> +		/*
>> +		 * Ownership was lost and reacquired by the driver.
>> +		 * Handle it as if ownership was lost.
>> +		 */
>> +		nbcon_context_release(ctxt);
>> +		return false;

John