Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate and remove getrandom_uninit #454

Closed
briansmith opened this issue Jun 4, 2024 · 7 comments
Closed

Deprecate and remove getrandom_uninit #454

briansmith opened this issue Jun 4, 2024 · 7 comments
Milestone

Comments

@briansmith
Copy link
Contributor

I had proposed and implemented getrandom_uninit as a safer alternative to the "raw" API proposed in #279. @josephlr has pointed out that we ended up adding a non-trivial amount of unsafe code for this, but there really isn't a significant performance benefit, and he suggested we remove it in the next breaking release. We've recently decided not to build other primitives like #281 on top of it within getrandom itself.

The only time there would really be a performance benefit is if one were randomly-initializing an abnormally-large (for cryptography code) buffer.

Off the time of my head, would be in the implementation of RSA (or similar) blinding on non-SIMD 32-bit targets, where we could have (8192 / 32) = 256 words to zero before calling getrandom if we don't have getrandom_uninit. Still, the syscall and actual CSPRNG computation cost is going to dominate that, and also blinding implementations typically reuse the buffer for the blinding so that the initial zeroing would only need to be done the very first time (per blinding context), so the zeroing cost is neglibile.

@briansmith briansmith mentioned this issue Jun 4, 2024
@josephlr
Copy link
Member

josephlr commented Jun 4, 2024

Copying from #439

Is it worth keeping the uninit methods?

This brings up a more important question, does it even make sense to keep the uninit methods at all? They add complexity and unsafe code, but it's unclear if they are worth it. We have benchmarks which compare calling getrandom_uninit vs zeroing a buffer and calling getrandom. Even with very fast (> 100 MB/sec) rng sources, the uninit methods aren't consistently faster (and any difference is within the margin of error).

On Linux (cargo bench --jobs=1):

test aes128::bench_getrandom        ... bench:         409 ns/iter (+/- 20) = 39 MB/s
test aes128::bench_getrandom_uninit ... bench:         413 ns/iter (+/- 37) = 38 MB/s
test p256::bench_getrandom          ... bench:         421 ns/iter (+/- 19) = 76 MB/s
test p256::bench_getrandom_uninit   ... bench:         416 ns/iter (+/- 23) = 76 MB/s
test p384::bench_getrandom          ... bench:         527 ns/iter (+/- 126) = 91 MB/s
test p384::bench_getrandom_uninit   ... bench:         543 ns/iter (+/- 169) = 88 MB/s
test page::bench_getrandom          ... bench:       8,676 ns/iter (+/- 219) = 472 MB/s
test page::bench_getrandom_uninit   ... bench:       8,591 ns/iter (+/- 551) = 476 MB/s

I would be fine removing the uninit methods entirely, as it would simplify our implementation in multiple places.

@briansmith
Copy link
Contributor Author

When working on Memory Sanitizer support (#463), I realized that the MaybeUninit functionality is actually quite useful for testing, because we can use Memory Sanitizer to ensure that we at least write to the given buffer. We can't __msan_poison a &mut [u8] without trigger UB.

In particular, if/when we update the signature of custom implementations to take MaybeUninit, then memory sanitizer will be able to also test them. So I think it is useful to keep around.

@briansmith
Copy link
Contributor Author

In ring, currently I do use the pattern let mut x = [0; LEN]; init(&mut x) in many places, but I am planning to replace those with use of MaybeUninit exactly so I can use memory sanitizer to ensure that the init() actually initializes what it is supposed to, instead of leaving some of the zeros untouched. I guess it will likely be the case for other getrandom users.

@newpavlov
Copy link
Member

I don't think that MaybeUninit adds a lot of complexity. Almost all backends (except js) use raw pointers to call system APIs and there is no difference between casting pointer from &[u8] and &mut [MaybeUninit<u8>]. Buffer zeroization is unnecessary, even if it's performance impact is negligible compared to syscalls. So I am in favor of keeping the uninit function.

@notgull
Copy link

notgull commented Jun 7, 2024

+1 to keeping it

@briansmith
Copy link
Contributor Author

I think it is fine to keep it. We have this unsafe code in getrandom mostly because stabilizing some libcore features related to MaybeUninit is taking a long time. In the future we'll be able to eliminate the unsafe bits. In the meantime, we've done quite a bit to ensure that the unsafe bits are safe. So I am fine with closing this issue, assuming this also means that we're committing to changing the custom API to be MaybeUninit-aware if/when we change that API.

@josephlr
Copy link
Member

josephlr commented Jun 8, 2024

Sounds good to me. Personally, I think the only strong argument to keep it is @briansmith's point about sanitizers, but that is a very good point.

@josephlr josephlr closed this as completed Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants