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.
2) For the arch_get_random_seed_long() make sure the CPACF trng
instruction is never called in any interrupt context. This
is done by adding an additional condition in_task().
Justification for 2):
There are some constrains to satisfy for the invocation of the
arch_get_random_seed_long() function:
- It should provide good random data during kernel initialization.
- It should not be called in interrupt context as the instruction
is relatively heavy weight and may for example make some network
loads cause to timeout and buck.
However, it was not clear what kind of interrupt context is exactly
encountered during kernel init or network traffic eventually calling
arch_get_random_seed_long().
After some days of investigations it is clear that the s390
start_kernel function is not running in any interrupt context and
so the trng is called:
Jul 11 18:33:39 t35lp54 kernel: [<00000001064e90ca>] arch_get_random_seed_long.part.0+0x32/0x70
Jul 11 18:33:39 t35lp54 kernel: [<000000010715f246>] random_init+0xf6/0x238
Jul 11 18:33:39 t35lp54 kernel: [<000000010712545c>] start_kernel+0x4a4/0x628
Jul 11 18:33:39 t35lp54 kernel: [<000000010590402a>] startup_continue+0x2a/0x40
The condition in_task() is true and the CPACF trng provides random data
during kernel startup.
The network traffic however, is more difficult. A typical call stack
looks like this:
Jul 06 17:37:07 t35lp54 kernel: [<000000008b5600fc>] extract_entropy.constprop.0+0x23c/0x240
Jul 06 17:37:07 t35lp54 kernel: [<000000008b560136>] crng_reseed+0x36/0xd8
Jul 06 17:37:07 t35lp54 kernel: [<000000008b5604b8>] crng_make_state+0x78/0x340
Jul 06 17:37:07 t35lp54 kernel: [<000000008b5607e0>] _get_random_bytes+0x60/0xf8
Jul 06 17:37:07 t35lp54 kernel: [<000000008b56108a>] get_random_u32+0xda/0x248
Jul 06 17:37:07 t35lp54 kernel: [<000000008aefe7a8>] kfence_guarded_alloc+0x48/0x4b8
Jul 06 17:37:07 t35lp54 kernel: [<000000008aeff35e>] __kfence_alloc+0x18e/0x1b8
Jul 06 17:37:07 t35lp54 kernel: [<000000008aef7f10>] __kmalloc_node_track_caller+0x368/0x4d8
Jul 06 17:37:07 t35lp54 kernel: [<000000008b611eac>] kmalloc_reserve+0x44/0xa0
Jul 06 17:37:07 t35lp54 kernel: [<000000008b611f98>] __alloc_skb+0x90/0x178
Jul 06 17:37:07 t35lp54 kernel: [<000000008b6120dc>] __napi_alloc_skb+0x5c/0x118
Jul 06 17:37:07 t35lp54 kernel: [<000000008b8f06b4>] qeth_extract_skb+0x13c/0x680
Jul 06 17:37:07 t35lp54 kernel: [<000000008b8f6526>] qeth_poll+0x256/0x3f8
Jul 06 17:37:07 t35lp54 kernel: [<000000008b63d76e>] __napi_poll.constprop.0+0x46/0x2f8
Jul 06 17:37:07 t35lp54 kernel: [<000000008b63dbec>] net_rx_action+0x1cc/0x408
Jul 06 17:37:07 t35lp54 kernel: [<000000008b937302>] __do_softirq+0x132/0x6b0
Jul 06 17:37:07 t35lp54 kernel: [<000000008abf46ce>] __irq_exit_rcu+0x13e/0x170
Jul 06 17:37:07 t35lp54 kernel: [<000000008abf531a>] irq_exit_rcu+0x22/0x50
Jul 06 17:37:07 t35lp54 kernel: [<000000008b922506>] do_io_irq+0xe6/0x198
Jul 06 17:37:07 t35lp54 kernel: [<000000008b935826>] io_int_handler+0xd6/0x110
Jul 06 17:37:07 t35lp54 kernel: [<000000008b9358a6>] psw_idle_exit+0x0/0xa
Jul 06 17:37:07 t35lp54 kernel: ([<000000008ab9c59a>] arch_cpu_idle+0x52/0xe0)
Jul 06 17:37:07 t35lp54 kernel: [<000000008b933cfe>] default_idle_call+0x6e/0xd0
Jul 06 17:37:07 t35lp54 kernel: [<000000008ac59f4e>] do_idle+0xf6/0x1b0
Jul 06 17:37:07 t35lp54 kernel: [<000000008ac5a28e>] cpu_startup_entry+0x36/0x40
Jul 06 17:37:07 t35lp54 kernel: [<000000008abb0d90>] smp_start_secondary+0x148/0x158
Jul 06 17:37:07 t35lp54 kernel: [<000000008b935b9e>] restart_int_handler+0x6e/0x90
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.
Signed-off-by: Harald Freudenberger <[email protected]>
---
arch/s390/include/asm/archrandom.h | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/arch/s390/include/asm/archrandom.h b/arch/s390/include/asm/archrandom.h
index 2c6e1c6ecbe7..9662c9fa7683 100644
--- a/arch/s390/include/asm/archrandom.h
+++ b/arch/s390/include/asm/archrandom.h
@@ -2,7 +2,7 @@
/*
* Kernel interface for the s390 arch_random_* functions
*
- * Copyright IBM Corp. 2017, 2020
+ * Copyright IBM Corp. 2017, 2022
*
* Author: Harald Freudenberger <[email protected]>
*
@@ -14,6 +14,7 @@
#ifdef CONFIG_ARCH_RANDOM
#include <linux/static_key.h>
+#include <linux/preempt.h>
#include <linux/atomic.h>
#include <asm/cpacf.h>
@@ -33,20 +34,17 @@ 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)) {
- cpacf_trng(NULL, 0, (u8 *)v, sizeof(*v));
- atomic64_add(sizeof(*v), &s390_arch_random_counter);
- return true;
+ if (in_task()) {
+ cpacf_trng(NULL, 0, (u8 *)v, sizeof(*v));
+ atomic64_add(sizeof(*v), &s390_arch_random_counter);
+ return true;
+ }
}
return false;
}
static inline bool __must_check arch_get_random_seed_int(unsigned int *v)
{
- if (static_branch_likely(&s390_arch_random_available)) {
- cpacf_trng(NULL, 0, (u8 *)v, sizeof(*v));
- atomic64_add(sizeof(*v), &s390_arch_random_counter);
- return true;
- }
return false;
}
--
2.25.1
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%[email protected]/
- 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.
> 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/[email protected]/
, 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.
Regards,
Jason
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%[email protected]/
> - 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/[email protected]/
> , 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
Hi Harald,
On Tue, Jul 12, 2022 at 02:09:35PM +0200, Harald Freudenberger wrote:
> > 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.
That's too bad, but okay.
Final question: do you see any of the in_task() vs in_whatever()
semantics changing if arch_get_random_words{,_seed}() is ever
implemented, which would reduce the current multitude of calls to the
trng to a single call?
Jason
On 2022-07-12 14:27, Jason A. Donenfeld wrote:
> Hi Harald,
>
> On Tue, Jul 12, 2022 at 02:09:35PM +0200, Harald Freudenberger wrote:
>> > 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.
>
> That's too bad, but okay.
>
> Final question: do you see any of the in_task() vs in_whatever()
> semantics changing if arch_get_random_words{,_seed}() is ever
> implemented, which would reduce the current multitude of calls to the
> trng to a single call?
>
> Jason
Hm, no, I can't see a way to provide trng random data in any whatever
interrupt context for the next future. The only enabler would be to
use a buffer ... I started to get in contact with our hardware guys
to make the trng data internally buffered and this the invocation
could be in no time give back random data. But this may be a
hardware development thing for the next machine generation.
Thanks Jason for your work
Regards
Harald Freudenberger
Hi Harald,
On Tue, Jul 12, 2022 at 04:35:41PM +0200, Harald Freudenberger wrote:
> On 2022-07-12 14:27, Jason A. Donenfeld wrote:
> > Hi Harald,
> >
> > On Tue, Jul 12, 2022 at 02:09:35PM +0200, Harald Freudenberger wrote:
> >> > 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.
> >
> > That's too bad, but okay.
> >
> > Final question: do you see any of the in_task() vs in_whatever()
> > semantics changing if arch_get_random_words{,_seed}() is ever
> > implemented, which would reduce the current multitude of calls to the
> > trng to a single call?
> >
> > Jason
>
> Hm, no, I can't see a way to provide trng random data in any whatever
> interrupt context for the next future. The only enabler would be to
> use a buffer ... I started to get in contact with our hardware guys
> to make the trng data internally buffered and this the invocation
> could be in no time give back random data. But this may be a
> hardware development thing for the next machine generation.
Alrightie then. Well, I'll Ack a v2 that keeps the _int variant. Sounds
like then we'll be done here.
Tangential topic: would be nice to add the TRNG instruction to QEMU's
TCG so that VMs in CI have a bit of randomness available.
Jason