| Message ID | YgQOgqWr0nwqZCh6@owl.dominikbrodowski.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | random: fix locking for crng_init in crng_reseed() | expand |
Hi Dominik, Thanks, applied. I changed complete_init to finalize_init, to match our naming scheme from earlier, and I moved invalidate_batched_entropy() to outside the lock and after crng_init=2, since now it uses atomics, and it should probably be ordered after crng_init = 2, so the new batch gets the new entropy. Actually, though, come to think of it: shouldn't we always call invalidate_batched_entropy() after reseeding? More generally, we can instead probably tie the entropy generation counter to the base_crng counter, and have this all done automatically. That might be something interesting to do in the future. Jason
Hi Jason, Am Wed, Feb 09, 2022 at 10:39:17PM +0100 schrieb Jason A. Donenfeld: > Thanks, applied. I changed complete_init to finalize_init, to match > our naming scheme from earlier, and I moved > invalidate_batched_entropy() to outside the lock and after > crng_init=2, since now it uses atomics, and it should probably be > ordered after crng_init = 2, so the new batch gets the new entropy. Doesn't that mean that there is a small window where crng_init == 2, but get_random_u64/get_random_u32 still returns old data, with potentially insufficient entropy (as obtained at a time when crng_init was still < 2)? That's why I moved invalidate_batched_entropy() under the lock. But with your subsequent patch, it doesn't matter any more. Thanks, Dominik
Hi Dominik, Thanks, I see your point. I'll do it the way you suggested (which as you pointed out goes away anyway right after). Jason
On Wed, Feb 09, 2022 at 07:57:06PM +0100, Dominik Brodowski wrote: > crng_init is protected by primary_crng->lock. Therefore, we need > to hold this lock when increasing crng_init to 2. As we shouldn't > hold this lock for too long, only hold it for those parts which > require protection. > > Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net> > --- > drivers/char/random.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > Reviewed-by: Eric Biggers <ebiggers@google.com> Though to bikeshed on the variable name, I think that 'became_ready' would be more self-explanatory than 'complete_init' (this patch) and 'finalize_init' (the version committed). - Eric
diff --git a/drivers/char/random.c b/drivers/char/random.c index cc4d9d414df2..aee56032ebb4 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -497,6 +497,7 @@ static void crng_slow_load(const void *cp, unsigned int len) static void crng_reseed(void) { + bool complete_init = false; unsigned long flags; int entropy_count; unsigned long next_gen; @@ -526,12 +527,14 @@ static void crng_reseed(void) ++next_gen; WRITE_ONCE(base_crng.generation, next_gen); base_crng.birth = jiffies; - spin_unlock_irqrestore(&base_crng.lock, flags); - memzero_explicit(key, sizeof(key)); - if (crng_init < 2) { invalidate_batched_entropy(); crng_init = 2; + complete_init = true; + } + spin_unlock_irqrestore(&base_crng.lock, flags); + memzero_explicit(key, sizeof(key)); + if (complete_init) { process_random_ready_list(); wake_up_interruptible(&crng_init_wait); kill_fasync(&fasync, SIGIO, POLL_IN);
crng_init is protected by primary_crng->lock. Therefore, we need to hold this lock when increasing crng_init to 2. As we shouldn't hold this lock for too long, only hold it for those parts which require protection. Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net> --- drivers/char/random.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)