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]

Message-ID: <87a66c66px.fsf@jogness.linutronix.de>
Date:   Mon, 03 Oct 2022 21:41:22 +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 06/18] printk: Protect [un]register_console()
 with a mutex

On 2022-10-03, Petr Mladek <pmladek@...e.com> wrote:
> What is exactly wrong with console_lock, please?

It is ambiguously performing multiple tasks:

- protecting the console list
- protecting individual console fields
- serializing console printing
- stopping all console printing

That last item is actually quite complex because nobody really knows
_why_ all consoles need to be stopped. It is mostly because fbdev is
using the console_lock to protect itself from its own write()
callback. But (as has been mentioned in this thread) there are other
code sites where we are not sure which part of the above tasks it is
used for and why.

> Is the main problem that it is a semaphore?

A semaphore has been needed because we are performing global locking for
ambiguous reasons in all possible contexts. We should be using
fine-grained lock and synchronization mechanisms that are appropriate
for their used contexts to precisely lock/synchronize exactly what needs
to be locked/synchronized.

Your first question is literally, "what is wrong with a BKL".

And the answer to that is: A BKL is preventing us from optimizing the
kernel by decoupling unrelated activities.

> The above proposal suggests that it might be something like:
>
> register_console()
> {
> 	console_list_lock();
>
> 	if (!need_console())
> 		goto out;
>
> 	if (!try_enable_console())
> 		goto out;
>
> 	if (!(con->flags & CON_NOBLK))
> 		console_lock()

Why are you taking the console_lock here? The console_list_lock needs to
replace this responsibility. I realize the RFC and this v1 series does
not do this. For v2, it will be clear.

> 	add_console_into_the_list();
>
> 	if (!(con->flags & CON_NOBLK))
> 		console_unlock()

I would request that you continue reviewing the later patches in the
series. Particularly 13-18. My v2 will involve a significantly reworked
version of patches 6-12, not only changing the order of presentation,
but also explicitly removing console list update protection by the
console_lock. I think having actual code to discuss will greatly help us
continue this discussion.

Patches 13-18 will not change much for v2, unless I get some feedback
otherwise.

John