* [PATCH 1/1] x86: replace RDRAND forced-reseed with simple sanity check
@ 2015-07-31 15:27 Len Brown
2015-08-02 16:03 ` Pavel Machek
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Len Brown @ 2015-07-31 15:27 UTC (permalink / raw)
To: x86, linux-pm, linux-kernel; +Cc: Len Brown
From: Len Brown <len.brown@intel.com>
x86_init_rdrand() was added with 2 goals:
1. Sanity check that the built-in-self-test circuit on the Digital
Random Number Generator (DRNG) is not complaining. As RDRAND
HW self-checks on every invocation, this goal is achieved
by simply invoking RDRAND and checking its return code.
2. Force a full re-seed of the random number generator.
This was done out of paranoia to benefit the most un-sophisticated
DRNG implementation conceivable in the architecture,
an implementation that does not exist, and unlikely ever will.
This worst-case full-re-seed is achieved by invoking
a 64-bit RDRAND 8192 times.
Unfortunately, this worst-case re-seed costs O(1,000us).
Magnifying this cost, it is done from identify_cpu(), which is the
synchronous critical path to bring a processor on-line -- repeated
for every logical processor in the system at boot and resume from S3.
As it is very expensive, and of highly dubious value,
we delete the worst-case re-seed from the kernel.
We keep the 1st goal -- sanity check the hardware,
and mark it absent if it complains.
This change reduces the cost of x86_init_rdrand() by a factor of 1,000x,
to O(1us) from O(1,000us).
Signed-off-by: Len Brown <len.brown@intel.com>
---
arch/x86/kernel/cpu/rdrand.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kernel/cpu/rdrand.c b/arch/x86/kernel/cpu/rdrand.c
index 136ac74..b86817e 100644
--- a/arch/x86/kernel/cpu/rdrand.c
+++ b/arch/x86/kernel/cpu/rdrand.c
@@ -33,28 +33,26 @@ static int __init x86_rdrand_setup(char *s)
__setup("nordrand", x86_rdrand_setup);
/*
- * Force a reseed cycle; we are architecturally guaranteed a reseed
- * after no more than 512 128-bit chunks of random data. This also
- * acts as a test of the CPU capability.
+ * RDRAND has Built-In-Self-Test (BIST) that runs on every invocation.
+ * Run the instruction a few times as a sanity check.
+ * If it fails, it is simple to disable RDRAND here.
*/
-#define RESEED_LOOP ((512*128)/sizeof(unsigned long))
+#define SANITY_CHECK_LOOPS 8
void x86_init_rdrand(struct cpuinfo_x86 *c)
{
#ifdef CONFIG_ARCH_RANDOM
unsigned long tmp;
- int i, count, ok;
+ int i;
if (!cpu_has(c, X86_FEATURE_RDRAND))
- return; /* Nothing to do */
+ return;
- for (count = i = 0; i < RESEED_LOOP; i++) {
- ok = rdrand_long(&tmp);
- if (ok)
- count++;
+ for (i = 0; i < SANITY_CHECK_LOOPS; i++) {
+ if (!rdrand_long(&tmp)) {
+ clear_cpu_cap(c, X86_FEATURE_RDRAND);
+ return;
+ }
}
-
- if (count != RESEED_LOOP)
- clear_cpu_cap(c, X86_FEATURE_RDRAND);
#endif
}
--
2.5.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] x86: replace RDRAND forced-reseed with simple sanity check
2015-07-31 15:27 [PATCH 1/1] x86: replace RDRAND forced-reseed with simple sanity check Len Brown
@ 2015-08-02 16:03 ` Pavel Machek
2015-08-03 17:20 ` Len Brown
2015-08-02 17:42 ` Jeff Epler
2015-08-05 10:07 ` Ingo Molnar
2 siblings, 1 reply; 6+ messages in thread
From: Pavel Machek @ 2015-08-02 16:03 UTC (permalink / raw)
To: Len Brown; +Cc: x86, linux-pm, linux-kernel, Len Brown
On Fri 2015-07-31 11:27:39, Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
>
> x86_init_rdrand() was added with 2 goals:
>
> 1. Sanity check that the built-in-self-test circuit on the Digital
> Random Number Generator (DRNG) is not complaining. As RDRAND
> HW self-checks on every invocation, this goal is achieved
> by simply invoking RDRAND and checking its return code.
>
> 2. Force a full re-seed of the random number generator.
> This was done out of paranoia to benefit the most un-sophisticated
> DRNG implementation conceivable in the architecture,
> an implementation that does not exist, and unlikely ever will.
> This worst-case full-re-seed is achieved by invoking
> a 64-bit RDRAND 8192 times.
>
> Unfortunately, this worst-case re-seed costs O(1,000us).
> Magnifying this cost, it is done from identify_cpu(), which is the
> synchronous critical path to bring a processor on-line -- repeated
> for every logical processor in the system at boot and resume from S3.
>
> As it is very expensive, and of highly dubious value,
> we delete the worst-case re-seed from the kernel.
>
> We keep the 1st goal -- sanity check the hardware,
> and mark it absent if it complains.
If we trust built-in-self-test... why do we need to do this at all? We
should check the return value at every call, anyway...
Pavel
> This change reduces the cost of x86_init_rdrand() by a factor of 1,000x,
> to O(1us) from O(1,000us).
>
> Signed-off-by: Len Brown <len.brown@intel.com>
> ---
> arch/x86/kernel/cpu/rdrand.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/rdrand.c b/arch/x86/kernel/cpu/rdrand.c
> index 136ac74..b86817e 100644
> --- a/arch/x86/kernel/cpu/rdrand.c
> +++ b/arch/x86/kernel/cpu/rdrand.c
> @@ -33,28 +33,26 @@ static int __init x86_rdrand_setup(char *s)
> __setup("nordrand", x86_rdrand_setup);
>
> /*
> - * Force a reseed cycle; we are architecturally guaranteed a reseed
> - * after no more than 512 128-bit chunks of random data. This also
> - * acts as a test of the CPU capability.
> + * RDRAND has Built-In-Self-Test (BIST) that runs on every invocation.
> + * Run the instruction a few times as a sanity check.
> + * If it fails, it is simple to disable RDRAND here.
> */
> -#define RESEED_LOOP ((512*128)/sizeof(unsigned long))
> +#define SANITY_CHECK_LOOPS 8
>
> void x86_init_rdrand(struct cpuinfo_x86 *c)
> {
> #ifdef CONFIG_ARCH_RANDOM
> unsigned long tmp;
> - int i, count, ok;
> + int i;
>
> if (!cpu_has(c, X86_FEATURE_RDRAND))
> - return; /* Nothing to do */
> + return;
>
> - for (count = i = 0; i < RESEED_LOOP; i++) {
> - ok = rdrand_long(&tmp);
> - if (ok)
> - count++;
> + for (i = 0; i < SANITY_CHECK_LOOPS; i++) {
> + if (!rdrand_long(&tmp)) {
> + clear_cpu_cap(c, X86_FEATURE_RDRAND);
> + return;
> + }
> }
> -
> - if (count != RESEED_LOOP)
> - clear_cpu_cap(c, X86_FEATURE_RDRAND);
> #endif
> }
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] x86: replace RDRAND forced-reseed with simple sanity check
2015-07-31 15:27 [PATCH 1/1] x86: replace RDRAND forced-reseed with simple sanity check Len Brown
2015-08-02 16:03 ` Pavel Machek
@ 2015-08-02 17:42 ` Jeff Epler
2015-08-02 18:50 ` Thomas Gleixner
2015-08-05 10:07 ` Ingo Molnar
2 siblings, 1 reply; 6+ messages in thread
From: Jeff Epler @ 2015-08-02 17:42 UTC (permalink / raw)
To: Len Brown; +Cc: x86, linux-pm, linux-kernel, Len Brown
On Fri, Jul 31, 2015 at 11:27:39AM -0400, Len Brown wrote:
> if (!cpu_has(c, X86_FEATURE_RDRAND))
> - return; /* Nothing to do */
> + return;
Why remove this comment?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] x86: replace RDRAND forced-reseed with simple sanity check
2015-08-02 17:42 ` Jeff Epler
@ 2015-08-02 18:50 ` Thomas Gleixner
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Gleixner @ 2015-08-02 18:50 UTC (permalink / raw)
To: Jeff Epler; +Cc: Len Brown, x86, linux-pm, linux-kernel, Len Brown
On Sun, 2 Aug 2015, Jeff Epler wrote:
> On Fri, Jul 31, 2015 at 11:27:39AM -0400, Len Brown wrote:
> > if (!cpu_has(c, X86_FEATURE_RDRAND))
> > - return; /* Nothing to do */
> > + return;
>
> Why remove this comment?
Because it's pointless.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] x86: replace RDRAND forced-reseed with simple sanity check
2015-08-02 16:03 ` Pavel Machek
@ 2015-08-03 17:20 ` Len Brown
0 siblings, 0 replies; 6+ messages in thread
From: Len Brown @ 2015-08-03 17:20 UTC (permalink / raw)
To: Pavel Machek; +Cc: X86 ML, Linux PM list, linux-kernel, Len Brown
> If we trust built-in-self-test...
> why do we need to do this at all? We
> should check the return value at every call, anyway...
Yes, we do trust built-in-self-test.
Yes, we do check for errors on on every call, not just here in boot.
The sanity check at boot from the kernel allows Linux to disable
the feature, preventing user-space from thrashing trying to use it.
There is also a line of reasoning that if the circuit is going to fail,
chances are that it will fail immediately.
I have no reason to believe that the circuit will fail in the field
either at run-time or at boot-time. But that line of reasoning
suggests that O(1 usec) to check at boot is a prudent investment --
it is certainly a better investment per time than may of the other
things Linux does.
thanks,
Len Brown, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] x86: replace RDRAND forced-reseed with simple sanity check
2015-07-31 15:27 [PATCH 1/1] x86: replace RDRAND forced-reseed with simple sanity check Len Brown
2015-08-02 16:03 ` Pavel Machek
2015-08-02 17:42 ` Jeff Epler
@ 2015-08-05 10:07 ` Ingo Molnar
2 siblings, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2015-08-05 10:07 UTC (permalink / raw)
To: Len Brown; +Cc: x86, linux-pm, linux-kernel, Len Brown, Peter Zijlstra
* Len Brown <lenb@kernel.org> wrote:
> void x86_init_rdrand(struct cpuinfo_x86 *c)
> {
> #ifdef CONFIG_ARCH_RANDOM
> unsigned long tmp;
> - int i, count, ok;
> + int i;
>
> if (!cpu_has(c, X86_FEATURE_RDRAND))
> - return; /* Nothing to do */
> + return;
>
> - for (count = i = 0; i < RESEED_LOOP; i++) {
> - ok = rdrand_long(&tmp);
> - if (ok)
> - count++;
> + for (i = 0; i < SANITY_CHECK_LOOPS; i++) {
> + if (!rdrand_long(&tmp)) {
> + clear_cpu_cap(c, X86_FEATURE_RDRAND);
> + return;
So here we should emit a printk_once() warning that something's fishy, instead of
silently disabling a CPU feature.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-08-05 10:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-31 15:27 [PATCH 1/1] x86: replace RDRAND forced-reseed with simple sanity check Len Brown
2015-08-02 16:03 ` Pavel Machek
2015-08-03 17:20 ` Len Brown
2015-08-02 17:42 ` Jeff Epler
2015-08-02 18:50 ` Thomas Gleixner
2015-08-05 10:07 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).