Re: [PATCH v1 1/1] s390/arch_random: Buffer true random data

[Holger Dengler <dengler@linux.ibm.com>]

Hi Jason,

On 05/07/2022 17:11, Jason A. Donenfeld wrote:
> Hi Holger,
> 
> On Tue, Jul 05, 2022 at 04:58:30PM +0200, Holger Dengler wrote:
>> It is true, that the performance of the instruction is not really
>> relevant, but only for calls outside of an interrupt context. I did
>> some ftrace logging for the s390_random_get_seed_long() calls, and -
>> as you said - there are a few calls per minute. But there was also
>> some repeating calls in interrupt context. On systems with a huge
>> interrupt load, this can cause severe performance impacts. I've no
> 
> It'd be interesting to know more about this. The way you get
> arch_random_get_seed_long() from irq context is:
> 
> get_random_{bytes,int,long,u32,u64}()
>   crng_make_state()
>     crng_reseed() <-- Rarely
>       extract_entropy()
>         arch_get_random_seed_long()
> 
> So if an irq user of get_random_xx() is the unlucky one in the minute
> span who has to call crng_reseed() then, yea, that'll happen. But I
> wonder about this luck aspect. What scenarios are you seeing where this
> happens all the time? Which driver is using random bytes *so* commonly
> from irq context? Not that, per say, there's anything wrong with that,
> but it could be eyebrow raising, and might point to de facto solutions
> that mostly take care of this.

I saw a few calls in interrupt context during my tracing, but I didn't look to see which ones they were. Let me figure that out in the next few days and provide more information on that.

> One such direction might be making a driver that does such a thing do it
> a little bit less, somehow. Another direction would be preferring
> non-irqs to handle crng_reseed(), but not disallowing irqs entirely,
> with a patch something like the one below. Or maybe there are other
> ideas.

Reduce the number of trng in interrupt context is a possibility, but - in my opinion - only one single trng instruction call in interrupt context in one too much.

For the moment, I would propose to drop the buffering but also return false, if arch_random_get_seed_long() is called in interrupt context.

diff --git a/arch/s390/include/asm/archrandom.h b/arch/s390/include/asm/archrandom.h
index 2c6e1c6ecbe7..711357bdc464 100644
--- a/arch/s390/include/asm/archrandom.h
+++ b/arch/s390/include/asm/archrandom.h
@@ -32,7 +32,8 @@ static inline bool __must_check arch_get_random_int(unsigned int *v)

 static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
 {
-       if (static_branch_likely(&s390_arch_random_available)) {
+       if (static_branch_likely(&s390_arch_random_available) &&
+           !in_interrupt()) {
                cpacf_trng(NULL, 0, (u8 *)v, sizeof(*v));
                atomic64_add(sizeof(*v), &s390_arch_random_counter);
                return true;

(on-top of your commit, without our buffering patch)

> 
> But all this is to say that having some more of the "mundane" details
> about this might actually help us.
> 
> Jason
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index e3dd1dd3dd22..81df8cdf2a62 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -270,6 +270,9 @@ static bool crng_has_old_seed(void)
>  	static bool early_boot = true;
>  	unsigned long interval = CRNG_RESEED_INTERVAL;
> 
> +	if (in_hardirq())
> +		interval += HZ * 10;
> +
>  	if (unlikely(READ_ONCE(early_boot))) {
>  		time64_t uptime = ktime_get_seconds();
>  		if (uptime >= CRNG_RESEED_INTERVAL / HZ * 2)
> 

-- 
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@linux.ibm.com