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@xxxxxxxxx/
- 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@xxxxxxxxxxxxx/
, 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
