[PATCH] random: Initialize vsprintf's pointer hash once the random core is ready.

[PATCH] random: Initialize vsprintf's pointer hash once the random core is ready.

[Sebastian Andrzej Siewior]

The printk code invokes vnsprintf in order to compute the complete
string before adding it into its buffer. This happens in an IRQ-off
region which leads to a warning on PREEMPT_RT in the random code if the
format strings contains a %p for pointer printing. This happens because
the random core acquires locks which become sleeping locks on PREEMPT_RT
which must not be acquired with disabled interrupts and or preemption.
By default the pointers are hashed which requires a random value on the
first invocation (either by printk or another user which comes first.

One could argue that there is no need for printk to disable interrupts
during the vsprintf() invocation which would fix the just mentioned
problem. However printk itself can be invoked in a context with
disabled interrupts which would lead to the very same problem.

This late init via printk can be avoided by explicitly initializing
vsprintf's random value once the random-core has been initialized.

Remove the on demand init from __ptr_to_hashval() and keep the -EAGAIN if
the init has not yet been performed. Move the actual init bits to
vsprintf_init_hash_pointer() which are invoked from random-core once it
has been initialized and get_random_bytes() is available.

Reported-by: Mike Galbraith <efault@gmx.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/char/random.c  |  8 +++++++-
 include/linux/random.h |  2 ++
 lib/vsprintf.c         | 36 +++++++++++++++---------------------
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index a1af90bacc9f8..98f99026d1fba 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -202,6 +202,7 @@ static void extract_entropy(void *buf, size_t len);
 /* This extracts a new crng key from the input pool. */
 static void crng_reseed(void)
 {
+	bool init_hash_pointer = false;
 	unsigned long flags;
 	unsigned long next_gen;
 	u8 key[CHACHA_KEY_SIZE];
@@ -221,10 +222,15 @@ static void crng_reseed(void)
 		++next_gen;
 	WRITE_ONCE(base_crng.generation, next_gen);
 	WRITE_ONCE(base_crng.birth, jiffies);
-	if (!static_branch_likely(&crng_is_ready))
+	if (!static_branch_likely(&crng_is_ready)) {
 		crng_init = CRNG_READY;
+		init_hash_pointer = true;
+	}
 	spin_unlock_irqrestore(&base_crng.lock, flags);
 	memzero_explicit(key, sizeof(key));
+
+	if (init_hash_pointer)
+		vsprintf_init_hash_pointer();
 }
 
 /*
diff --git a/include/linux/random.h b/include/linux/random.h
index 20e389a14e5c7..229743ba5b4de 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -144,4 +144,6 @@ int random_online_cpu(unsigned int cpu);
 extern const struct file_operations random_fops, urandom_fops;
 #endif
 
+void vsprintf_init_hash_pointer(void);
+
 #endif /* _LINUX_RANDOM_H */
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3c1853a9d1c09..6fa2ebb9f9b9e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -751,36 +751,30 @@ static int __init debug_boot_weak_hash_enable(char *str)
 early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
 
 static DEFINE_STATIC_KEY_FALSE(filled_random_ptr_key);
+static siphash_key_t ptr_key __read_mostly;
 
-static void enable_ptr_key_workfn(struct work_struct *work)
+void vsprintf_init_hash_pointer(void)
 {
-	static_branch_enable(&filled_random_ptr_key);
+	static DEFINE_SPINLOCK(filling);
+	unsigned long flags;
+	static bool filled;
+
+	spin_lock_irqsave(&filling, flags);
+	if (!filled) {
+		get_random_bytes(&ptr_key, sizeof(ptr_key));
+		filled = true;
+		static_branch_enable(&filled_random_ptr_key);
+	}
+	spin_unlock_irqrestore(&filling, flags);
 }
 
 /* Maps a pointer to a 32 bit unique identifier. */
 static inline int __ptr_to_hashval(const void *ptr, unsigned long *hashval_out)
 {
-	static siphash_key_t ptr_key __read_mostly;
 	unsigned long hashval;
 
-	if (!static_branch_likely(&filled_random_ptr_key)) {
-		static bool filled = false;
-		static DEFINE_SPINLOCK(filling);
-		static DECLARE_WORK(enable_ptr_key_work, enable_ptr_key_workfn);
-		unsigned long flags;
-
-		if (!system_unbound_wq || !rng_is_initialized() ||
-		    !spin_trylock_irqsave(&filling, flags))
-			return -EAGAIN;
-
-		if (!filled) {
-			get_random_bytes(&ptr_key, sizeof(ptr_key));
-			queue_work(system_unbound_wq, &enable_ptr_key_work);
-			filled = true;
-		}
-		spin_unlock_irqrestore(&filling, flags);
-	}
-
+	if (!static_branch_likely(&filled_random_ptr_key))
+		return -EAGAIN;
 
 #ifdef CONFIG_64BIT
 	hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key);
-- 
2.36.1

Re: [PATCH] random: Initialize vsprintf's pointer hash once the random core is ready.

[Jason A. Donenfeld]

Hi Sebastian,

On Fri, Jul 29, 2022 at 10:52:58AM +0200, Sebastian Andrzej Siewior wrote:
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -202,6 +202,7 @@ static void extract_entropy(void *buf, size_t len);
>  /* This extracts a new crng key from the input pool. */
>  static void crng_reseed(void)
>  {
> +	bool init_hash_pointer = false;
>  	unsigned long flags;
>  	unsigned long next_gen;
>  	u8 key[CHACHA_KEY_SIZE];
> @@ -221,10 +222,15 @@ static void crng_reseed(void)
>  		++next_gen;
>  	WRITE_ONCE(base_crng.generation, next_gen);
>  	WRITE_ONCE(base_crng.birth, jiffies);
> -	if (!static_branch_likely(&crng_is_ready))
> +	if (!static_branch_likely(&crng_is_ready)) {
>  		crng_init = CRNG_READY;
> +		init_hash_pointer = true;
> +	}
>  	spin_unlock_irqrestore(&base_crng.lock, flags);
>  	memzero_explicit(key, sizeof(key));
> +
> +	if (init_hash_pointer)
> +		vsprintf_init_hash_pointer();
>  }

Gumming up random.c with these sorts of things isn't alright. vsprintf
isn't special in any regard here.

If you can't do this from ordinary context inside of vsprintf, just
launch a workqueue to do it. This is already needed for changing
vsprintf's static branch, so just move the get_random_bytes() call into
there on RT (leaving it alone on non-RT, I guess).

Jason

Re: [PATCH] random: Initialize vsprintf's pointer hash once the random core is ready.

[Sebastian Andrzej Siewior]

On 2022-07-29 12:12:08 [+0200], Jason A. Donenfeld wrote:
> Hi Sebastian,
Hi Jason,

> Gumming up random.c with these sorts of things isn't alright. vsprintf
> isn't special in any regard here.
> 
> If you can't do this from ordinary context inside of vsprintf, just
> launch a workqueue to do it. This is already needed for changing
> vsprintf's static branch, so just move the get_random_bytes() call into
> there on RT (leaving it alone on non-RT, I guess).

So launching a worker to obtain the random data? That would mean that
the first %p print won't have nothing, right? I could do it as part of
an init-call but I don't know when the random pool is ready. And trying
it again every other second if the random core isn't ready looks kind of
wasteful.

> Jason

Sebastian

Re: [PATCH] random: Initialize vsprintf's pointer hash once the random core is ready.

[Jason A. Donenfeld]

Hi Sebastian,

On Fri, Jul 29, 2022 at 12:21:27PM +0200, Sebastian Andrzej Siewior wrote:
> So launching a worker to obtain the random data? That would mean that
> the first %p print won't have nothing, right? I could do it as part of

"First" isn't very meaningful here. If the rng isn't initialized by
add_bootloader_randomness() or similar, then it'll almost miss some
amount of %p anyway.

But anyway, it sounds like you only need to hoist into a worker IF
you're `IS_ENABLED(CONFIG_PREEMPT_RT) && in_hardirq()`, right? So just
conditionalize it on that, and this should have pretty minimal impact.

I don't think this patch will require touching random.c.

Jason

Re: [PATCH] random: Initialize vsprintf's pointer hash once the random core is ready.

[Sebastian Andrzej Siewior]

On 2022-07-29 12:38:06 [+0200], Jason A. Donenfeld wrote:
> Hi Sebastian,
Hi Jason,

> On Fri, Jul 29, 2022 at 12:21:27PM +0200, Sebastian Andrzej Siewior wrote:
> > So launching a worker to obtain the random data? That would mean that
> > the first %p print won't have nothing, right? I could do it as part of
> 
> "First" isn't very meaningful here. If the rng isn't initialized by
> add_bootloader_randomness() or similar, then it'll almost miss some
> amount of %p anyway.

only if that printk happens during boot. But it could happen much later.
In that case !RT won't lose that pointer but RT will.

> But anyway, it sounds like you only need to hoist into a worker IF
> you're `IS_ENABLED(CONFIG_PREEMPT_RT) && in_hardirq()`, right? So just
> conditionalize it on that, and this should have pretty minimal impact.

I need always to hoist into a worker because there could warning in a
preempt-off region leading to this error.
Maybe I am putting too much importance into this. Let me do what you
suggest and always lose that first pointer and if someone complains than
maybe we think about something else…

> I don't think this patch will require touching random.c.
> 
> Jason

Sebastian

Re: [PATCH] random: Initialize vsprintf's pointer hash once the random core is ready.

[Petr Mladek]

On Fri 2022-07-29 10:52:58, Sebastian Andrzej Siewior wrote:
> The printk code invokes vnsprintf in order to compute the complete
> string before adding it into its buffer. This happens in an IRQ-off
> region which leads to a warning on PREEMPT_RT in the random code if the
> format strings contains a %p for pointer printing. This happens because
> the random core acquires locks which become sleeping locks on PREEMPT_RT
> which must not be acquired with disabled interrupts and or preemption.
> By default the pointers are hashed which requires a random value on the
> first invocation (either by printk or another user which comes first.
> 
> One could argue that there is no need for printk to disable interrupts
> during the vsprintf() invocation which would fix the just mentioned
> problem. However printk itself can be invoked in a context with
> disabled interrupts which would lead to the very same problem.
> 
> This late init via printk can be avoided by explicitly initializing
> vsprintf's random value once the random-core has been initialized.
> 
> Remove the on demand init from __ptr_to_hashval() and keep the -EAGAIN if
> the init has not yet been performed. Move the actual init bits to
> vsprintf_init_hash_pointer() which are invoked from random-core once it
> has been initialized and get_random_bytes() is available.

> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -221,10 +222,15 @@ static void crng_reseed(void)
>  		++next_gen;
>  	WRITE_ONCE(base_crng.generation, next_gen);
>  	WRITE_ONCE(base_crng.birth, jiffies);
> -	if (!static_branch_likely(&crng_is_ready))
> +	if (!static_branch_likely(&crng_is_ready)) {
>  		crng_init = CRNG_READY;
> +		init_hash_pointer = true;

I am not familiar with the crng code. I wonder if the following would work:

	if (!static_branch_likely(&crng_is_ready) && crng_init != CRNG_READY) {
		crng_init = CRNG_READY;
		init_hash_pointer = true;
	}

The point is that vsprintf_init_hash_pointer() will be called only by
the first caller. It would allow to remove the @filling spin lock.

> +	}
>  	spin_unlock_irqrestore(&base_crng.lock, flags);
>  	memzero_explicit(key, sizeof(key));
> +
> +	if (init_hash_pointer)
> +		vsprintf_init_hash_pointer();
>  }
>  
>  /*
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3c1853a9d1c09..6fa2ebb9f9b9e 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -751,36 +751,30 @@ static int __init debug_boot_weak_hash_enable(char *str)
>  early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
>  
>  static DEFINE_STATIC_KEY_FALSE(filled_random_ptr_key);
> +static siphash_key_t ptr_key __read_mostly;
>  
> -static void enable_ptr_key_workfn(struct work_struct *work)
> +void vsprintf_init_hash_pointer(void)
>  {
> -	static_branch_enable(&filled_random_ptr_key);
> +	static DEFINE_SPINLOCK(filling);
> +	unsigned long flags;
> +	static bool filled;
> +
> +	spin_lock_irqsave(&filling, flags);
> +	if (!filled) {
> +		get_random_bytes(&ptr_key, sizeof(ptr_key));
> +		filled = true;
> +		static_branch_enable(&filled_random_ptr_key);

This can't be called in an atomic context. Is crng_reseed() always
called in a non-atomic context?

That said, the static branch is an overkill. vsprintf() is a slow
path. It should be enough to use a simple boolean. It might require
a simple memory barrier to serialize @ptr_key and the new boolean
read&write.

> +	}
> +	spin_unlock_irqrestore(&filling, flags);
>  }

Best Regards,
Petr

Re: [PATCH] random: Initialize vsprintf's pointer hash once the random core is ready.

[Sebastian Andrzej Siewior]

On 2022-07-29 12:22:08 [+0200], Petr Mladek wrote:
> > --- a/drivers/char/random.c
> > +++ b/drivers/char/random.c
> > @@ -221,10 +222,15 @@ static void crng_reseed(void)
> >  		++next_gen;
> >  	WRITE_ONCE(base_crng.generation, next_gen);
> >  	WRITE_ONCE(base_crng.birth, jiffies);
> > -	if (!static_branch_likely(&crng_is_ready))
> > +	if (!static_branch_likely(&crng_is_ready)) {
> >  		crng_init = CRNG_READY;
> > +		init_hash_pointer = true;
> 
> I am not familiar with the crng code. I wonder if the following would work:
> 
> 	if (!static_branch_likely(&crng_is_ready) && crng_init != CRNG_READY) {
> 		crng_init = CRNG_READY;
> 		init_hash_pointer = true;
> 	}
> 
> The point is that vsprintf_init_hash_pointer() will be called only by
> the first caller. It would allow to remove the @filling spin lock.

Not sure about the resulting code gen but crng_is_ready is swapped with
"crng_init == CRNG_READY" and this patch already put init_hash_pointer
in an unlikely path so it might already what you suggest.

> > +	}
> >  	spin_unlock_irqrestore(&base_crng.lock, flags);
> >  	memzero_explicit(key, sizeof(key));
> > +
> > +	if (init_hash_pointer)
> > +		vsprintf_init_hash_pointer();
> >  }
> >  
> >  /*
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 3c1853a9d1c09..6fa2ebb9f9b9e 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -751,36 +751,30 @@ static int __init debug_boot_weak_hash_enable(char *str)
> >  early_param("debug_boot_weak_hash", debug_boot_weak_hash_enable);
> >  
> >  static DEFINE_STATIC_KEY_FALSE(filled_random_ptr_key);
> > +static siphash_key_t ptr_key __read_mostly;
> >  
> > -static void enable_ptr_key_workfn(struct work_struct *work)
> > +void vsprintf_init_hash_pointer(void)
> >  {
> > -	static_branch_enable(&filled_random_ptr_key);
> > +	static DEFINE_SPINLOCK(filling);
> > +	unsigned long flags;
> > +	static bool filled;
> > +
> > +	spin_lock_irqsave(&filling, flags);
> > +	if (!filled) {
> > +		get_random_bytes(&ptr_key, sizeof(ptr_key));
> > +		filled = true;
> > +		static_branch_enable(&filled_random_ptr_key);
> 
> This can't be called in an atomic context. Is crng_reseed() always
> called in a non-atomic context?

Since a "recent" change, get_random_bytes() and friends can't be called
from an explicit IRQ-off/preempt-off region. There was a little bit of
fallout on the RT side with this change including lockdep and something
else I don't recall. So two users that invoked that from an IRQ-off
region on RT which is in general not the rule and the code was altered.

This (vsprintf) popped up recently since there isn't much that uses %p.
And since printk can (and should) be invoked everywhere I would like to
keep that working.

> That said, the static branch is an overkill. vsprintf() is a slow
> path. It should be enough to use a simple boolean. It might require
> a simple memory barrier to serialize @ptr_key and the new boolean
> read&write.

I could do that if it is preferred. It was already there, this isn't
something I just added…

> > +	}
> > +	spin_unlock_irqrestore(&filling, flags);
> >  }
> 
> Best Regards,
> Petr

Sebastian