GetRNDRRS is supported in InitHardwareRand to avoid infinite loop
#31826
max_retries) to the GetRNDRRS function to prevent it from entering an infinite loop if the hardware random number generator fails to return a valid random number. This change improves stability and ensures that the function terminates after a predefined number of retries.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31826.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Instead of changing the code and repeating the code changes in the description, it would be better to actually explain why the problem happens and why this fix is the correct fix.
To me this seems like a problem where a system claims to support a feature, but doesn’t? So it would be better to instead investigate and fix the feature test or feature reporting.
241 __asm__ volatile("mrs %0, s3_3_c2_c4_1; cset %w1, ne;"
242 : "=r"(r1), "=r"(ok)::"cc");
243 if (ok) break;
244 __asm__ volatile("yield");
245- } while (true);
246+ } while (--max_retries > 0);
GetRNSR instead, rather than returning whatever arbitrary value was left in the register?
GetRNSR.
GetRNDR?
InitHardwareRand, so this condition can be detected, and the feature can be considered unsupported (so future calls to GetRNDRS will not even bother).
To me this seems like a problem where a system claims to support a feature, but doesn’t?
I read the RNDRRS’s documentation, We have checked if bitcoin support this feature by:g_rndr_supported:
0#elif defined(__aarch64__) && defined(HWCAP2_RNG)
1
2bool g_rndr_supported = false;
3
4void InitHardwareRand()
5{
6 if (getauxval(AT_HWCAP2) & HWCAP2_RNG) {
7 g_rndr_supported = true;
8 }
9}
it would be better to instead investigate and fix the feature test or feature reporting.
Agree, it need time to invistigate this bug.
And I did some research, this CPU have bug on rndrrs:
04:28 I found out yesterday that the bitcoin project uses rndrrs if it is available on aarch64 to fill its entropy pool. It spins in a loop until its pool is filled. 04:28 This means on Oryon it will hang at startup because of the bugged rndrrs :D
Ref:
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36907425995
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
210
211 void InitHardwareRand()
212 {
213 if (getauxval(AT_HWCAP2) & HWCAP2_RNG) {
214 g_rndr_supported = true;
215+ g_rndrrs_supported = VerifyRNDRRS();
GetRNDRRS(), to fall back to GetRNDR() instead.
221@@ -206,6 +222,9 @@ void ReportHardwareRand()
222 // from global constructors, before logging is initialized.
223 if (g_rndr_supported) {
224 LogPrintf("Using RNDR and RNDRRS as additional entropy sources\n");
225+ if (!g_rndrrs_supported) {
226+ LogPrintf("Did trial test by VerifyRNDRRS, RNDRRS failed 1000 times, falling back to RNDR\n");
250@@ -233,22 +251,18 @@ uint64_t GetRNDR() noexcept
251 */
252 uint64_t GetRNDRRS() noexcept
253 {
254+ if (!g_rndrrs_supported) return GetRNDR();
SeedHardwareSlow, instead of the g_rndr_supported that is used there now.
221 // This must be done in a separate function, as InitHardwareRand() may be indirectly called
222 // from global constructors, before logging is initialized.
223- if (g_rndr_supported) {
224+ if (g_rndr_supported && g_rndrrs_supported) {
225 LogPrintf("Using RNDR and RNDRRS as additional entropy sources\n");
226+ }else if (g_rndr_supported) {
}.
201+ int max_retries = 1000;
202+ do {
203+ __asm__ volatile("mrs %0, s3_3_c2_c4_1; cset %w1, ne;"
204+ : "=r"(test), "=r"(ok)::"cc");
205+ if (ok) return true;
206+ __asm__ volatile("yield");
319+ uint64_t out;
320+ if (g_rndrrs_supported) {
321+ out = GetRNDRRS();
322+ } else {
323+ out = GetRNDR();
324+ }
GetRNDR in SeedHardwareFast which is called way more often than SeedHardwareSlow (including in SeedSlow) i’m not sure if calling it here has any additional value. Let’s just skip this loop if rndrrs isn’t supported.
SeedHardwareFast only tries to get 64 bits of entropy, while SeedHardwareSlow tries to get a full (and fresh) 256 bits, if possible. But without the rndrrs instruction, the difference isn’t all that significant (especially as SeedSlow also gets 256 bit of entropy from the OS.
Concept ACK.
Please update the PR title to reflect the new set of changes.
314@@ -297,6 +315,9 @@ void SeedHardwareSlow(CSHA512& hasher) noexcept {
315 #elif defined(__aarch64__) && defined(HWCAP2_RNG)
316 if (g_rndr_supported) {
317 for (int i = 0; i < 4; ++i) {
318+ if (!g_rndrrs_supported) {
if (g_rndr_supported) { with if (g_rndrrs_support) {).
re-ACK 585aba6
i could test it on amazon graviton again like i did the original PR, but only the good-weather case, that won’t reproduce the edge case that led to this.
Would be good if @giahuy98 (who opened #31817) could test it on the specific target hardware.
I tested 585aba6 on a Samsung Galaxy S25, and it works. Thanks.
Checked on amazon r7g.xlarge (Graviton 3):
02025-02-12T10:07:35Z Bitcoin Core version v28.99.0-585aba6eec85 (release build)
12025-02-12T10:07:35Z Using the 'arm_shani(1way,2way)' SHA256 implementation
22025-02-12T10:07:35Z Using RNDR and RNDRRS as additional entropy sources
Also verified that both GetRNDR and GetRNDRRS are still being called.
As baseline i also built and tested on rpi5 (ARM64 which has no support for RNG). It is still correctly not detected, no invalid instruction errors happen.
Combined with the report above that the workaround works, this should be safe to merge in 29.0 and maybe even backported to 28.x.
191@@ -192,20 +192,38 @@ uint64_t GetRdSeed() noexcept
192 #elif defined(__aarch64__) && defined(HWCAP2_RNG)
193
194 bool g_rndr_supported = false;
195+bool g_rndrrs_supported = false;
191@@ -192,20 +192,38 @@ uint64_t GetRdSeed() noexcept
192 #elif defined(__aarch64__) && defined(HWCAP2_RNG)
193
194 bool g_rndr_supported = false;
195+bool g_rndrrs_supported = false;
196+
197+bool VerifyRNDRRS()
198+{
199+ uint8_t ok;
I think ok and should be initialized to sane values because some sanitizers (and reviewers :p) aren’t clever enough to know if they’re guaranteed to be set by the asm.test
edit: I guess the same goes for the other copy of this.
Code review ACK 585aba6eec858e5b1411ae9a8684ef8f82a7e435. Untested. A few (non-blocking) notes, assuming this works as-is.
uint64_t and returns a bool.__rndr and __rndrrs that would imo be preferable.204+ : "=r"(test), "=r"(ok)::"cc");
205+ if (ok) return true;
206+ __asm__ volatile("yield");
207+ } while (--max_retries > 0);
208+ return false;
209+}
GetRNDRRS(); it should instead be refactored to avoid duplicating.
GetRNDRRS().
It’s unclear to me under what scenario the call might fail
The ARM documentation isn’t very specific on that:
If the hardware returns a genuine random number, PSTATE.NZCV is set to 0b0000.
If the instruction cannot return a genuine random number in a reasonable period of time, PSTATE.NZCV is set to 0b0100 and the data value returned is 0.
i imagine there’s some internal conditions under which the RNG module can’t deliver a value quickly enough. What can be concluded is that !ok is a temporary failure, for chips working according to spec. So the normal usage (as we have before this PR, and in OpenSSL and such) is to keep trying forever. This simplifies the code paths, so that callers don’t have to handle a “randomness failed” case.
The broken chips always return !ok for RNDRRS, the reason for retrying 10 times in the detection loop is to prevent false positives. That is, a working one might fail temporarily, but not for (say) 10 consecutive timeshares, so it won’t be detected as broken.
gcc (since v10) and clang (since v13) have intrinsics for __rndr and __rndrrs that would imo be preferable.
Yes. But that’s imo out of scope for this workaround PR.
220 {
221 // This must be done in a separate function, as InitHardwareRand() may be indirectly called
222 // from global constructors, before logging is initialized.
223- if (g_rndr_supported) {
224+ if (g_rndr_supported && g_rndrrs_supported) {
225 LogPrintf("Using RNDR and RNDRRS as additional entropy sources\n");
HWCAP2_RNG isn’t defined), so not much to test here.
gcc (since v10) and clang (since v13) have intrinsics for __rndr and __rndrrs that would imo be preferable.
Yes. But that’s imo out of scope for this workaround PR.
For sure out of scope, yes, I didn’t mean to imply that should be done here.
Signed-off-by: Eval EXEC <execvy@gmail.com>
196
197 void InitHardwareRand()
198 {
199 if (getauxval(AT_HWCAP2) & HWCAP2_RNG) {
200 g_rndr_supported = true;
201+ g_rndrrs_supported = VerifyRNDRRS();
I don’t think it is possible in C++ to use a function before the function signature has been given to the compiler.
I fail to see how this compiles on any compiler. See https://godbolt.org/z/cYPda9784
Locally I get:
0[ 40%] Building CXX object src/util/CMakeFiles/bitcoin_util.dir/__/random.cpp.o
1/__w/b-c-nightly/b-c-nightly/src/random.cpp:201:30: error: use of undeclared identifier 'VerifyRNDRRS'
2 201 | g_rndrrs_supported = VerifyRNDRRS();
3 | ^
(clang)
