linux-kernel - Re: [PATCH v2] random: remove batched entropy locking

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: <CAHmME9r0XxX3LqNLpVeqAjDQ_OVskPf15QOwxtZYy0tb_x_7HQ@mail.gmail.com>
Date:   Fri, 4 Feb 2022 14:42:03 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     Andy Lutomirski <luto@...capital.net>,
        Boqun Feng <boqun.feng@...il.com>,
        Will Deacon <will@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Waiman Long <longman@...hat.com>,
        Sultan Alsawaf <sultan@...neltoast.com>,
        "Theodore Ts'o" <tytso@....edu>, Andy Lutomirski <luto@...nel.org>,
        Jonathan Neuschäfer <j.neuschaefer@....net>,
        LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v2] random: remove batched entropy locking

Hi Sebastian,

Please calm down a bit: this patch doesn't minimize the importance of
working out a real solution for PREEMPT_RT, and I'm not under the
illusion that this one here is the silver bullet. It does, however,
have other merits, which may or may not have anything to do with
PREEMPT_RT. To reiterate: I am taking your PREEMPT_RT concerns
seriously, and I want to come up with a solution to that, which we're
working toward more broadly in that other thread.

Per your feedback on v1, this is no longer marked for stable and no
longer purports to fix the PREEMPT_RT issues entirely. Actually, a
large motivation for this includes the reason why Andy's original
patch was laying around in the first place: we're trying to make this
code faster.

I can improve the commit message a bit though.

On Fri, Feb 4, 2022 at 12:10 PM Sebastian Andrzej Siewior
<bigeasy@...utronix.de> wrote:
> - This splat only occurs with CONFIG_PROVE_RAW_LOCK_NESTING enabled.

Right, the commit message for v2 mentions that.

> - The problem identified by the splat affects only PREEMPT_RT. Non-RT is
>   not affected by this.

Right.

>
> - This patch disables interrupts and invokes extract_crng() which leads
>   to other problems.

The existing code, which uses a spinlock, also disables interrupts,
right? So this isn't actually regressing in that regard. It just
doesn't fix your PREEMPT_RT issue, right?

Or is the issue you see that spinlock_t is a mutex on PREEMPT_RT, so
we're disabling interrupts here in a way that we _weren't_ originally,
in a PREEMPT_RT context? If that's the case, then I think I see your
objection.

I wonder if it'd be enough here to disable preemption instead? But
then we run into trouble if this is called from an interrupt.

Maybe it'd be best to retain the spinlock_t, which will amount to
disabling interrupts on !PREEMPT_RT, since it'll never be contended,
but will turn into a mutex on PREEMPT_RT, where it'll do the right
thing from an exclusivity perspective. Would this be reasonable?

Andy? Any suggestions?

Jason