Messages in this thread | | | Date | Fri, 29 Jul 2022 12:34:46 +0200 | From | Sebastian Andrzej Siewior <> | Subject | Re: [PATCH] random: Initialize vsprintf's pointer hash once the random core is ready. |
| |
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
| |