[PATCH 1/2] random: access primary_pool directly rather than through pointer

[PATCH 1/2] random: access primary_pool directly rather than through pointer

[Dominik Brodowski]

Both crng_initialize_primary() and crng_init_try_arch_early() are
only called for the primary_pool. Accessing it directly instead of
through a function parameter simplifies the code.

Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/char/random.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 68613f0b6887..d332054bbbb6 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -762,7 +762,7 @@ static bool crng_init_try_arch(struct crng_state *crng)
 	return arch_init;
 }
 
-static bool __init crng_init_try_arch_early(struct crng_state *crng)
+static bool __init crng_init_try_arch_early(void)
 {
 	int i;
 	bool arch_init = true;
@@ -774,7 +774,7 @@ static bool __init crng_init_try_arch_early(struct crng_state *crng)
 			rv = random_get_entropy();
 			arch_init = false;
 		}
-		crng->state[i] ^= rv;
+		primary_crng.state[i] ^= rv;
 	}
 
 	return arch_init;
@@ -788,16 +788,16 @@ static void crng_initialize_secondary(struct crng_state *crng)
 	crng->init_time = jiffies - CRNG_RESEED_INTERVAL - 1;
 }
 
-static void __init crng_initialize_primary(struct crng_state *crng)
+static void __init crng_initialize_primary(void)
 {
-	_extract_entropy(&crng->state[4], sizeof(u32) * 12);
-	if (crng_init_try_arch_early(crng) && trust_cpu && crng_init < 2) {
+	_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;
 		pr_notice("crng init done (trusting CPU's manufacturer)\n");
 	}
-	crng->init_time = jiffies - CRNG_RESEED_INTERVAL - 1;
+	primary_crng.init_time = jiffies - CRNG_RESEED_INTERVAL - 1;
 }
 
 static void crng_finalize_init(struct crng_state *crng)
@@ -1698,7 +1698,7 @@ int __init rand_initialize(void)
 	init_std_data();
 	if (crng_need_final_init)
 		crng_finalize_init(&primary_crng);
-	crng_initialize_primary(&primary_crng);
+	crng_initialize_primary();
 	crng_global_init_time = jiffies;
 	if (ratelimit_disable) {
 		urandom_warning.interval = 0;
-- 
2.35.1

[PATCH 2/2] random: only call crng_finalize_init() for primary_crng

[Dominik Brodowski]

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

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

Re: [PATCH 1/2] random: access primary_pool directly rather than through pointer

[Jason A. Donenfeld]

Applied, thanks.

Re: [PATCH 2/2] random: only call crng_finalize_init() for primary_crng

[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.

Jason

Re: [PATCH 2/2] random: only call crng_finalize_init() for primary_crng

[Dominik Brodowski]

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