s390/archrandom: remove CPACF trng invocations in interrupt context
Subsystems (3)
Harald Freudenberger
maintainerPATCH 1/1 s390/archrandom: remove CPACF trng invocations in interrupt context
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.
--- arch/s390/include/asm/archrandom.h | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
arch/s390/include/asm/archrandom.h +7 -9
@@ -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; }
Signature · 1 line
2.25.1
Hi Harald,
Quoted 4 lines · Harald Freudenberger
This patch introduces two things:
1) Thearch_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.
Quoted 2 lines
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.
Quoted 1 line
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.
Quoted 3 lines
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
Harald Freudenberger
maintainerQuoted 20 lines · Jason A. Donenfeld
Hi Harald,
Quoted 4 lines · Harald Freudenberger
This patch introduces two things:
1) Thearch_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.
Quoted 1 line
So please remove this snippet.
Ok, will do.
Quoted 21 lines
Quoted 3 lines
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.Quoted 1 line
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 3 lines
Regards,
Jason
Regards,
Harald Freudenberger
Hi Harald,
Quoted 12 lines · Harald Freudenberger
Quoted 3 lines
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.
Quoted 3 lines
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
Harald Freudenberger
maintainerQuoted 19 lines · Jason A. Donenfeld
Hi Harald,
Quoted 12 lines · Harald Freudenberger
Quoted 3 lines
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.Quoted 3 lines
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.
Quoted 8 lines
That's too bad, but okay.
Final question: do you see any of the
in_task()vsin_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,
Quoted 28 lines · Harald Freudenberger
Quoted 27 lines · Jason A. Donenfeld
Hi Harald,
Quoted 13 lines · Harald Freudenberger
Quoted 3 lines
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.Quoted 3 lines
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()vsin_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?
Quoted 8 lines
Quoted 1 line
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