linux-kernel - Re: [PATCH RFC v1] random: do not take spinlocks in irq handler

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: <CAHmME9oYj6tp+THaz_74SWikpTLSzukH-DynypB+7SZ56hucog@mail.gmail.com>
Date:   Fri, 4 Feb 2022 23:08:35 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        "Theodore Ts'o" <tytso@....edu>,
        Sultan Alsawaf <sultan@...neltoast.com>,
        Jonathan Neuschäfer <j.neuschaefer@....net>
Subject: Re: [PATCH RFC v1] random: do not take spinlocks in irq handler

Hi Sebastian,

On Fri, Feb 4, 2022 at 9:47 PM Sebastian Andrzej Siewior
<bigeasy@...utronix.de> wrote:
> No need for atomic. If this is truly per-CPU then there will be no
> cross-CPU access, right?
> Therefore I would suggest to use __this_cpu_inc_return() which would avoid
> the sync prefix for the inc operation. Same for __this_cpu_or(). And you
> could use unsigned int.

Good thinking. Done.

>
> > +             schedule_work(&fast_pool->mix);
>
> schedule_work() has a check which ensures that the work is not scheduled
> again if still pending. But we could consider it fast-path and say that
> it makes sense to keep it.

The PENDING flag is cleared after the work item has been assigned to a
worker, but _before_ the function has actually run. This means you
might wind up scheduling for that work to run again while it's already
running, which isn't what we want. So ORing in that flag lets us track
things until the end of that function.

> You could use schedule_work_on() so it remains on the local CPU. Makes
> probably even more sense on NUMA systems. Otherwise it is an unbound
> worker and the scheduler (and even the worker)_could_ move it around.

Was just thinking about that too. Will do.

> With schedule_work_on() it still _could_ be moved to another CPU during
> CPU hotplug. Therefore you should check in mix_interrupt_randomness() if
> the worker is == this_cpu_ptr() and otherwise abort. Puh this asks for a
> CPU-hotplug handler to reset FAST_POOL_MIX_INFLIGHT in the CPU-UP path.
> That would be the price for using this_cpu_inc()/or ;)

Not a fan of adding a hotplug handler. However, actually I think we
don't need the worker==this_cpu_ptr() check. If a CPU is taken
offline, yet that CPU contributed a healthy set of IRQs to the fast
pool such that the worker got queued up, then we should just go ahead
and ingest them. In the worst case, the CPU comes back online midway
and contributes a little bit more than usual, but this is already the
race we have with the worker itself being interrupted by an irq, so it
seems fine.

> This might even classify for using system_highpri_wq (e.g.
>   queue_work_on(raw_smp_processor_id(), system_highpri_wq, &fast_pool->mix);

Good thinking.

Re:crng_fast_load, you said:
> RT wise we _could_ acquire that spinlock_t in IRQ context early during
> boot as long as system_state < SYSTEM_SCHEDULING. After that, we could
> dead lock.

Unfortunately, I don't think that's a guarantee we have on all systems
:-\. I'll try to think more carefully through the consequences of just
_removing those spinlocks_ and letting it race, and see if there's a
way that makes sense. I'm not sure it does, but I'll need to put my
thinking cap on. Either way, it seems like that's now a third puzzle
we'll wind up addressing separately.

I'll roll a v2.

Jason