[MODERATED] Re: Additional sampling fun

On Fri, Feb 28, 2020 at 05:21:40PM +0100, speck for Borislav Petkov wrote:
> On Thu, Feb 20, 2020 at 09:14:20AM +0100, speck for Greg KH wrote:
> > Then we need to stop using RDRAND internally for our "give me a random
> > number api" which has spread to more and more parts of the kernel.
> > 
> > Here's a patch that does so:
> > 	https://lore.kernel.org/lkml/[email protected]/
> > which I'm going to advise get merged now and backported to the stable
> > branches.

Note, the above patch (well the v2 version) is now merged and should
show up in the next -rc1 release.

> So one of our guys - Nicolai Stange - was looking at this
> wrt backporting it to trees and there's another problem in
> add_interrupt_randomness() which could potentially turn out
> to be nasty.
> 
> We asked him to write it up for speck@ (he's not subscribed) so that we
> can discuss it here first. Here is the deal in his own words:
> 
> "In the context of the get_random_long() patch posted at [1], I noticed
> that there's also a RDSEED insn issued from the interrupt path, which
> perhaps might have undesired effects performance-wise.
> 
> More specifically, add_interrupt_randomness() would issue one RDSEED
> either once a second or every 64 interrupts, whichever comes first:
> 
>   void add_interrupt_randomness(int irq, int irq_flags)
>   {
>     fast_mix(fast_pool); /* increments fast_pool->count */
>     ...
>     if ((fast_pool->count < 64) &&
>          !time_after(now, fast_pool->last + HZ))
>       return;
>     ...
>     fast_pool->last = now;
>     if (arch_get_random_seed_long(&seed)) {}
>     ...
>   }
> 
> So while this certainly won't matter much on average, I'm still
> wondering whether or not this RDSEED could potentially cause IRQ
> latency spikes relevant e.g. to -RT and/or under high IRQ load?
> 
> FWIW, the commit introducing the arch_get_random_seed_long() invocation
> to add_interrupt_randomness() was commit 83664a6928a4 ("random: Use
> arch_get_random_seed*() at init time and once a second").
> 
> I can only guess, but I think the motivation for mixing
> arch_get_random_seed_long() from the interrupt path was probably to
> sync that with the IRQ rate. That is, to make sure that the entropy
> mixed from RDSEED doesn't dominate the interrupt entropy source.
> 
> [1] https://lkml.kernel.org/r/[email protected]
> "

Ugh.  I think we need to drag Jason into this as well, but really,
talking about that can be done on the mailing list as there's nothing
wrong with trying to get that slow code out of the irq path today,
right?

thanks,

greg k-h