Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
Date:   Wed, 13 Jul 2022 17:18:47 +0200
From:   Harald Freudenberger <freude@linux.ibm.com>
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     linux390-list@tuxmaker.boeblingen.de.ibm.com,
        linux-crypto@vger.kernel.org, linux-s390@vger.kernel.org,
        jchrist@linux.ibm.com, dengler@linux.ibm.com
Subject: Re: [PATCH] s390/archrandom: remove CPACF trng invocations in irq
 context
Reply-To: freude@linux.ibm.com
In-Reply-To: <Ys7NMKkrELPT3T6H@zx2c4.com>
References: <20220713131721.257907-1-freude@linux.ibm.com>
 <Ys7NMKkrELPT3T6H@zx2c4.com>
User-Agent: Roundcube Webmail/1.4.13
Message-ID: <38033968bc22cf97427109be0df243e1@linux.ibm.com>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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