| Message ID | 20220211170732.571775-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v7] random: defer fast pool mixing to worker | expand |
On Fri, Feb 11, 2022 at 06:07:32PM +0100, Jason A. Donenfeld wrote: > On PREEMPT_RT, it's problematic to take spinlocks from hard irq > handlers. We can fix this by deferring to a workqueue the dumping of > the fast pool into the input pool. > > We accomplish this with some careful rules on fast_pool->count: > > - When it's incremented to >= 64, we schedule the work. > - If the top bit is set, we never schedule the work, even if >= 64. > - The worker is responsible for setting it back to 0 when it's done. > > There are two small issues around using workqueues for this purpose that > we work around. > > The first issue is that mix_interrupt_randomness() might be migrated to > another CPU during CPU hotplug. This issue is rectified by checking that > it hasn't been migrated (after disabling irqs). If it has been migrated, > then we set the count to zero, so that when the CPU comes online again, > it can requeue the work. As part of this, we switch to using an > atomic_t, so that the increment in the irq handler doesn't wipe out the > zeroing if the CPU comes back online while this worker is running. > > The second issue is that, though relatively minor in effect, we probably > want to make sure we get a consistent view of the pool onto the stack, > in case it's interrupted by an irq while reading. To do this, we don't > reenable irqs until after the copy. There are only 18 instructions > between the cli and sti, so this is a pretty tiny window. > > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Theodore Ts'o <tytso@mit.edu> > Cc: Sultan Alsawaf <sultan@kerneltoast.com> > Cc: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > Reviewed-by: Dominik Brodowski <linux@dominikbrodowski.net> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > Sebastian - as requested, we now disable irqs for a very short 18 > instructions rather than fishing into migrate_disable() and upsetting > PeterZ. Might this be the lucky patch? -Jason I think this might be the lucky patch. Reviewed-by: Sultan Alsawaf <sultan@kerneltoast.com> Sultan
On 2022-02-11 18:07:32 [+0100], Jason A. Donenfeld wrote: > On PREEMPT_RT, it's problematic to take spinlocks from hard irq > handlers. We can fix this by deferring to a workqueue the dumping of > the fast pool into the input pool. > > We accomplish this with some careful rules on fast_pool->count: > > - When it's incremented to >= 64, we schedule the work. > - If the top bit is set, we never schedule the work, even if >= 64. > - The worker is responsible for setting it back to 0 when it's done. > > There are two small issues around using workqueues for this purpose that > we work around. > > The first issue is that mix_interrupt_randomness() might be migrated to > another CPU during CPU hotplug. This issue is rectified by checking that > it hasn't been migrated (after disabling irqs). If it has been migrated, > then we set the count to zero, so that when the CPU comes online again, > it can requeue the work. As part of this, we switch to using an > atomic_t, so that the increment in the irq handler doesn't wipe out the > zeroing if the CPU comes back online while this worker is running. > > The second issue is that, though relatively minor in effect, we probably > want to make sure we get a consistent view of the pool onto the stack, > in case it's interrupted by an irq while reading. To do this, we don't > reenable irqs until after the copy. There are only 18 instructions > between the cli and sti, so this is a pretty tiny window. > > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Theodore Ts'o <tytso@mit.edu> > Cc: Sultan Alsawaf <sultan@kerneltoast.com> > Cc: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > Reviewed-by: Dominik Brodowski <linux@dominikbrodowski.net> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > Sebastian - as requested, we now disable irqs for a very short 18 > instructions rather than fishing into migrate_disable() and upsetting > PeterZ. Might this be the lucky patch? -Jason I think we good. I'm not going to comment on the 90 char wide comment :) Sebastian
diff --git a/drivers/char/random.c b/drivers/char/random.c index 1dbf21c02b40..b921127ea693 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -1162,9 +1162,10 @@ EXPORT_SYMBOL_GPL(add_bootloader_randomness); struct fast_pool { unsigned long pool[16 / sizeof(long)]; + struct work_struct mix; unsigned long last; + atomic_t count; u16 reg_idx; - u8 count; }; /* @@ -1214,12 +1215,49 @@ static u32 get_reg(struct fast_pool *f, struct pt_regs *regs) return *ptr; } +static void mix_interrupt_randomness(struct work_struct *work) +{ + struct fast_pool *fast_pool = container_of(work, struct fast_pool, mix); + unsigned long pool[ARRAY_SIZE(fast_pool->pool)]; + + /* Check to see if we're running on the wrong CPU due to hotplug. */ + local_irq_disable(); + if (fast_pool != this_cpu_ptr(&irq_randomness)) { + local_irq_enable(); + /* + * If we are unlucky enough to have been moved to another CPU, + * during CPU hotplug while the CPU was shutdown then we set + * our count to zero atomically so that when the CPU comes + * back online, it can enqueue work again. The _release here + * pairs with the atomic_inc_return_acquire in + * add_interrupt_randomness(). + */ + atomic_set_release(&fast_pool->count, 0); + return; + } + + /* + * Copy the pool to the stack so that the mixer always has a + * consistent view, before we reenable irqs again. + */ + memcpy(pool, fast_pool->pool, sizeof(pool)); + atomic_set(&fast_pool->count, 0); + fast_pool->last = jiffies; + local_irq_enable(); + + mix_pool_bytes(pool, sizeof(pool)); + credit_entropy_bits(1); + memzero_explicit(pool, sizeof(pool)); +} + void add_interrupt_randomness(int irq) { + enum { MIX_INFLIGHT = 1U << 31 }; struct fast_pool *fast_pool = this_cpu_ptr(&irq_randomness); struct pt_regs *regs = get_irq_regs(); unsigned long now = jiffies; cycles_t cycles = random_get_entropy(); + unsigned int new_count; if (cycles == 0) cycles = get_reg(fast_pool, regs); @@ -1235,12 +1273,13 @@ void add_interrupt_randomness(int irq) } fast_mix((u32 *)fast_pool->pool); - ++fast_pool->count; + /* The _acquire here pairs with the atomic_set_release in mix_interrupt_randomness(). */ + new_count = (unsigned int)atomic_inc_return_acquire(&fast_pool->count); if (unlikely(crng_init == 0)) { - if (fast_pool->count >= 64 && + if (new_count >= 64 && crng_fast_load(fast_pool->pool, sizeof(fast_pool->pool)) > 0) { - fast_pool->count = 0; + atomic_set(&fast_pool->count, 0); fast_pool->last = now; /* @@ -1254,20 +1293,16 @@ void add_interrupt_randomness(int irq) return; } - if ((fast_pool->count < 64) && !time_after(now, fast_pool->last + HZ)) + if (new_count & MIX_INFLIGHT) return; - if (!spin_trylock(&input_pool.lock)) + if (new_count < 64 && !time_after(now, fast_pool->last + HZ)) return; - fast_pool->last = now; - _mix_pool_bytes(&fast_pool->pool, sizeof(fast_pool->pool)); - spin_unlock(&input_pool.lock); - - fast_pool->count = 0; - - /* Award one bit for the contents of the fast pool. */ - credit_entropy_bits(1); + if (unlikely(!fast_pool->mix.func)) + INIT_WORK(&fast_pool->mix, mix_interrupt_randomness); + atomic_or(MIX_INFLIGHT, &fast_pool->count); + queue_work_on(raw_smp_processor_id(), system_highpri_wq, &fast_pool->mix); } EXPORT_SYMBOL_GPL(add_interrupt_randomness);