linux-kernel - Re: [PATCH printk v2 27/38] printk: console_flush

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]

Date:   Mon, 07 Nov 2022 01:06:02 +0106
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 27/38] printk: console_flush_all: use srcu
 console list iterator

On 2022-10-25, Petr Mladek <pmladek@...e.com> wrote:
>>   console_lock()
>>   | mutex_acquire(&console_lock_dep_map)       <-- console lock
>>   |
>>   console_unlock()
>>   | console_flush_all()
>>   | | srcu_read_lock(&console_srcu)            <-- srcu lock
>>   | | console_emit_next_record()
>>   | | | console_lock_spinning_disable_and_check()
>>   | | | | srcu_read_unlock(&console_srcu)      <-- srcu unlock
>>   | | | | mutex_release(&console_lock_dep_map) <-- console unlock
>>
>> @@ -2819,12 +2827,17 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
>>  				/* Extended consoles do not print "dropped messages". */
>>  				progress = console_emit_next_record(con, &text[0],
>>  								    &ext_text[0], NULL,
>> -								    handover);
>> +								    handover, cookie);
>>  			} else {
>>  				progress = console_emit_next_record(con, &text[0],
>>  								    NULL, &dropped_text[0],
>> -								    handover);
>> +								    handover, cookie);
>>  			}
>> +
>> +			/*
>> +			 * If a handover has occurred, the SRCU read lock
>> +			 * is already released.
>> +			 */
>>  			if (*handover)
>>  				return false;
>
> Please, release the SRCU read lock here:
>
> 			if (*handover) {
> 				console_srcu_read_unlock(cookie);
> 				return false;
> 			}
>
> The lock should be released in the same function where it was taken.
> It does not require passing the cookie and looks more straightforward.

It looks more straight forward, but it is incorrect from a locking
perspective.

The locking order was:

console_lock()
console_srcu_read_lock()

But for a handover at this point in code, console_emit_next_record() has
already released the console_lock (to the spinning context). The
console_srcu_read_lock should have been released first.

> We actually do the same when abandon_console_lock_in_panic()
> returns true. We could share the code:
>
> handover_abandon:
> 	console_srcu_read_unlock(cookie);
> 	return false;

This case is different. Here the console_lock was not released yet so it
is fine to perform the console_srcu_read_unlock() here.

John Ogness