Re: [PATCH] s390/archrandom: remove CPACF trng invocations in interrupt context

[Harald Freudenberger <freude@linux.ibm.com>]

On 2022-07-12 12:23, Jason A. Donenfeld wrote:
> Hi Harald,
> 
> On Tue, Jul 12, 2022 at 12:08:29PM +0200, Harald Freudenberger wrote:
>> This patch introduces two things:
>> 1) The arch_get_random_seed_int() implementation now always
>>    returns false. There is no user in the whole kernel using
>>    this function.
> 
> Please do not do this. It has nothing to do with the rest of the patch,
> but also this isn't really the right place to decide on that. As we
> discussed last week with the arch_get_random_words{,_seed} branch of 
> the
> conversation - 
> https://lore.kernel.org/all/YsQ%2FvZSkzWPLwIte@zx2c4.com/
> - there are a few things that might be suboptimal about the API. When 
> we
> fix these, I'd prefer for it to be done in some coherent step. What
> you're doing here is just gimping the present API, which preemptively
> rots the entire thing and *forces* us to remove it for all 
> architectures
> since it would become non-dependable. And I don't like having our hands
> be forced here. I'd much rather carefully consider this.
> 
> So please remove this snippet.

Ok, will do.

> 
>> 2) For the arch_get_random_seed_long() make sure the CPACF trng
>>    instruction is never called in any interrupt context.
> 
> I don't object overly loudly to this. However, based on your comment in
> https://lore.kernel.org/all/7e65130c6e66ce7a9f9eb469eb7e64e0@linux.ibm.com/
> , I was under the impression that this wasn't necessary. If you think 
> it
> is, it'd be useful to show some measured latency numbers on actual
> systems. Otherwise it seems like premature optimization? Anyway, if you
> have solid rationale, I'm fine with this as I mentioned in the other
> thread. I'm just a bit confused now on the particulars of the "why" 
> part
> given your earlier comment.
> 
>> This is done by adding an additional condition in_task().
> 
> That doesn't seem right. Instead use `!in_hardirq()`, or perhaps
> `!in_nmi() && !in_hardirq()`? Otherwise you also disallow this when
> serving softirqs, which based on the discussion, I don't think you
> really want to do. Or do you? Without actual latency measurements and a
> real world look at the implications, it's hard to see what we're after.
> 
>> which confirms that the call is in softirq context. So in_task() 
>> covers exactly
>> the cases where we want to have CPACF trng called: not in nmi, not in 
>> hard irq,
>> not in soft irq but in normal task context and during kernel init.
> 
> You've gone through the troubles of confirming experimentally what
> in_task() does, but that doesn't answer *why* it should be disallowed
> variously in each one of these contexts.

I think, I showed this. The only real occurrences remaining for the
arch_get_random_seed_long() call is within softirq context when the
network layer tries to allocate some skb buffers. My personal feeling
about this is that it does not hurt - but I asked our network guys
and their feedback is clear: no way - every delay there may cause
high bandwidth traffic to stumble and this is to be absolutely avoided.
However, they can't give me any measurements.

So yes, the intention is now with checking for in_task() to prevent
the trng call in hard and soft interrupt context. But still I'd like
to meet your condition to provide good random at kernel startup.

> 
> Regards,
> Jason

Regards,
Harald Freudenberger