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

["Jason A. Donenfeld" <Jason@zx2c4.com>]

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