| Message ID | 20220713131721.257907-1-freude@linux.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | s390/archrandom: remove CPACF trng invocations in irq context | expand |
Hi Harald, On Wed, Jul 13, 2022 at 03:17:21PM +0200, Harald Freudenberger wrote: > This patch slightly reworks the s390 arch_get_random_seed_{int,long} > implementation: Make sure the CPACF trng instruction is never > called in any interrupt context. This is done by adding an > additional condition in_task(). > > Justification: > > There are some constrains to satisfy for the invocation of the > arch_get_random_seed_{int,long}() functions: > - They should provide good random data during kernel initialization. > - They should not be called in interrupt context as the TRNG > 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. Reluctantly, Acked-by: Jason A. Donenfeld <Jason@zx2c4.com> I'll let you know if I ever get rid of or optimize the call from kfence_guarded_alloc() so that maybe there's a chance of reverting this. One small unimportant nit: > 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()) { You can avoid a level of indentation by making this: if (static_branch_likely(&s390_arch_random_available) && in_task()) But not my code so doesn't really matter to me. So have my Ack above and I'll stop being nitpicky :). Jason
On 2022-07-13 15:48, Jason A. Donenfeld wrote: > Hi Harald, > > On Wed, Jul 13, 2022 at 03:17:21PM +0200, Harald Freudenberger wrote: >> This patch slightly reworks the s390 arch_get_random_seed_{int,long} >> implementation: Make sure the CPACF trng instruction is never >> called in any interrupt context. This is done by adding an >> additional condition in_task(). >> >> Justification: >> >> There are some constrains to satisfy for the invocation of the >> arch_get_random_seed_{int,long}() functions: >> - They should provide good random data during kernel initialization. >> - They should not be called in interrupt context as the TRNG >> 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. > > Reluctantly, > > Acked-by: Jason A. Donenfeld <Jason@zx2c4.com> > > I'll let you know if I ever get rid of or optimize the call from > kfence_guarded_alloc() so that maybe there's a chance of reverting > this. > > One small unimportant nit: > >> 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()) { > > You can avoid a level of indentation by making this: > > if (static_branch_likely(&s390_arch_random_available) && in_task()) > > But not my code so doesn't really matter to me. So have my Ack above > and > I'll stop being nitpicky :). > > Jason Thanks for your ack. I'll do the improvement you suggested and then push this patch into the s390 subsystem (with cc: stable). ... and then let's see if we can establish something like arch_get_random_seed_ bytes() and a way to invoke the trng in interrupt context without the network guys complaining. regards Harald Freudenberger
Hey Harald, On Wed, Jul 13, 2022 at 05:18:47PM +0200, Harald Freudenberger wrote: > ... and then let's see if we can establish something like > arch_get_random_seed_ bytes() and a way to invoke the trng in interrupt > context without the network guys complaining. Alright, I went ahead and implemented this. Patch is here: https://lore.kernel.org/lkml/20220717112439.1795588-1-Jason@zx2c4.com/ Jason
diff --git a/arch/s390/include/asm/archrandom.h b/arch/s390/include/asm/archrandom.h index 2c6e1c6ecbe7..83ba8aa8b4b0 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 <freude@de.ibm.com> * @@ -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,9 +34,11 @@ 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; } @@ -43,9 +46,11 @@ static inline bool __must_check arch_get_random_seed_long(unsigned long *v) 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; + if (in_task()) { + cpacf_trng(NULL, 0, (u8 *)v, sizeof(*v)); + atomic64_add(sizeof(*v), &s390_arch_random_counter); + return true; + } } return false; }
This patch slightly reworks the s390 arch_get_random_seed_{int,long} implementation: Make sure the CPACF trng instruction is never called in any interrupt context. This is done by adding an additional condition in_task(). Justification: There are some constrains to satisfy for the invocation of the arch_get_random_seed_{int,long}() functions: - They should provide good random data during kernel initialization. - They should not be called in interrupt context as the TRNG 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 <freude@linux.ibm.com> --- arch/s390/include/asm/archrandom.h | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)