| Message ID | 20220130210320.3997-2-linux@dominikbrodowski.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [1/2] random: access primary_pool directly rather than through pointer | expand |
Thanks, I'll apply this. I do wonder, though, do we have locking concerns around crng_init transitioning from 1 to 2, or with calls to crng_need_final_init? For example, can crng_reseed be called at the same time as rand_initialize? Or are we still single core at this point in the boot sequence? I don't think that this patch changes anything from that perspective, which is why it seems reasonable to apply, but I do wonder. Jason
Am Sun, Jan 30, 2022 at 11:11:22PM +0100 schrieb Jason A. Donenfeld: > Thanks, I'll apply this. I do wonder, though, do we have locking > concerns around crng_init transitioning from 1 to 2, or with calls to > crng_need_final_init? For example, can crng_reseed be called at the > same time as rand_initialize? Or are we still single core at this > point in the boot sequence? I don't think that this patch changes > anything from that perspective, which is why it seems reasonable to > apply, but I do wonder. Well, the comment * crng_init is protected by primary_crng->lock is currently not adhered to. It's unproblematic to set it at rand_initialize() time (by calling crng_finalize_init()), as the system is still running with IRQs disabled and only the boot CPU active (but not yet in PID 1). So its call to crng_finalize_init() will not race with crng_reseed() calling crng_finalize_init(). However, I think the other sites setting crng_init - crng_reseed() calling crng_finalize_init() - crng_fast_load() might race, in particular two parallel calls to crng_reseed(). So let's try to keep the promise to increase[*] crng_init only while holding primary_crng->lock. UNTESTED, not even compile-tested patch below. What do you think? Thanks, Dominik [*] The read sites still need to be checked, but at a first glance, I did not notice any obvious problematic code. diff --git a/drivers/char/random.c b/drivers/char/random.c index 7ed910c23858..e21c73cadcc2 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -465,7 +465,7 @@ static struct crng_state primary_crng = { * its value (from 0->1->2). */ static int crng_init = 0; -static bool crng_need_final_init = false; +static bool crng_needs_numa_init = false; #define crng_ready() (likely(crng_init > 1)) static int crng_init_cnt = 0; static unsigned long crng_global_init_time = 0; @@ -788,31 +788,29 @@ static void crng_initialize_secondary(struct crng_state *crng) crng->init_time = jiffies - CRNG_RESEED_INTERVAL - 1; } +static void crng_finalize_init(void) +{ + invalidate_batched_entropy(); + /* We can't call numa_crng_init() until we have workqueues, + * but we will pick this up in rand_initialize() */ + if (system_wq) + numa_crng_init(); + else + crng_needs_numa_init = true; + crng_init = 2; +} + static void __init crng_initialize_primary(void) { _extract_entropy(&primary_crng.state[4], sizeof(u32) * 12); if (crng_init_try_arch_early() && trust_cpu && crng_init < 2) { - invalidate_batched_entropy(); - numa_crng_init(); - crng_init = 2; + crng_finalize_init(); pr_notice("crng init done (trusting CPU's manufacturer)\n"); } primary_crng.init_time = jiffies - CRNG_RESEED_INTERVAL - 1; } -static void crng_finalize_init(void) -{ - if (!system_wq) { - /* We can't call numa_crng_init until we have workqueues, - * so mark this for processing later. */ - crng_need_final_init = true; - return; - } - - invalidate_batched_entropy(); - numa_crng_init(); - crng_init = 2; - crng_need_final_init = false; +static void crng_late_init(void) { process_random_ready_list(); wake_up_interruptible(&crng_init_wait); kill_fasync(&fasync, SIGIO, POLL_IN); @@ -896,12 +894,13 @@ static size_t crng_fast_load(const u8 *cp, size_t len) p[crng_init_cnt % CHACHA_KEY_SIZE] ^= *cp; cp++; crng_init_cnt++; len--; ret++; } - spin_unlock_irqrestore(&primary_crng.lock, flags); if (crng_init_cnt >= CRNG_INIT_CNT_THRESH) { invalidate_batched_entropy(); crng_init = 1; - pr_notice("fast init done\n"); } + spin_unlock_irqrestore(&primary_crng.lock, flags); + if (crng_init == 1) + pr_notice("fast init done\n"); return ret; } @@ -954,6 +953,7 @@ static void crng_reseed(struct crng_state *crng, bool use_input_pool) { unsigned long flags; int i, num; + bool needs_late_init = false; union { u8 block[CHACHA_BLOCK_SIZE]; u32 key[8]; @@ -978,9 +978,17 @@ static void crng_reseed(struct crng_state *crng, bool use_input_pool) } memzero_explicit(&buf, sizeof(buf)); WRITE_ONCE(crng->init_time, jiffies); - spin_unlock_irqrestore(&crng->lock, flags); - if (crng == &primary_crng && crng_init < 2) + if (crng == &primary_crng && crng_init < 2) { crng_finalize_init(); + /* crng_late_init() is only needed if crng_init progresses to 2 + * after rand_initialize(). Note that while userspace may reset + * crng_global_init_time to 0, it cannot reset crng_init to 2 */ + if (crng_global_init_time > 0) + needs_late_init = true; + } + spin_unlock_irqrestore(&crng->lock, flags); + if (needs_late_init) + crng_late_init(); } static void _extract_crng(struct crng_state *crng, u8 out[CHACHA_BLOCK_SIZE]) @@ -1696,8 +1704,8 @@ static void __init init_std_data(void) int __init rand_initialize(void) { init_std_data(); - if (crng_need_final_init) - crng_finalize_init(); + if (crng_needs_numa_init) + numa_crng_init(); crng_initialize_primary(); crng_global_init_time = jiffies; if (ratelimit_disable) {
diff --git a/drivers/char/random.c b/drivers/char/random.c index d332054bbbb6..7ed910c23858 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -800,10 +800,8 @@ static void __init crng_initialize_primary(void) primary_crng.init_time = jiffies - CRNG_RESEED_INTERVAL - 1; } -static void crng_finalize_init(struct crng_state *crng) +static void crng_finalize_init(void) { - if (crng != &primary_crng || crng_init >= 2) - return; if (!system_wq) { /* We can't call numa_crng_init until we have workqueues, * so mark this for processing later. */ @@ -814,6 +812,7 @@ static void crng_finalize_init(struct crng_state *crng) invalidate_batched_entropy(); numa_crng_init(); crng_init = 2; + crng_need_final_init = false; process_random_ready_list(); wake_up_interruptible(&crng_init_wait); kill_fasync(&fasync, SIGIO, POLL_IN); @@ -980,7 +979,8 @@ static void crng_reseed(struct crng_state *crng, bool use_input_pool) memzero_explicit(&buf, sizeof(buf)); WRITE_ONCE(crng->init_time, jiffies); spin_unlock_irqrestore(&crng->lock, flags); - crng_finalize_init(crng); + if (crng == &primary_crng && crng_init < 2) + crng_finalize_init(); } static void _extract_crng(struct crng_state *crng, u8 out[CHACHA_BLOCK_SIZE]) @@ -1697,7 +1697,7 @@ int __init rand_initialize(void) { init_std_data(); if (crng_need_final_init) - crng_finalize_init(&primary_crng); + crng_finalize_init(); crng_initialize_primary(); crng_global_init_time = jiffies; if (ratelimit_disable) {
crng_finalize_init() returns instantly if it is called for another pool than primary_crng. The test whether crng_finalize_init() is still required can be moved to the relevant caller in crng_reseed(), and crng_need_final_init can be reset to false if crng_finalize_init() is called with workqueues ready. Then, no previous callsite will call crng_finalize_init() unless it is needed, and we can get rid of the superfluous function parameter. Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net> --- drivers/char/random.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)