| Message ID | 20221106015042.98538-1-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v2] hw_random: use add_hwgenerator_randomness() for early entropy | expand |
Am Sun, Nov 06, 2022 at 02:50:42AM +0100 schrieb Jason A. Donenfeld: > Rather than calling add_device_randomness(), the add_early_randomness() > function should use add_hwgenerator_randomness(), so that the early > entropy can be potentially credited, which allows for the RNG to > initialize earlier without having to wait for the kthread to come up. We're already at device_initcall() level here, so that shouldn't be much of an additional delay. > Since we don't want to sleep from add_early_randomness(), we also > refactor the API a bit so that hw_random/core.c can do the sleep, this > time using the correct function, hwrng_msleep, rather than having > random.c awkwardly do it. Isn't this something you were quite hesistant about just recently[*]? | I was thinking the other day that under certain circumstances, it | would be nice if random.c could ask hwrng for more bytes NOW, rather | than waiting. With the code as it is currently, this could be | accomplished by having a completion event or something similar to | that. With your proposed change, now it's left up to the hwrng | interface to handle. | | That's not the end of the world, but it does mean we'd have to come up | with a patch down the line that exports a hwrng function saying, "stop | the delays and schedule the worker NOW". Now impossible, just more | complex, as now the state flow is split across two places. Wondering | if you have any thoughts about this. Thanks, Dominik [*] https://lore.kernel.org/lkml/CAHmME9rhunb05DEnc=UfGr8k9_LBi1NW2Hi0OsRbGwcCN2NzjQ@mail.gmail.com/
Hi Dominik, On Sun, Nov 06, 2022 at 08:02:56AM +0100, Dominik Brodowski wrote: > Am Sun, Nov 06, 2022 at 02:50:42AM +0100 schrieb Jason A. Donenfeld: > > Rather than calling add_device_randomness(), the add_early_randomness() > > function should use add_hwgenerator_randomness(), so that the early > > entropy can be potentially credited, which allows for the RNG to > > initialize earlier without having to wait for the kthread to come up. > > We're already at device_initcall() level here, so that shouldn't be much of > an additional delay. Either the delay is not relevant, in which case we should entirely remove `add_early_randomness()`, or the delay is relevant, in which case hw_random should use the right function, so that it gets credited as it should. (Semantically this is the right thing to do, too, so that random.c can actually know what the deal is with the data it's getting; knowing, "oh this comes from hw_random" could be useful.) > > Since we don't want to sleep from add_early_randomness(), we also > > refactor the API a bit so that hw_random/core.c can do the sleep, this > > time using the correct function, hwrng_msleep, rather than having > > random.c awkwardly do it. > > Isn't this something you were quite hesistant about just recently[*]? It is, yes, thanks. The concern is that now it means random.c can't ask for more hw_random data by canceling that sleep. I was thinking that it would be nice to cancel the sleep during system resume, vm fork, and other similar events, so that we can get some fresh bits during the times it matters most. But there's a bit of a problem of doing it in a basic way like that: we might get the bits, but that doesn't mean we'll reseed right away at that advanced stage in uptime. So we'd actually need something more complicated like, "unblock from sleep now and reseed after mixing data". When writing this patch last night, I was thinking this is a more complicated thing that could benefit from a more complicated API. But actually, maybe there's a way I can make it work for the existing thing. So you're right: let's hold off on this v2. And I'll send a v3 that just adds a boolean parameter of whether or not to sleep. And then I'll also try tackling the unblock&reseed thing too at the same time. Jason
Hi Jason, Am Sun, Nov 06, 2022 at 03:50:51PM +0100 schrieb Jason A. Donenfeld: > On Sun, Nov 06, 2022 at 08:02:56AM +0100, Dominik Brodowski wrote: > > Am Sun, Nov 06, 2022 at 02:50:42AM +0100 schrieb Jason A. Donenfeld: > > > Rather than calling add_device_randomness(), the add_early_randomness() > > > function should use add_hwgenerator_randomness(), so that the early > > > entropy can be potentially credited, which allows for the RNG to > > > initialize earlier without having to wait for the kthread to come up. > > > > We're already at device_initcall() level here, so that shouldn't be much of > > an additional delay. > > Either the delay is not relevant, in which case we should entirely > remove `add_early_randomness()`, There's another subtlety going on here: add_device_randomness() is called for *all* hw_random devices upon their registration, while the hwrng thread currently only works with the hw_random device with the best quality available. Thanks, Dominik
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c index cc002b0c2f0c..f6bbe0d8b95d 100644 --- a/drivers/char/hw_random/core.c +++ b/drivers/char/hw_random/core.c @@ -69,8 +69,10 @@ static void add_early_randomness(struct hwrng *rng) mutex_lock(&reading_mutex); bytes_read = rng_get_data(rng, rng_fillbuf, 32, 0); mutex_unlock(&reading_mutex); - if (bytes_read > 0) - add_device_randomness(rng_fillbuf, bytes_read); + if (bytes_read > 0) { + size_t entropy = bytes_read * 8 * rng->quality / 1024; + add_hwgenerator_randomness(rng_fillbuf, bytes_read, entropy); + } } static inline void cleanup_rng(struct kref *kref) @@ -499,6 +501,7 @@ static int hwrng_fillfn(void *unused) while (!kthread_should_stop()) { unsigned short quality; struct hwrng *rng; + unsigned long sleep; rng = get_current_rng(); if (IS_ERR(rng) || !rng) @@ -527,8 +530,10 @@ static int hwrng_fillfn(void *unused) entropy_credit = entropy; /* Outside lock, sure, but y'know: randomness. */ - add_hwgenerator_randomness((void *)rng_fillbuf, rc, - entropy >> 10); + sleep = add_hwgenerator_randomness((void *)rng_fillbuf, rc, + entropy >> 10); + if (sleep) + hwrng_msleep(rng, jiffies_to_msecs(sleep)); } hwrng_fill = NULL; return 0; diff --git a/drivers/char/random.c b/drivers/char/random.c index 4591d55cb135..16ee170151a9 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -893,9 +893,9 @@ EXPORT_SYMBOL(add_device_randomness); /* * Interface for in-kernel drivers of true hardware RNGs. * Those devices may produce endless random bits and will be throttled - * when our pool is full. + * when our pool is full. Returns suggested sleep time in jiffies. */ -void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy) +unsigned long add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy) { mix_pool_bytes(buf, len); credit_init_bits(entropy); @@ -904,8 +904,9 @@ void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy) * Throttle writing to once every reseed interval, unless we're not yet * initialized or no entropy is credited. */ - if (!kthread_should_stop() && (crng_ready() || !entropy)) - schedule_timeout_interruptible(crng_reseed_interval()); + if (crng_ready() || !entropy) + return crng_reseed_interval(); + return 0; } EXPORT_SYMBOL_GPL(add_hwgenerator_randomness); diff --git a/include/linux/random.h b/include/linux/random.h index 2bdd3add3400..02ef65fc02c2 100644 --- a/include/linux/random.h +++ b/include/linux/random.h @@ -17,7 +17,7 @@ void __init add_bootloader_randomness(const void *buf, size_t len); void add_input_randomness(unsigned int type, unsigned int code, unsigned int value) __latent_entropy; void add_interrupt_randomness(int irq) __latent_entropy; -void add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy); +unsigned long add_hwgenerator_randomness(const void *buf, size_t len, size_t entropy); #if defined(LATENT_ENTROPY_PLUGIN) && !defined(__CHECKER__) static inline void add_latent_entropy(void)
Rather than calling add_device_randomness(), the add_early_randomness() function should use add_hwgenerator_randomness(), so that the early entropy can be potentially credited, which allows for the RNG to initialize earlier without having to wait for the kthread to come up. Since we don't want to sleep from add_early_randomness(), we also refactor the API a bit so that hw_random/core.c can do the sleep, this time using the correct function, hwrng_msleep, rather than having random.c awkwardly do it. Cc: Herbert Xu <herbert@gondor.apana.org.au> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- drivers/char/hw_random/core.c | 13 +++++++++---- drivers/char/random.c | 9 +++++---- include/linux/random.h | 2 +- 3 files changed, 15 insertions(+), 9 deletions(-)