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) {
