linux-kernel - Re: [PATCH printk v2 6/8] printk: nbcon: Add ownership state functions

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: <87h6oirq94.fsf@jogness.linutronix.de>
Date:   Tue, 29 Aug 2023 14:29:51 +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
Subject: Re: [PATCH printk v2 6/8] printk: nbcon: Add ownership state functions

On 2023-08-10, Petr Mladek <pmladek@...e.com> wrote:
>> +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".

Correct.

>> +
>> +		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.

The function does release the lock and return false... after it clears
state.unsafe.

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

Sorry, I do not understand what you mean.

>> +	ctxt->unsafe = unsafe;
>> +
>> +	return true;
>
> An easy solution would be to do here:
>
> 	ctxt->unsafe = unsafe;
> 	return nbcon_context_can_proceed(ctxt, &cur);

ctxt->unsafe is an input. It does not need to be updated anyway. (And
besides, it will be removed for v3.)

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

I think with ctxt->unsafe removed, this v2 implementation is ok.

John