Switch backup entropy generator to JitterRng #196
Conversation
JitterRng is a much stronger generator and available on most platforms; see http://www.chronox.de/jent.html Drawbacks are speed and potential failure on some platforms
|
Looks good! Surprisingly little changes where necessary. Also a reason not to include tests is that the extra debug instructions have so much influence, that you are not really testing the timer anymore. |
|
Oh, you mean the usual test mode is debug mode which is inappropriate for this code? Would it be better to add a small benchmark then? Edit: added a small benchmark and CI benchmarking. I can remove if necessary, but it may be useful (lets see if any CI failures occur). |
Partly this is just to add an *optimised* test
|
Then the benchmark would run the tests in |
|
Hmm, again the standard library exposes a timer with 100ns precision, and we can't read the value of the high-resolution one used in |
|
You were faster than I could make a second pr... |
| // the CPU any more and therefore a wider range of CPU wait states is | ||
| // necessary for accesses. L3 and real memory accesses have even a wider | ||
| // range of wait states. However, to reliably access either L3 or memory, | ||
| // the `self.mem` memory must be quite large which is usually not desirable. |
There was a problem hiding this comment.
Sounds to me like accesses in this block could be replaced with seqcst atomic operations and memory fences (aka. barriers) to make it contribute a significantly greater amount of jitter per round.
There was a problem hiding this comment.
But the RNG isn't multi-threaded? Also part of the goal was to make the code usable on embedded systems which provide their own timer (the timer itself is the only part of JitterRng which is not no_std compatible).
There was a problem hiding this comment.
Fences (a building block for atomic memory operations) are also available and important on single-threaded machines as well. For example (citing this, but applicable in general):
- processors can re-order memory transfers to improve performance if it does not affect the behavior of the instruction sequence;
- processors can have multiple bus interfaces that have different wait-states;
- processors can have write buffers, and some write buffers can merge multiple transfers into a single transfer.
A fence will ensure that the memory will see the writes come in the right order regardless of any of the considerations above.
Only the most primitive chip designs will not have barriers and would fall back to regular reads, however these same most primitive designs have fairly constant memory access latency, and memory accesses would contribute no jitter anyway.
I don’t think I see any reason to not use what’s essentially the most jitter-susceptible and fairly well supported operation to collect entropy.
At least I would be significantly more comfortable with implementation using fences than the one that tries to outsmart the modern processors’ caching strategy with what seems to be a linear access pattern.
There was a problem hiding this comment.
That seems like a better way! (I have never used barriers though).
Yet I would like to leave it as it is for now, because this mirrors jitterentropy. That one has research and statistics done on it, and it is nice to be able to point there as a 'proof' our entropy collection method works. That and the NIST entropy test.
There was a problem hiding this comment.
Note that JitterRng can work pretty well without this part, previous versions of jitterentropy did not have it. If in the future CPU's are to smart for this method test_timer will just say more rounds of measure_jitter are necessary .
There was a problem hiding this comment.
I’m fine with keeping the implementation as-is for now, but I would like to see an issue filled to collect information and explore better ways to collect jitter-based entropy based on memory accesses.
http://chronox.de/jent/doc/CPU-Jitter-NPTRNG.html is something I’ve found a few minutes ago. It also references a number of other jitter-based RNG implementations and has measurements for barriers (though it seems it does not do actual memory accesses in the measurement).
There was a problem hiding this comment.
That is exactly the one this is based on. If you look at the original pr dhardy#28, I have a comment and commit messages so that our Rust code can be read side-by-side with the code from jitterentropy.
|
@dhardy I don't have too many thoughts here myself other than that I trust you've got it in hand :) |
|
From my point of view this is ready, but I'm not sure if @nagisa was happy with us merging this version? One thing to point out is that this is a much stronger RNG than what it replaces and I don't think anybody has started implementing the suggested improvements using memory fences, so it may be better to merge and leave an open issue with possible future improvements. |
|
An open issue is fine. I don't find my comments critical given that memory
access jitter is only one of a few entropy sources.
…On Nov 27, 2017 22:33, "Diggory Hardy" ***@***.***> wrote:
From my point of view this is ready, but I'm not sure if @nagisa
<https://github.com/nagisa> was happy with us merging this version? One
thing to point out is that this is a much stronger RNG than what it
replaces and I don't think anybody has started implementing the suggested
improvements using memory fences, so it may be better to merge and leave an
open issue with possible future improvements.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#196 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AApc0gjC3j5BRWcx6SND_QtvP3poBqRbks5s6xx8gaJpZM4QqcRS>
.
|
Switch backup entropy generator to JitterRng
This replaces the weak backup generator added by @cpeterso in #181, using the jitter RNG implemented by @pitdicker.
See also dhardy#22
Travis testing on various platforms: https://travis-ci.org/dhardy/rand/builds/307142531 (testing fails on cross-compiled platforms where binaries are not compatible, works on Linux, Mac; untested on Windows); note: the timer is not tested in the code in this PR because it is platform dependent, although we could add a test if desired (it does work on a lot of platforms).
@pitdicker review? @alexcrichton thoughts?