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