linux-kernel - Re: hostile takeover: Re: [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic

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] [day] [month] [year] [list]

Message-ID: <87a5twhgag.fsf@jogness.linutronix.de>
Date:   Fri, 08 Sep 2023 16:54:39 +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: hostile takeover: Re: [PATCH printk v2 3/8] printk: nbcon: Add
 acquire/release logic

On 2023-09-07, Petr Mladek <pmladek@...e.com> wrote:
> Hmm, I am actually not sure if nbcon_emit_next_record()
> calls printk_get_next_message() and con->write_atomic(con, wctxt)
> in "safe" or "unsafe" context.

It does not! In the early versions of this code it was not a problem
because we had a per-priority buffer for each console. But now we only
have one buffer that is shared by the NORMAL and EMERGENCY priorities
per console. For v4 I am creating a safe region around
printk_get_next_message() and console_prepend_dropped(). That will fix
the issue. Nice catch!

For the callbacks write_atomic() (and later, write_thread()) it should
not be called in unsafe. It is up to the drivers to decide what is safe
and unsafe.

>> 1. try direct
>> 2. try safe takeover
>> 3. try handover
>> 4. try hostile takeover
>
> I rather meant:
>
>   1. try direct
>   2. try handover +
>      try safe takeover in every waiting cycle
>   3. try hostile takeover
>
> But then it won't be a handover anymore.
>
> Anyway, I would keep it as is for now. As mentioned above,
> the current handover is more conservative approach because
> the lock is passed on well defined locations.

For v3 I made the change as I suggested above. So it will perform a
direct takeover if the priority is higher and it is safe. Yes, waiting
first might seem more polite and conservative. But if the driver is in a
safe state, I see no reason to make a higher priority context wait. If
it would be a problem for the first context to suddenly lose ownership,
then it should be marking it an unsafe region.

For fun I implemented it such that it only directly takes over an owner
after having waited. But it adds quite a bit of ugliness to the routine
and I don't think it is worth it.

For v4 I will keep it the same as v3: a direct takeover of an existing
owner when safe before trying the handover.

John Ogness