Following Buggy AMD RdRand, The Linux Kernel Will Begin Sanity Checking Randomness At Boot Time

[johnp117]

While true, the calls to rdrand() is probably so quick that adding a break in the loop will #1 not improve the performance in any meaningful way even on a low power system and #2 might actually lower performance (since a break statement would add code inside the tight loop).

Given that `RDRAND` is quite consistently the instruction with the highest reciprocal throughput I find that unlikely: https://www.agner.org/optimize/instruction_tables.pdf
Still, we're only talking about a couple thousand clock cycles, which should be imperceptible for users.

[Marc.2377]

Except that code's still not very good, unless I'm missing something, because all it's looking for is any change ever, but it still runs SANITY_CHECK_LOOPS instead of breaking out once it finds one...

Now that is a good point.

You're missing the point of the comment. If one change is good enough, the loop should just break out once that happens, rather than continuing to loop and uselessly accumulating the number of changes.

Actually, although that's what the current code is doing, I would think it should kinda be the opposite. If my understanding is correct, one single non-change should be bad enough to break the loop and emit the error message by calling 'pr_emerg()'.

While true, the calls to rdrand() is probably so quick that adding a break in the loop will #1 not improve the performance in any meaningful way even on a low power system and #2 might actually lower performance (since a break statement would add code inside the tight loop).

#2 is not true. A 'break' statement is usually not detrimental unless it requires a condition - and even then, the cost of the aditional branch (if misprediction happens) must be weighed against the cost of not breaking the loop. But in the case of the code at hand, the condition is already there. I checked with Compiler Explorer to be sure. A 'break' statement cannot lead to lower performance in this case, even with compiler optimizations disabled.

[Marc.2377]

Actually, although that's what the current code is doing, I would think it should kinda be the opposite. If my understanding is correct, one single non-change should be bad enough to break the loop and emit the error message by calling 'pr_emerg()'.

Should we bring this to the mailing list? I feel a bit awkward, and I'm not a C programmer. Plus, this is Borislav Petkov's code. My understanding is probably wrong.

What you guys think?

[tildearrow]

If you examine the code in the patch alone, you might conclude that. But consider the entire function after applying the patch:

[COLOR=#d55fde]void[/COLOR][COLOR=#61afef] x86_init_rdrand[/COLOR][COLOR=#abb2bf]([/COLOR][COLOR=#d55fde]struct[/COLOR][COLOR=#ef596f] cpuinfo_x86 [/COLOR][COLOR=#d55fde]*[/COLOR][COLOR=#ef596f][I]c[/I][/COLOR][COLOR=#abb2bf])[/COLOR]
[COLOR=#abb2bf]{[/COLOR]
[COLOR=#d55fde] unsigned[/COLOR][COLOR=#d55fde] int[/COLOR][COLOR=#abb2bf] changed [/COLOR][COLOR=#d55fde]=[/COLOR][COLOR=#d19a66] 0[/COLOR][COLOR=#abb2bf];[/COLOR]
[COLOR=#d55fde]unsigned[/COLOR][COLOR=#d55fde] long[/COLOR][COLOR=#abb2bf] tmp[/COLOR][COLOR=#abb2bf],[/COLOR][COLOR=#abb2bf] prev[/COLOR][COLOR=#abb2bf];[/COLOR]
[COLOR=#d55fde]int[/COLOR][COLOR=#abb2bf] i[/COLOR][COLOR=#abb2bf];[/COLOR]

[COLOR=#d55fde] if[/COLOR][COLOR=#abb2bf] ([/COLOR][COLOR=#2bbac5]![/COLOR][COLOR=#61afef]cpu_has[/COLOR][COLOR=#abb2bf](c[/COLOR][COLOR=#abb2bf],[/COLOR][COLOR=#abb2bf] X86_FEATURE_RDRAND)[/COLOR][COLOR=#abb2bf])[/COLOR]
[COLOR=#d55fde]return[/COLOR][COLOR=#abb2bf];[/COLOR]

[COLOR=#d55fde]for [/COLOR][COLOR=#abb2bf]([/COLOR][COLOR=#abb2bf]i [/COLOR][COLOR=#d55fde]= [/COLOR][COLOR=#d19a66]0[/COLOR][COLOR=#abb2bf];[/COLOR][COLOR=#abb2bf] i [/COLOR][COLOR=#d55fde]<[/COLOR][COLOR=#abb2bf] SANITY_CHECK_LOOPS[/COLOR][COLOR=#abb2bf];[/COLOR][COLOR=#abb2bf] i[/COLOR][COLOR=#d55fde]++[/COLOR][COLOR=#abb2bf])[/COLOR][COLOR=#abb2bf] {[/COLOR]
[COLOR=#d55fde]if[/COLOR][COLOR=#abb2bf] ([/COLOR][COLOR=#2bbac5]![/COLOR][COLOR=#61afef]rdrand_long[/COLOR][COLOR=#abb2bf]([/COLOR][COLOR=#d55fde]&[/COLOR][COLOR=#abb2bf]tmp)[/COLOR][COLOR=#abb2bf]) [/COLOR][COLOR=#abb2bf]{[/COLOR]
[COLOR=#61afef]clear_cpu_cap[/COLOR][COLOR=#abb2bf](c[/COLOR][COLOR=#abb2bf],[/COLOR][COLOR=#abb2bf] X86_FEATURE_RDRAND)[/COLOR][COLOR=#abb2bf];[/COLOR]
[COLOR=#61afef]pr_warn_once[/COLOR][COLOR=#abb2bf]([/COLOR][COLOR=#89ca78]"rdrand: disabled[/COLOR][COLOR=#2bbac5]\n[/COLOR][COLOR=#89ca78]"[/COLOR][COLOR=#abb2bf])[/COLOR][COLOR=#abb2bf];[/COLOR]
[COLOR=#d55fde]return[/COLOR][COLOR=#abb2bf];[/COLOR]
[COLOR=#abb2bf]}[/COLOR]
[COLOR=#abb2bf]}[/COLOR]

[COLOR=#7f848e][I] /*[/I][/COLOR]
[COLOR=#7f848e][I] * Stupid sanity-check whether RDRAND does *actually* generate[/I][/COLOR]
[COLOR=#7f848e][I] * some at least random-looking data.[/I][/COLOR]
[COLOR=#7f848e][I] */[/I][/COLOR]
[COLOR=#abb2bf] prev [/COLOR][COLOR=#d55fde]=[/COLOR][COLOR=#abb2bf] tmp[/COLOR][COLOR=#abb2bf];[/COLOR]
[COLOR=#d55fde]for [/COLOR][COLOR=#abb2bf]([/COLOR][COLOR=#abb2bf]i [/COLOR][COLOR=#d55fde]=[/COLOR][COLOR=#d19a66] 0[/COLOR][COLOR=#abb2bf];[/COLOR][COLOR=#abb2bf] i [/COLOR][COLOR=#d55fde]<[/COLOR][COLOR=#abb2bf] SANITY_CHECK_LOOPS[/COLOR][COLOR=#abb2bf];[/COLOR][COLOR=#abb2bf] i[/COLOR][COLOR=#d55fde]++[/COLOR][COLOR=#abb2bf])[/COLOR][COLOR=#abb2bf] {[/COLOR]
[COLOR=#d55fde]if[/COLOR][COLOR=#abb2bf] ([/COLOR][COLOR=#61afef]rdrand_long[/COLOR][COLOR=#abb2bf]([/COLOR][COLOR=#d55fde]&[/COLOR][COLOR=#abb2bf]tmp)[/COLOR][COLOR=#abb2bf])[/COLOR][COLOR=#abb2bf] {[/COLOR]
[COLOR=#d55fde]if[/COLOR][COLOR=#abb2bf] ([/COLOR][COLOR=#abb2bf]prev [/COLOR][COLOR=#d55fde]!=[/COLOR][COLOR=#abb2bf] tmp[/COLOR][COLOR=#abb2bf])[/COLOR]
[COLOR=#abb2bf] changed[/COLOR][COLOR=#d55fde]++[/COLOR][COLOR=#abb2bf];[/COLOR]

[COLOR=#abb2bf] prev [/COLOR][COLOR=#d55fde]=[/COLOR][COLOR=#abb2bf] tmp[/COLOR][COLOR=#abb2bf];[/COLOR]
[COLOR=#abb2bf]}[/COLOR]
[COLOR=#abb2bf]}[/COLOR]

[COLOR=#d55fde]if[/COLOR][COLOR=#abb2bf] ([/COLOR][COLOR=#61afef]WARN_ON_ONCE[/COLOR][COLOR=#abb2bf]([/COLOR][COLOR=#2bbac5]![/COLOR][COLOR=#abb2bf]changed)[/COLOR][COLOR=#abb2bf])[/COLOR]
[COLOR=#61afef]pr_emerg[/COLOR][COLOR=#abb2bf]([/COLOR]
[COLOR=#89ca78]"RDRAND gives funky smelling output, might consider not using it by booting with [/COLOR][COLOR=#2bbac5]\"[/COLOR][COLOR=#89ca78]nordrand[/COLOR][COLOR=#2bbac5]\"[/COLOR][COLOR=#89ca78]"[/COLOR][COLOR=#abb2bf])[/COLOR][COLOR=#abb2bf];[/COLOR]

[COLOR=#abb2bf]}[/COLOR]

Notice how 'tmp' is assigned to, at line 12 in the above code.

(Edit: formatting; had to replace tabs with spaces, sorry!)

How did you get colors inside a code section?

[Marc.2377]

How did you get colors inside a code section?

Nice, heh? I just copied from VS Code and pasted here. The colors are not quite right, though. And the formatting is messed up (see how some of the indenting was messed inside your blockquote).

Btw I use the One Dark Pro theme (the 'Vivid' variant). The font is Adobe Source Code Pro.

[F.Ultra]

#2 is not true. A 'break' statement is usually not detrimental unless it requires a condition - and even then, the cost of the aditional branch (if misprediction happens) must be weighed against the cost of not breaking the loop. But in the case of the code at hand, the condition is already there. I checked with Compiler Explorer to be sure. A 'break' statement cannot lead to lower performance in this case, even with compiler optimizations disabled.

You are probably correct, I was speaking more of a general case since I've seen quite a lot of "optimizations" done in this way that in the end have lead to slower code in practice. However what the compiler does is only half of the truth, then comes the modern CPU that can do all sorts of magic things behind the curtain.

I have an example that I use to show to people where what looks like an obvious optimization actually for some reason make things slower which you can find here: https://www.phoronix.com/forums/foru...57#post1069357, and the experience from that is what always makes me cautious about making claims about "obvious optimizations".

[xorbe]

Why single out RDRAND? Let's check MOV, JMP, CMP & co while we're at it

The serious answer here is that MOV, JMP, etc are cpu only, whereas RDRAND has a dependency on boot/resume firmware.