linux-kernel - Re: [PATCH v2] random: do crng pre-init loading in worker rather than irq

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, 28 Feb 2022 15:29:32 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Eric Biggers <ebiggers@...nel.org>,
        Theodore Ts'o <tytso@....edu>,
        Dominik Brodowski <linux@...inikbrodowski.net>
Subject: Re: [PATCH v2] random: do crng pre-init loading in worker rather
 than irq

On 2022-02-28 15:17:19 [+0100], Jason A. Donenfeld wrote:
> Hey Sebastian,
Hi Jason,

> On 2/28/22, Sebastian Andrzej Siewior <bigeasy@...utronix.de> wrote:
> > On 2022-02-24 16:29:37 [+0100], Jason A. Donenfeld wrote:
> >> Taking spinlocks from IRQ context is problematic for PREEMPT_RT. That
> >> is, in part, why we take trylocks instead. But apparently this still
> >> trips up various lock dependency analyzers. That seems like a bug in the
> >> analyzers that should be fixed, rather than having to change things
> >> here.
> >
> > Could you please post a lockdep report so I can take a look?
> 
> I thought the problem with lockdep was stated by you somewhere in this thread?
> https://lore.kernel.org/lkml/YfOqsOiNfURyvFRX@linutronix.de/
> "But even then we need to find a way to move the crng init part
> (crng_fast_load()) out of the hard-IRQ."
> And Jonathan posted two related (?) splats he ran into.
> 
> I may have gotten that all wrong, in which case, I'll just excise that
> part from the commit message. I'm pretty sure you want this patch
> either way, right?

Oh, that report. So yes, I want that patch ;)

In this case the lockdep is right. The thing that it affects only
PREEMPT_RT.
That trylock is not the thing that lockdep complains about but the
spin_lock_irqsave() within invalidate_batched_entropy().

Taking a spinlock_t from IRQ context is problematic for PREEMPT_RT,
correct. A spin_try_lock() is also problematic since another spin_lock()
invocation would PI-boost the wrong task (the spin_try_lock() is invoked
from an IRQ-context so the task on CPU (random task or idle) is not the
actual owner). I'm pointing this out because there was also _another_
problem with try_lock from hard-IRQ context which was fixed in the
meantime.

Would it work for you to update the commit message? Basically I'm fine
with the firs sentence but the remaining part is misleading.

> Jason

Sebastian