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;
Date:   Tue, 12 Jul 2022 12:23:31 +0200
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
To:     Harald Freudenberger <freude@linux.ibm.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
 interrupt context
Message-ID: <Ys1Loyu21C48Zm6n@zx2c4.com>
References: <20220712100829.128574-1-freude@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220712100829.128574-1-freude@linux.ibm.com>
Precedence: bulk

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%2FvZSkzWPLwIte@zx2c4.com/
- 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/7e65130c6e66ce7a9f9eb469eb7e64e0@linux.ibm.com/
, 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