linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC v1] random: use static branch for crng_ready()
@ 2022-05-03 13:40 Jason A. Donenfeld
  2022-05-11 11:41 ` Jason A. Donenfeld
  0 siblings, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-05-03 13:40 UTC (permalink / raw)
  To: linux-kernel, linux-crypto
  Cc: Jason A. Donenfeld, Theodore Ts'o, Dominik Brodowski,
	Steven Rostedt, Ard Biesheuvel

Since crng_ready() is only false briefly during initialization and then
forever after becomes true, we don't need to evaluate it after, making
it a prime candidate for a static branch.

One complication, however, is that it changes state in a particular call
to credit_entropy_bits(), which might be made from atomic context. So
rather than changing it then, we keep around the same state variable we
had before, but the next time it's used from non-atomic context, we
change it lazily then.

This is an RFC for now because it seems a bit complicated and fiddly,
and potentially not worth the complexity. I'm interested to hear if
people have opinions about this or if there's a better way to do it.

Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 845f610b6611..977093022430 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -75,12 +75,13 @@
  * crng_init =  0 --> Uninitialized
  *		1 --> Initialized
  *		2 --> Initialized from input_pool
+ *		3 --> Initialized from input_pool and static key set
  *
  * crng_init is protected by base_crng->lock, and only increases
- * its value (from 0->1->2).
+ * its value (from 0->1->2->3).
  */
 static int crng_init = 0;
-#define crng_ready() (likely(crng_init > 1))
+static DEFINE_STATIC_KEY_FALSE(crng_ready_static);
 /* Various types of waiters for crng_init->2 transition. */
 static DECLARE_WAIT_QUEUE_HEAD(crng_init_wait);
 static struct fasync_struct *fasync;
@@ -96,6 +97,17 @@ static int ratelimit_disable __read_mostly;
 module_param_named(ratelimit_disable, ratelimit_disable, int, 0644);
 MODULE_PARM_DESC(ratelimit_disable, "Disable random ratelimit suppression");
 
+static bool crng_ready_slowpath(void)
+{
+	if (crng_init <= 1)
+		return false;
+	if (in_atomic() || irqs_disabled() || cmpxchg(&crng_init, 2, 3) != 2)
+		return true;
+	static_branch_enable(&crng_ready_static);
+	return true;
+}
+#define crng_ready() (static_branch_likely(&crng_ready_static) || unlikely(crng_ready_slowpath()))
+
 /*
  * Returns whether or not the input pool has been seeded and thus guaranteed
  * to supply cryptographically secure random numbers. This applies to: the
-- 
2.35.1


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

* Re: [PATCH RFC v1] random: use static branch for crng_ready()
  2022-05-03 13:40 [PATCH RFC v1] random: use static branch for crng_ready() Jason A. Donenfeld
@ 2022-05-11 11:41 ` Jason A. Donenfeld
  2022-05-12  4:35   ` Herbert Xu
  0 siblings, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-05-11 11:41 UTC (permalink / raw)
  To: linux-kernel, linux-crypto
  Cc: Theodore Ts'o, Dominik Brodowski, Steven Rostedt, Ard Biesheuvel

On Tue, May 03, 2022 at 03:40:52PM +0200, Jason A. Donenfeld wrote:
> +static bool crng_ready_slowpath(void)
> +{
> +	if (crng_init <= 1)
> +		return false;
> +	if (in_atomic() || irqs_disabled() || cmpxchg(&crng_init, 2, 3) != 2)
> +		return true;

Nobody chimed in here, but for posterity I thought I should point out
that this approach actually won't work, since in_atomic() doesn't work
with CONFIG_PREEMPT_COUNT=n kernels.

So back to the drawing board in trying to figure out the best way to do
this...
 
Jason

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

* Re: [PATCH RFC v1] random: use static branch for crng_ready()
  2022-05-11 11:41 ` Jason A. Donenfeld
@ 2022-05-12  4:35   ` Herbert Xu
  2022-05-12 11:18     ` Jason A. Donenfeld
  0 siblings, 1 reply; 8+ messages in thread
From: Herbert Xu @ 2022-05-12  4:35 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-kernel, linux-crypto, tytso, linux, rostedt, ardb

Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Tue, May 03, 2022 at 03:40:52PM +0200, Jason A. Donenfeld wrote:
>> +static bool crng_ready_slowpath(void)
>> +{
>> +     if (crng_init <= 1)
>> +             return false;
>> +     if (in_atomic() || irqs_disabled() || cmpxchg(&crng_init, 2, 3) != 2)
>> +             return true;
> 
> Nobody chimed in here, but for posterity I thought I should point out
> that this approach actually won't work, since in_atomic() doesn't work
> with CONFIG_PREEMPT_COUNT=n kernels.
> 
> So back to the drawing board in trying to figure out the best way to do
> this...

Well the standard solution to code paths that require sleeping is
to use a work queue.  So any reason why you can't just schedule
a work to do the static_branch_enable?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

* Re: [PATCH RFC v1] random: use static branch for crng_ready()
  2022-05-12  4:35   ` Herbert Xu
@ 2022-05-12 11:18     ` Jason A. Donenfeld
  2022-05-12 12:22       ` [PATCH v2] " Jason A. Donenfeld
  0 siblings, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-05-12 11:18 UTC (permalink / raw)
  To: Herbert Xu; +Cc: linux-kernel, linux-crypto, tytso, linux, rostedt, ardb

Hi Herbert,

On Thu, May 12, 2022 at 12:35:51PM +0800, Herbert Xu wrote:
> Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > On Tue, May 03, 2022 at 03:40:52PM +0200, Jason A. Donenfeld wrote:
> >> +static bool crng_ready_slowpath(void)
> >> +{
> >> +     if (crng_init <= 1)
> >> +             return false;
> >> +     if (in_atomic() || irqs_disabled() || cmpxchg(&crng_init, 2, 3) != 2)
> >> +             return true;
> > 
> > Nobody chimed in here, but for posterity I thought I should point out
> > that this approach actually won't work, since in_atomic() doesn't work
> > with CONFIG_PREEMPT_COUNT=n kernels.
> > 
> > So back to the drawing board in trying to figure out the best way to do
> > this...
> 
> Well the standard solution to code paths that require sleeping is
> to use a work queue.  So any reason why you can't just schedule
> a work to do the static_branch_enable?

Yea, that would work but becomes kind of messier than most cases,
because the transition from not-init'd to init'd can happen before
workqueues are even initialized, which means some logic is necessary for
detecting this and then deferring that to happen later on in
initialization. I thought this was pretty "meh", hence looking for other
solutions. But maybe you're right and it's the best we can do here.

For the sake of discussion, below is what the code for that would
approximately look like, modulo whatever details I'm missing. I guess
it's not _that_ bad, if I just rely on the obscure property that
rand_initialize() is called before IRQs are enabled system-wide but
after system_wq has been initialized, but I just hate having the
deferred state be implicit like that. If we go that route, it'll need
comments I suppose.

Jason

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 8fa7a5b9aa93..809ee3c1cf78 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -79,7 +79,8 @@ static enum {
 	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))
+static DEFINE_STATIC_KEY_FALSE(crng_is_ready);
+#define crng_ready() (static_branch_likely(&crng_is_ready) || unlikely(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;
@@ -109,6 +110,12 @@ bool rng_is_initialized(void)
 }
 EXPORT_SYMBOL(rng_is_initialized);

+static void crng_set_ready(struct work_struct *work)
+{
+	static_branch_enable(&crng_is_ready);
+}
+static DECLARE_WORK(crng_set_ready_work, crng_set_ready);
+
 /* Used by wait_for_random_bytes(), and considered an entropy collector, below. */
 static void try_to_generate_entropy(void);

@@ -268,6 +275,8 @@ static void crng_reseed(void)
 		++next_gen;
 	WRITE_ONCE(base_crng.generation, next_gen);
 	WRITE_ONCE(base_crng.birth, jiffies);
+	if (!crng_ready() && system_wq)
+		schedule_work(&crng_set_ready_work);
 	crng_init = CRNG_READY;
 	spin_unlock_irqrestore(&base_crng.lock, flags);
 	memzero_explicit(key, sizeof(key));
@@ -945,9 +954,18 @@ int __init rand_initialize(void)
 	_mix_pool_bytes(&now, sizeof(now));
 	_mix_pool_bytes(utsname(), sizeof(*(utsname())));

-	if (crng_ready())
+	if (crng_ready()) {
+		/*
+		 * If rand_initialize() is called with the crng already
+		 * initialized, then it means it was done so prior to
+		 * system_wq being available, which means we should now
+		 * schedule the work to change the static key.
+		 */
+		schedule_work(&crng_set_ready_work);
+
+		/* Immediately use the above architectural contributions. */
 		crng_reseed();
-	else if (arch_init && trust_cpu)
+	} else if (arch_init && trust_cpu)
 		credit_init_bits(BLAKE2S_BLOCK_SIZE * 8);

 	WARN_ON(register_pm_notifier(&pm_notifier));

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

* [PATCH v2] random: use static branch for crng_ready()
  2022-05-12 11:18     ` Jason A. Donenfeld
@ 2022-05-12 12:22       ` Jason A. Donenfeld
  2022-05-12 14:42         ` [PATCH v3] " Jason A. Donenfeld
  0 siblings, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-05-12 12:22 UTC (permalink / raw)
  To: linux-kernel, linux-crypto
  Cc: herbert, Jason A. Donenfeld, Theodore Ts'o, Dominik Brodowski

Since crng_ready() is only false briefly during initialization and then
forever after becomes true, we don't need to evaluate it after, making
it a prime candidate for a static branch.

One complication, however, is that it changes state in a particular call
to credit_init_bits(), which might be made from atomic context, which
means we must kick off a workqueue to change the static key.

Further complicating things, credit_init_bits() may be called
sufficiently early on in system initialization such that system_wq is
NULL. In that case, we skip scheduling the work item from
credit_init_bits(), but check to see if the rng is already initialized
by the time it hits rand_initialize(), in which case we assume that
there was no crediting event between system_wq becoming non-NULL and
rand_initialize() being called, so we schedule the work then.

We make that assumption because this is still being called in an early
boot environement in which IRQs are disabled. To understand more
clearly, the two flows are as follows.

First possibility, in which credit_init_bits() is called relatively late
in boot:

  - workqueue_init_early()
  - ...
  - rand_initialize()
    * crng_ready()==false → do nothing
  - ...
  - credit_init_bits(256)
    * system_wq!=NULL → schedule_work()

Second possibility, in which credit_init_bits() is called by the super
early EFI-handling code:

  - credit_init_bits(256)
    * system_wq==NULL → do nothing
  - workqueue_init_early()
  - rcu_init(), trace_init(), initcall_debug_enable(),
    context_tracking_init(), early_irq_init(), init_IRQ(), tick_init(),
    rcu_init_nohz(), init_timers(), srcu_init(), hrtimers_init(),
    softirq_init(), timekeeping_init(), kfence_init(), time_init()
    * We assume that none of these call credit_init_bits(). Since IRQs
      are still off, we don't have to worry about other code preempting
      these.
  - rand_initialize()
    * crng_ready()==true → schedule_work()

Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Dominik - this is somewhat reminiscent of the misery we went through
with the NUMA node workqueue situation, which I was pretty happy about
removing. Hopefully recent other simplifications make adding something
similar back in not quite as bad, but if you've got the bandwidth, this
could use another pair of eyes. -Jason

 drivers/char/random.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 8fa7a5b9aa93..d4bc9beaed2c 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -79,7 +79,8 @@ static enum {
 	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))
+static DEFINE_STATIC_KEY_FALSE(crng_is_ready);
+#define crng_ready() (static_branch_likely(&crng_is_ready) || 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;
@@ -109,6 +110,12 @@ bool rng_is_initialized(void)
 }
 EXPORT_SYMBOL(rng_is_initialized);
 
+static void crng_set_ready(struct work_struct *work)
+{
+	static_branch_enable(&crng_is_ready);
+}
+static DECLARE_WORK(crng_set_ready_work, crng_set_ready);
+
 /* Used by wait_for_random_bytes(), and considered an entropy collector, below. */
 static void try_to_generate_entropy(void);
 
@@ -268,6 +275,8 @@ static void crng_reseed(void)
 		++next_gen;
 	WRITE_ONCE(base_crng.generation, next_gen);
 	WRITE_ONCE(base_crng.birth, jiffies);
+	if (!crng_ready() && system_wq)
+		schedule_work(&crng_set_ready_work);
 	crng_init = CRNG_READY;
 	spin_unlock_irqrestore(&base_crng.lock, flags);
 	memzero_explicit(key, sizeof(key));
@@ -945,9 +954,18 @@ int __init rand_initialize(void)
 	_mix_pool_bytes(&now, sizeof(now));
 	_mix_pool_bytes(utsname(), sizeof(*(utsname())));
 
-	if (crng_ready())
+	if (crng_ready()) {
+		/*
+		 * If this function is called with the crng already
+		 * initialized, then it means it was done so prior to
+		 * system_wq being available, which means we should now
+		 * schedule the work to change the static key from here.
+		 */
+		schedule_work(&crng_set_ready_work);
+
+		/* Immediately use the above architectural contributions. */
 		crng_reseed();
-	else if (arch_init && trust_cpu)
+	} else if (arch_init && trust_cpu)
 		credit_init_bits(BLAKE2S_BLOCK_SIZE * 8);
 
 	WARN_ON(register_pm_notifier(&pm_notifier));
-- 
2.35.1


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

* [PATCH v3] random: use static branch for crng_ready()
  2022-05-12 12:22       ` [PATCH v2] " Jason A. Donenfeld
@ 2022-05-12 14:42         ` Jason A. Donenfeld
  2022-05-12 16:37           ` [PATCH v4] " Jason A. Donenfeld
  0 siblings, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-05-12 14:42 UTC (permalink / raw)
  To: linux-kernel, linux-crypto
  Cc: Jason A. Donenfeld, Theodore Ts'o, Dominik Brodowski, Sultan Alsawaf

Since crng_ready() is only false briefly during initialization and then
forever after becomes true, we don't need to evaluate it after, making
it a prime candidate for a static branch.

One complication, however, is that it changes state in a particular call
to credit_init_bits(), which might be made from atomic context, which
means we must kick off a workqueue to change the static key.

Further complicating things, credit_init_bits() may be called
sufficiently early on in system initialization such that system_wq is
NULL. In that case, we skip scheduling the work item from
credit_init_bits(), but check to see if the rng is already initialized
by the time it hits random_init(), in which case we assume that there
was no crediting event between system_wq becoming non-NULL and
random_init() being called, so we schedule the work then.

We make that assumption because this is still being called in an early
boot environement in which IRQs are disabled. To understand more
clearly, the two flows are as follows.

First possibility, in which credit_init_bits() is called relatively late
in boot:

  - workqueue_init_early()
  - ...
  - random_init()
    * crng_ready()==false → do nothing
  - ...
  - credit_init_bits(256)
    * system_wq!=NULL → schedule_work()

Second possibility, in which credit_init_bits() is called by the super
early EFI-handling code:

  - credit_init_bits(256)
    * system_wq==NULL → do nothing
  - workqueue_init_early()
  - rcu_init(), trace_init(), initcall_debug_enable(),
    context_tracking_init(), early_irq_init(), init_IRQ(), tick_init(),
    rcu_init_nohz(), init_timers(), srcu_init(), hrtimers_init(),
    softirq_init(), timekeeping_init(), kfence_init(), time_init()
    * We assume that none of these call credit_init_bits(). Since IRQs
      are still off, we don't have to worry about other code preempting
      these.
  - random_init()
    * crng_ready()==true → schedule_work()

Finally, schedule_work() must be called outside of the base_crng.lock,
because it can call into get_random_u32() itself, which will in some
cases attempt to acquire that same lock. So we hoist it outside of the
locked region, and ensure that the branches for that complexity get
removed as well.

Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Sultan Alsawaf <sultan@kerneltoast.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Dominik - this is somewhat reminiscent of the misery we went through
with the NUMA node workqueue situation, which I was pretty happy about
removing. Hopefully recent other simplifications make adding something
similar back in not quite as bad, but if you've got the bandwidth, this
could use another pair of eyes. -Jason

Changes v2->v3:
- Call schedule_work() outside of the lock.

Changes v1->v2:
- Use a workqueue instead of doing it on-demand.

 drivers/char/random.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 2f5460edba28..ecf017ac5c3d 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -79,7 +79,8 @@ static enum {
 	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))
+static DEFINE_STATIC_KEY_FALSE(crng_is_ready);
+#define crng_ready() (static_branch_likely(&crng_is_ready) || 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;
@@ -109,6 +110,12 @@ bool rng_is_initialized(void)
 }
 EXPORT_SYMBOL(rng_is_initialized);
 
+static void crng_set_ready(struct work_struct *work)
+{
+	static_branch_enable(&crng_is_ready);
+}
+static DECLARE_WORK(crng_set_ready_work, crng_set_ready);
+
 /* Used by wait_for_random_bytes(), and considered an entropy collector, below. */
 static void try_to_generate_entropy(void);
 
@@ -252,6 +259,7 @@ static void crng_reseed(void)
 	unsigned long flags;
 	unsigned long next_gen;
 	u8 key[CHACHA_KEY_SIZE];
+	bool was_ready;
 
 	extract_entropy(key, sizeof(key));
 
@@ -268,9 +276,12 @@ static void crng_reseed(void)
 		++next_gen;
 	WRITE_ONCE(base_crng.generation, next_gen);
 	WRITE_ONCE(base_crng.birth, jiffies);
+	was_ready = crng_ready();
 	crng_init = CRNG_READY;
 	spin_unlock_irqrestore(&base_crng.lock, flags);
 	memzero_explicit(key, sizeof(key));
+	if (!static_branch_likely(&crng_is_ready) && !was_ready && system_wq)
+		schedule_work(&crng_set_ready_work);
 }
 
 /*
@@ -948,9 +959,18 @@ int __init random_init(const char *command_line)
 	_mix_pool_bytes(command_line, strlen(command_line));
 	add_latent_entropy();
 
-	if (crng_ready())
+	if (crng_ready()) {
+		/*
+		 * If this function is called with the crng already
+		 * initialized, then it means it was done so prior to
+		 * system_wq being available, which means we should now
+		 * schedule the work to change the static key from here.
+		 */
+		schedule_work(&crng_set_ready_work);
+
+		/* Immediately use the above architectural contributions. */
 		crng_reseed();
-	else if (trust_cpu)
+	} else if (trust_cpu)
 		credit_init_bits(arch_init * 8);
 
 	WARN_ON(register_pm_notifier(&pm_notifier));
-- 
2.35.1


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

* [PATCH v4] random: use static branch for crng_ready()
  2022-05-12 14:42         ` [PATCH v3] " Jason A. Donenfeld
@ 2022-05-12 16:37           ` Jason A. Donenfeld
  2022-05-13  6:22             ` Dominik Brodowski
  0 siblings, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2022-05-12 16:37 UTC (permalink / raw)
  To: linux-kernel, linux-crypto
  Cc: Jason A. Donenfeld, Theodore Ts'o, Dominik Brodowski, Sultan Alsawaf

Since crng_ready() is only false briefly during initialization and then
forever after becomes true, we don't need to evaluate it after, making
it a prime candidate for a static branch.

One complication, however, is that it changes state in a particular call
to credit_init_bits(), which might be made from atomic context, which
means we must kick off a workqueue to change the static key. Further
complicating things, credit_init_bits() may be called sufficiently early
on in system initialization such that system_wq is NULL.

Fortunately, there exists the nice function execute_in_process_context(),
which will immediately execute the function if !in_interrupt(), and
otherwise defer it to a workqueue. During early init, before workqueues
are available, in_interrupt() is always false, because interrupts
haven't even been enabled yet, which means the function in that case
executes immediately. Later on, after workqueues are available,
in_interrupt() might be true, but in that case, the work is queued in
system_wq and all goes well.

Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Sultan Alsawaf <sultan@kerneltoast.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Sorry for all the churn. execute_in_process_context() appeared out of
the blue and so clearly represents the better option, so happily going
with that now, making this patch finally tiny.

Changes v3->v4:
- Use execute_in_process_context() to resolve quandries.

Changes v2->v3:
- Call schedule_work() outside of the lock.

Changes v1->v2:
- Use a workqueue instead of doing it on-demand.

 drivers/char/random.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 2f5460edba28..ec4e8c2657fb 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -78,8 +78,9 @@ 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))
+} crng_init __read_mostly = CRNG_EMPTY;
+static DEFINE_STATIC_KEY_FALSE(crng_is_ready);
+#define crng_ready() (static_branch_likely(&crng_is_ready) || 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;
@@ -109,6 +110,11 @@ bool rng_is_initialized(void)
 }
 EXPORT_SYMBOL(rng_is_initialized);
 
+static void crng_set_ready(struct work_struct *work)
+{
+	static_branch_enable(&crng_is_ready);
+}
+
 /* Used by wait_for_random_bytes(), and considered an entropy collector, below. */
 static void try_to_generate_entropy(void);
 
@@ -268,7 +274,8 @@ static void crng_reseed(void)
 		++next_gen;
 	WRITE_ONCE(base_crng.generation, next_gen);
 	WRITE_ONCE(base_crng.birth, jiffies);
-	crng_init = CRNG_READY;
+	if (!static_branch_likely(&crng_is_ready))
+		crng_init = CRNG_READY;
 	spin_unlock_irqrestore(&base_crng.lock, flags);
 	memzero_explicit(key, sizeof(key));
 }
@@ -785,6 +792,7 @@ static void extract_entropy(void *buf, size_t nbytes)
 
 static void credit_init_bits(size_t nbits)
 {
+	static struct execute_work set_ready;
 	unsigned int new, orig, add;
 	unsigned long flags;
 
@@ -800,6 +808,7 @@ static void credit_init_bits(size_t nbits)
 
 	if (orig < POOL_READY_BITS && new >= POOL_READY_BITS) {
 		crng_reseed(); /* Sets crng_init to CRNG_READY under base_crng.lock. */
+		execute_in_process_context(crng_set_ready, &set_ready);
 		process_random_ready_list();
 		wake_up_interruptible(&crng_init_wait);
 		kill_fasync(&fasync, SIGIO, POLL_IN);
-- 
2.35.1


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

* Re: [PATCH v4] random: use static branch for crng_ready()
  2022-05-12 16:37           ` [PATCH v4] " Jason A. Donenfeld
@ 2022-05-13  6:22             ` Dominik Brodowski
  0 siblings, 0 replies; 8+ messages in thread
From: Dominik Brodowski @ 2022-05-13  6:22 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-kernel, linux-crypto, Theodore Ts'o, Sultan Alsawaf

Am Thu, May 12, 2022 at 06:37:48PM +0200 schrieb Jason A. Donenfeld:
> Since crng_ready() is only false briefly during initialization and then
> forever after becomes true, we don't need to evaluate it after, making
> it a prime candidate for a static branch.
> 
> One complication, however, is that it changes state in a particular call
> to credit_init_bits(), which might be made from atomic context, which
> means we must kick off a workqueue to change the static key. Further
> complicating things, credit_init_bits() may be called sufficiently early
> on in system initialization such that system_wq is NULL.
> 
> Fortunately, there exists the nice function execute_in_process_context(),
> which will immediately execute the function if !in_interrupt(), and
> otherwise defer it to a workqueue. During early init, before workqueues
> are available, in_interrupt() is always false, because interrupts
> haven't even been enabled yet, which means the function in that case
> executes immediately. Later on, after workqueues are available,
> in_interrupt() might be true, but in that case, the work is queued in
> system_wq and all goes well.

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

Thanks,
	Dominik

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

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

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-03 13:40 [PATCH RFC v1] random: use static branch for crng_ready() Jason A. Donenfeld
2022-05-11 11:41 ` Jason A. Donenfeld
2022-05-12  4:35   ` Herbert Xu
2022-05-12 11:18     ` Jason A. Donenfeld
2022-05-12 12:22       ` [PATCH v2] " Jason A. Donenfeld
2022-05-12 14:42         ` [PATCH v3] " Jason A. Donenfeld
2022-05-12 16:37           ` [PATCH v4] " Jason A. Donenfeld
2022-05-13  6:22             ` Dominik Brodowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).