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;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>,
        linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org,
        arnd@arndb.de
Cc:     "Jason A. Donenfeld" <Jason@zx2c4.com>,
        Borislav Petkov <bp@alien8.de>, x86@kernel.org
Subject: Re: [PATCH v6 13/17] x86: use fallback for random_get_entropy()
 instead of zero
In-Reply-To: <20220423212623.1957011-14-Jason@zx2c4.com>
References: <20220423212623.1957011-1-Jason@zx2c4.com>
 <20220423212623.1957011-14-Jason@zx2c4.com>
Date:   Mon, 25 Apr 2022 14:35:39 +0200
Message-ID: <871qxl2vdw.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Sat, Apr 23 2022 at 23:26, Jason A. Donenfeld wrote:

Please follow the guidelines of the tip maintainers when touching x86
code. https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-subject

> In the event that random_get_entropy() can't access a cycle counter or
> similar, falling back to returning 0 is really not the best we can do.
> Instead, at least calling random_get_entropy_fallback() would be
> preferable, because that always needs to return _something_, even
> falling back to jiffies eventually. It's not as though
> random_get_entropy_fallback() is super high precision or guaranteed to
> be entropic, but basically anything that's not zero all the time is
> better than returning zero all the time.
>
> If CONFIG_X86_TSC=n, then it's possible that we're running on a 486
> with no RDTSC, so we only need the fallback code for that case.

There are also 586 CPUs which lack TSC.

> While we're at it, fix up both the new function and the get_cycles()
> function from which its derived to use cpu_feature_enabled() rather
> than boot_cpu_has().

Lot's of 'we' here. We are not doing anything.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

> +static inline unsigned long random_get_entropy(void)
> +{
> +#ifndef CONFIG_X86_TSC
> +	if (!cpu_feature_enabled(X86_FEATURE_TSC))
> +		return random_get_entropy_fallback();
> +#endif

Please get rid of this ifdeffery. While you are right, that anything
with CONFIG_X86_TSC=y should have a TSC, there is virt ....

cpu_feature_enabled() is runtime patched and only evaluated before
alternative patching, so the win of this ifdef is marginally, if even
noticable.

We surely can think about making TSC mandatory, but not selectively in a
particalur context.

Thanks,

        tglx