linux-kernel - Re: [PATCH printk 06/18] printk: Protect [un]register

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:   Fri, 30 Sep 2022 20:04:44 +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 06/18] printk: Protect [un]register_console() with
 a mutex

On Fri 2022-09-30 16:22:30, John Ogness wrote:
> On 2022-09-30, Petr Mladek <pmladek@...e.com> wrote:
> > You know, the more locks we have, the bigger is the risk of
> > deadlocks, and the more hacks would be needed in
> > console_flush_on_panic(). And I am afraid
> > that console_lock() will be with us for many years and
> > maybe forever.
> 
> Sure. Removing console_lock() will be a long battle involving many
> drivers. I am not trying to fight that battle right now. I just want
> console_lock() out of the way of NOBKL consoles.

There is some misunderstanding. I am going to think more about your
arguments over the weekend.

But maybe, the above is important. You want to get cosole_lock() out
of the way of NOBLK consoles. What does it exactly mean, please?
What code paths are important to achieve this?

>From my POV, the most important code path is the kthread. But it
should use SRCU. I mean that the kthread will take neither
cosnole_lock() nor console_list_lock().

Is there any other code path where console_list_lock() will help
you to get console_lock() out of the way?


>From my POV, the proposed code does:

register_console()
{
	console_list_lock();
	console_lock();

	/* manipulate struct console and the console_list */

	console_unlock();
	console_list_unlock();
}

register_console()
{
	console_list_lock();
	console_lock();

	/* manipulate struct console and the console_list */

	console_unlock();
	console_list_unlock();
}

printk_kthread()
{
	while() {
		srcu_read_lock();

		if (read_flags_srcu())
		     /* print line */

		srcu_read_unlock();
	}
}

vprintk_emit()
{
	/* store message */

	if (do_not_allow_sync_mode)
		return;

	if (console_trylock()) {
		console_flush_all();
		__console_unlock();
	}
}

some_other_func()
{
	console_list_lock();
	/* do something with all registered consoles */
	console_list_unlock();
}

console_flush_all()
{
	do_something_with_all_consoles();
	do_something_else_with_all_consoles();
}

What if?

do_something_with_all_consoles()
{
	console_list_lock();
	/* do something */
	console_list_unlock();
}

Wait, there is a possible ABBA deadlock because
do_something_with_all_consoles() takes console_list_lock()
under console_lock(). And register_console() does it
the other way around.

But it is less obvious because these are different locks.

>From my POV, both locks serialize the same things
(console_list manipulation). SRCU walk should be
enough for most iterations over the list.

And I do not see which code path would really benefit from
having the new console_list_lock() instead of console_lock().

Best Regards,
Petr