All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] random: use symbolic constants for crng_init states
@ 2022-05-08 11:26 Jason A. Donenfeld
  2022-05-08 12:57 ` Joe Perches
  0 siblings, 1 reply; 5+ messages in thread
From: Jason A. Donenfeld @ 2022-05-08 11:26 UTC (permalink / raw)
  To: linux-kernel; +Cc: Jason A. Donenfeld, Dominik Brodowski

crng_init represents a state machine, with three states, and various
rules for transitions. For the longest time, we've been managing these
with "0", "1", and "2", and expecting people to figure it out. To make
the code more obvious, replace these with proper enum values
representing the transition, and then redocument what each of these
states mean.

Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 2af7a755d632..8f4a4452b31b 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -72,16 +72,16 @@
  *********************************************************************/
 
 /*
- * crng_init =  0 --> Uninitialized
- *		1 --> Initialized
- *		2 --> Initialized from input_pool
- *
  * crng_init is protected by base_crng->lock, and only increases
  * its value (from 0->1->2).
  */
-static int crng_init = 0;
-#define crng_ready() (likely(crng_init > 1))
-/* Various types of waiters for crng_init->2 transition. */
+static enum {
+	CRNG_EMPTY = 0, /* Little to no entropy collected */
+	CRNG_EARLY = 1, /* At least POOL_EARLY_BITS collected */
+	CRNG_READY = 2  /* Fully iniitalized with POOL_READY_BITS collected */
+} crng_init = CRNG_EMPTY;
+#define crng_ready() (likely(crng_init >= CRNG_READY))
+/* Various types of waiters for crng_init->CRNG_READY transition. */
 static DECLARE_WAIT_QUEUE_HEAD(crng_init_wait);
 static struct fasync_struct *fasync;
 static DEFINE_SPINLOCK(random_ready_chain_lock);
@@ -284,7 +284,7 @@ static void crng_reseed(void)
 	WRITE_ONCE(base_crng.generation, next_gen);
 	WRITE_ONCE(base_crng.birth, jiffies);
 	if (!crng_ready()) {
-		crng_init = 2;
+		crng_init = CRNG_READY;
 		finalize_init = true;
 	}
 	spin_unlock_irqrestore(&base_crng.lock, flags);
@@ -378,7 +378,7 @@ static void crng_make_state(u32 chacha_state[CHACHA_STATE_WORDS],
 	 * For the fast path, we check whether we're ready, unlocked first, and
 	 * then re-check once locked later. In the case where we're really not
 	 * ready, we do fast key erasure with the base_crng directly, extracting
-	 * when crng_init==0.
+	 * when crng_init==CRNG_EMPTY.
 	 */
 	if (!crng_ready()) {
 		bool ready;
@@ -386,7 +386,7 @@ static void crng_make_state(u32 chacha_state[CHACHA_STATE_WORDS],
 		spin_lock_irqsave(&base_crng.lock, flags);
 		ready = crng_ready();
 		if (!ready) {
-			if (crng_init == 0)
+			if (crng_init == CRNG_EMPTY)
 				extract_entropy(base_crng.key, sizeof(base_crng.key));
 			crng_fast_key_erasure(base_crng.key, chacha_state,
 					      random_data, random_data_len);
@@ -740,8 +740,8 @@ EXPORT_SYMBOL(get_random_bytes_arch);
 
 enum {
 	POOL_BITS = BLAKE2S_HASH_SIZE * 8,
-	POOL_INIT_BITS = POOL_BITS, /* No point in settling for less. */
-	POOL_FAST_INIT_BITS = POOL_INIT_BITS / 2
+	POOL_READY_BITS = POOL_BITS, /* When crng_init->CRNG_READY */
+	POOL_EARLY_BITS = POOL_READY_BITS / 2 /* When crng_init->CRNG_EARLY */
 };
 
 static struct {
@@ -836,13 +836,13 @@ static void credit_init_bits(size_t nbits)
 		init_bits = min_t(unsigned int, POOL_BITS, orig + add);
 	} while (cmpxchg(&input_pool.init_bits, orig, init_bits) != orig);
 
-	if (!crng_ready() && init_bits >= POOL_INIT_BITS)
+	if (!crng_ready() && init_bits >= POOL_READY_BITS)
 		crng_reseed();
-	else if (unlikely(crng_init == 0 && init_bits >= POOL_FAST_INIT_BITS)) {
+	else if (unlikely(crng_init == CRNG_EMPTY && init_bits >= POOL_EARLY_BITS)) {
 		spin_lock_irqsave(&base_crng.lock, flags);
-		if (crng_init == 0) {
+		if (crng_init == CRNG_EMPTY) {
 			extract_entropy(base_crng.key, sizeof(base_crng.key));
-			crng_init = 1;
+			crng_init = CRNG_EARLY;
 		}
 		spin_unlock_irqrestore(&base_crng.lock, flags);
 	}
@@ -1595,7 +1595,7 @@ const struct file_operations urandom_fops = {
  *
  * - write_wakeup_threshold - the amount of entropy in the input pool
  *   below which write polls to /dev/random will unblock, requesting
- *   more entropy, tied to the POOL_INIT_BITS constant. It is writable
+ *   more entropy, tied to the POOL_READY_BITS constant. It is writable
  *   to avoid breaking old userspaces, but writing to it does not
  *   change any behavior of the RNG.
  *
@@ -1610,7 +1610,7 @@ const struct file_operations urandom_fops = {
 #include <linux/sysctl.h>
 
 static int sysctl_random_min_urandom_seed = CRNG_RESEED_INTERVAL / HZ;
-static int sysctl_random_write_wakeup_bits = POOL_INIT_BITS;
+static int sysctl_random_write_wakeup_bits = POOL_READY_BITS;
 static int sysctl_poolsize = POOL_BITS;
 static u8 sysctl_bootid[UUID_SIZE];
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] random: use symbolic constants for crng_init states
  2022-05-08 11:26 [PATCH] random: use symbolic constants for crng_init states Jason A. Donenfeld
@ 2022-05-08 12:57 ` Joe Perches
  2022-05-08 17:23   ` Jason A. Donenfeld
  0 siblings, 1 reply; 5+ messages in thread
From: Joe Perches @ 2022-05-08 12:57 UTC (permalink / raw)
  To: Jason A. Donenfeld, linux-kernel; +Cc: Dominik Brodowski

On Sun, 2022-05-08 at 13:26 +0200, Jason A. Donenfeld wrote:
> crng_init represents a state machine, with three states, and various
> rules for transitions. For the longest time, we've been managing these
> with "0", "1", and "2", and expecting people to figure it out. To make
> the code more obvious, replace these with proper enum values
> representing the transition, and then redocument what each of these
> states mean.

good idea

> diff --git a/drivers/char/random.c b/drivers/char/random.c
[]
> @@ -72,16 +72,16 @@
[]
> -/* Various types of waiters for crng_init->2 transition. */
> +static enum {
> +	CRNG_EMPTY = 0, /* Little to no entropy collected */
> +	CRNG_EARLY = 1, /* At least POOL_EARLY_BITS collected */
> +	CRNG_READY = 2  /* Fully iniitalized with POOL_READY_BITS collected */

typo: initialized

>  enum {
>  	POOL_BITS = BLAKE2S_HASH_SIZE * 8,
> -	POOL_INIT_BITS = POOL_BITS, /* No point in settling for less. */
> -	POOL_FAST_INIT_BITS = POOL_INIT_BITS / 2
> +	POOL_READY_BITS = POOL_BITS, /* When crng_init->CRNG_READY */
> +	POOL_EARLY_BITS = POOL_READY_BITS / 2 /* When crng_init->CRNG_EARLY */

Seems odd to use a divisor with an enum



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] random: use symbolic constants for crng_init states
  2022-05-08 12:57 ` Joe Perches
@ 2022-05-08 17:23   ` Jason A. Donenfeld
  2022-05-08 17:36     ` [PATCH v2] " Jason A. Donenfeld
  0 siblings, 1 reply; 5+ messages in thread
From: Jason A. Donenfeld @ 2022-05-08 17:23 UTC (permalink / raw)
  To: Joe Perches; +Cc: linux-kernel, Dominik Brodowski

On Sun, May 08, 2022 at 05:57:18AM -0700, Joe Perches wrote:
> > diff --git a/drivers/char/random.c b/drivers/char/random.c
> []
> > @@ -72,16 +72,16 @@
> []
> > -/* Various types of waiters for crng_init->2 transition. */
> > +static enum {
> > +	CRNG_EMPTY = 0, /* Little to no entropy collected */
> > +	CRNG_EARLY = 1, /* At least POOL_EARLY_BITS collected */
> > +	CRNG_READY = 2  /* Fully iniitalized with POOL_READY_BITS collected */
> 
> typo: initialized

Thanks, will do.

> 
> >  enum {
> >  	POOL_BITS = BLAKE2S_HASH_SIZE * 8,
> > -	POOL_INIT_BITS = POOL_BITS, /* No point in settling for less. */
> > -	POOL_FAST_INIT_BITS = POOL_INIT_BITS / 2
> > +	POOL_READY_BITS = POOL_BITS, /* When crng_init->CRNG_READY */
> > +	POOL_EARLY_BITS = POOL_READY_BITS / 2 /* When crng_init->CRNG_EARLY */
> 
> Seems odd to use a divisor with an enum

Why? The constants are defined in terms of other constants.

Jason

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2] random: use symbolic constants for crng_init states
  2022-05-08 17:23   ` Jason A. Donenfeld
@ 2022-05-08 17:36     ` Jason A. Donenfeld
  2022-05-09  6:06       ` Dominik Brodowski
  0 siblings, 1 reply; 5+ messages in thread
From: Jason A. Donenfeld @ 2022-05-08 17:36 UTC (permalink / raw)
  To: linux-kernel; +Cc: Jason A. Donenfeld, Dominik Brodowski, Joe Perches

crng_init represents a state machine, with three states, and various
rules for transitions. For the longest time, we've been managing these
with "0", "1", and "2", and expecting people to figure it out. To make
the code more obvious, replace these with proper enum values
representing the transition, and then redocument what each of these
states mean.

Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 2af7a755d632..8af29507ae0d 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -72,16 +72,16 @@
  *********************************************************************/
 
 /*
- * crng_init =  0 --> Uninitialized
- *		1 --> Initialized
- *		2 --> Initialized from input_pool
- *
  * crng_init is protected by base_crng->lock, and only increases
- * its value (from 0->1->2).
+ * its value (from empty->early->ready).
  */
-static int crng_init = 0;
-#define crng_ready() (likely(crng_init > 1))
-/* Various types of waiters for crng_init->2 transition. */
+static enum {
+	CRNG_EMPTY = 0, /* Little to no entropy collected */
+	CRNG_EARLY = 1, /* At least POOL_EARLY_BITS collected */
+	CRNG_READY = 2  /* Fully initialized with POOL_READY_BITS collected */
+} crng_init = CRNG_EMPTY;
+#define crng_ready() (likely(crng_init >= CRNG_READY))
+/* Various types of waiters for crng_init->CRNG_READY transition. */
 static DECLARE_WAIT_QUEUE_HEAD(crng_init_wait);
 static struct fasync_struct *fasync;
 static DEFINE_SPINLOCK(random_ready_chain_lock);
@@ -284,7 +284,7 @@ static void crng_reseed(void)
 	WRITE_ONCE(base_crng.generation, next_gen);
 	WRITE_ONCE(base_crng.birth, jiffies);
 	if (!crng_ready()) {
-		crng_init = 2;
+		crng_init = CRNG_READY;
 		finalize_init = true;
 	}
 	spin_unlock_irqrestore(&base_crng.lock, flags);
@@ -378,7 +378,7 @@ static void crng_make_state(u32 chacha_state[CHACHA_STATE_WORDS],
 	 * For the fast path, we check whether we're ready, unlocked first, and
 	 * then re-check once locked later. In the case where we're really not
 	 * ready, we do fast key erasure with the base_crng directly, extracting
-	 * when crng_init==0.
+	 * when crng_init==CRNG_EMPTY.
 	 */
 	if (!crng_ready()) {
 		bool ready;
@@ -386,7 +386,7 @@ static void crng_make_state(u32 chacha_state[CHACHA_STATE_WORDS],
 		spin_lock_irqsave(&base_crng.lock, flags);
 		ready = crng_ready();
 		if (!ready) {
-			if (crng_init == 0)
+			if (crng_init == CRNG_EMPTY)
 				extract_entropy(base_crng.key, sizeof(base_crng.key));
 			crng_fast_key_erasure(base_crng.key, chacha_state,
 					      random_data, random_data_len);
@@ -740,8 +740,8 @@ EXPORT_SYMBOL(get_random_bytes_arch);
 
 enum {
 	POOL_BITS = BLAKE2S_HASH_SIZE * 8,
-	POOL_INIT_BITS = POOL_BITS, /* No point in settling for less. */
-	POOL_FAST_INIT_BITS = POOL_INIT_BITS / 2
+	POOL_READY_BITS = POOL_BITS, /* When crng_init->CRNG_READY */
+	POOL_EARLY_BITS = POOL_READY_BITS / 2 /* When crng_init->CRNG_EARLY */
 };
 
 static struct {
@@ -836,13 +836,13 @@ static void credit_init_bits(size_t nbits)
 		init_bits = min_t(unsigned int, POOL_BITS, orig + add);
 	} while (cmpxchg(&input_pool.init_bits, orig, init_bits) != orig);
 
-	if (!crng_ready() && init_bits >= POOL_INIT_BITS)
+	if (!crng_ready() && init_bits >= POOL_READY_BITS)
 		crng_reseed();
-	else if (unlikely(crng_init == 0 && init_bits >= POOL_FAST_INIT_BITS)) {
+	else if (unlikely(crng_init == CRNG_EMPTY && init_bits >= POOL_EARLY_BITS)) {
 		spin_lock_irqsave(&base_crng.lock, flags);
-		if (crng_init == 0) {
+		if (crng_init == CRNG_EMPTY) {
 			extract_entropy(base_crng.key, sizeof(base_crng.key));
-			crng_init = 1;
+			crng_init = CRNG_EARLY;
 		}
 		spin_unlock_irqrestore(&base_crng.lock, flags);
 	}
@@ -1595,7 +1595,7 @@ const struct file_operations urandom_fops = {
  *
  * - write_wakeup_threshold - the amount of entropy in the input pool
  *   below which write polls to /dev/random will unblock, requesting
- *   more entropy, tied to the POOL_INIT_BITS constant. It is writable
+ *   more entropy, tied to the POOL_READY_BITS constant. It is writable
  *   to avoid breaking old userspaces, but writing to it does not
  *   change any behavior of the RNG.
  *
@@ -1610,7 +1610,7 @@ const struct file_operations urandom_fops = {
 #include <linux/sysctl.h>
 
 static int sysctl_random_min_urandom_seed = CRNG_RESEED_INTERVAL / HZ;
-static int sysctl_random_write_wakeup_bits = POOL_INIT_BITS;
+static int sysctl_random_write_wakeup_bits = POOL_READY_BITS;
 static int sysctl_poolsize = POOL_BITS;
 static u8 sysctl_bootid[UUID_SIZE];
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] random: use symbolic constants for crng_init states
  2022-05-08 17:36     ` [PATCH v2] " Jason A. Donenfeld
@ 2022-05-09  6:06       ` Dominik Brodowski
  0 siblings, 0 replies; 5+ messages in thread
From: Dominik Brodowski @ 2022-05-09  6:06 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: linux-kernel, Joe Perches

Am Sun, May 08, 2022 at 07:36:10PM +0200 schrieb Jason A. Donenfeld:
> crng_init represents a state machine, with three states, and various
> rules for transitions. For the longest time, we've been managing these
> with "0", "1", and "2", and expecting people to figure it out. To make
> the code more obvious, replace these with proper enum values
> representing the transition, and then redocument what each of these
> states mean.
> 
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

	Reviewed-by: Dominik Brodowski <linux@dominikbrodowski.net>

Thanks,
	Dominik

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-05-09  6:17 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-08 11:26 [PATCH] random: use symbolic constants for crng_init states Jason A. Donenfeld
2022-05-08 12:57 ` Joe Perches
2022-05-08 17:23   ` Jason A. Donenfeld
2022-05-08 17:36     ` [PATCH v2] " Jason A. Donenfeld
2022-05-09  6:06       ` Dominik Brodowski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.