From 65003072af20651f2c47d4bdb1090a611c7ba6f4 Mon Sep 17 00:00:00 2001 From: Joe Richey Date: Thu, 2 Jan 2020 00:22:35 -0800 Subject: [PATCH] use_file: Avoid use of spin-locks Remove the general purpose spin-lock from getrandom, and don't spin when polling /dev/random. We can just use the poll() call itself to have threads wait for /dev/urandom to be safe. Now the only use of spin locks is to make sure open() is called exactly once when a file is used to get random data. This is fine, as the open() syscall will never block when opening a device, unlike read/writes which can obviously block. Signed-off-by: Joe Richey --- src/use_file.rs | 130 ++++++++++++++++++++++++++++++++++++----------- src/util.rs | 32 ------------ src/util_libc.rs | 31 ----------- 3 files changed, 100 insertions(+), 93 deletions(-) diff --git a/src/use_file.rs b/src/use_file.rs index d3adaf2af..65d35d834 100644 --- a/src/use_file.rs +++ b/src/use_file.rs @@ -7,8 +7,10 @@ // except according to those terms. //! Implementations that just need to read from a file -use crate::util_libc::{last_os_error, open_readonly, sys_fill_exact, LazyFd}; +use crate::util::LazyUsize; +use crate::util_libc::{last_os_error, open_readonly, sys_fill_exact}; use crate::Error; +use core::sync::atomic::{AtomicUsize, Ordering::Relaxed}; #[cfg(target_os = "redox")] const FILE_PATH: &str = "rand:\0"; @@ -21,10 +23,21 @@ const FILE_PATH: &str = "rand:\0"; target_os = "illumos" ))] const FILE_PATH: &str = "/dev/random\0"; +#[cfg(any(target_os = "android", target_os = "linux"))] +const FILE_PATH: &str = "/dev/urandom\0"; pub fn getrandom_inner(dest: &mut [u8]) -> Result<(), Error> { - static FD: LazyFd = LazyFd::new(); - let fd = FD.init(init_file).ok_or_else(last_os_error)?; + // On Linux, check that /dev/urandom will not return insecure values. + #[cfg(any(target_os = "android", target_os = "linux"))] + { + static RANDOM_INIT: LazyUsize = LazyUsize::new(); + if RANDOM_INIT.unsync_init(random_init) == LazyUsize::UNINIT { + return Err(last_os_error()); + } + } + + static FD: RngFd = RngFd::new(); + let fd = FD.open_once().ok_or_else(last_os_error)?; let read = |buf: &mut [u8]| unsafe { libc::read(fd, buf.as_mut_ptr() as *mut _, buf.len()) }; if cfg!(target_os = "emscripten") { @@ -38,36 +51,93 @@ pub fn getrandom_inner(dest: &mut [u8]) -> Result<(), Error> { Ok(()) } -cfg_if! { - if #[cfg(any(target_os = "android", target_os = "linux"))] { - fn init_file() -> Option { - // Poll /dev/random to make sure it is ok to read from /dev/urandom. - let mut pfd = libc::pollfd { - fd: unsafe { open_readonly("/dev/random\0")? }, - events: libc::POLLIN, - revents: 0, - }; +struct RngFd(AtomicUsize); + +impl RngFd { + const fn new() -> Self { + Self(AtomicUsize::new(Self::EMPTY)) + } - let ret = loop { - // A negative timeout means an infinite timeout. - let res = unsafe { libc::poll(&mut pfd, 1, -1) }; - if res == 1 { - break unsafe { open_readonly("/dev/urandom\0") }; - } else if res < 0 { - let e = last_os_error().raw_os_error(); - if e == Some(libc::EINTR) || e == Some(libc::EAGAIN) { - continue; - } + // We have not opened the file and are not waiting on an open() to complete. + const EMPTY: usize = usize::max_value(); + // We are waiting on the open(2) syscall to complete. + const OPENING: usize = usize::max_value() - 1; + + // Synchronously opens the device file used to retrive random numbers. This + // call behaves like open_readonly(FILE_PATH) except that only one caller + // will have the open() syscall running at a time, and exactly one + // successful call will be run. Once the call succeeds once, subsequent + // calls to open_once will return the same file descriptor. This common file + // descriptor is never closed. + fn open_once(&self) -> Option { + // Common and fast path with no contention. Don't wast time on CAS. + match self.0.load(Relaxed) { + Self::EMPTY | Self::OPENING => {} + val => return Some(val as libc::c_int), + } + + // Here we essentially are using a spin-lock to make sure open_readonly + // is called (successfully) exactly once. This is OK because unlike the + // read() or poll() syscalls, the open() syscall will not block if the + // file is a device. This is true even if the kernel's entropy pool + // isn't initialized. In that case, only reads will block. + loop { + // Relaxed ordering is fine, as we only have a single atomic variable. + match self.0.compare_and_swap(Self::EMPTY, Self::OPENING, Relaxed) { + Self::EMPTY => { + let fd = unsafe { open_readonly(FILE_PATH) }; + let val = match fd { + // OK as val >= 0 and val <= c_int::MAX < Self::OPENING + Some(fd) => fd as usize, + None => Self::EMPTY, + }; + self.0.store(val, Relaxed); + return fd; + } + Self::OPENING => { + // This loop will consume a negligible amount of CPU on most + // platforms, as the open_readonly() call does not block. + unsafe { libc::usleep(10) }; } - // We either hard failed, or poll() returned the wrong pfd. - break None; - }; - unsafe { libc::close(pfd.fd) }; - ret + val => return Some(val as libc::c_int), + } } - } else { - fn init_file() -> Option { - unsafe { open_readonly(FILE_PATH) } + } +} + +// Returns 0 if urandom is safe to read from, LazyUsize::UNINIT otherwise. +#[cfg(any(target_os = "android", target_os = "linux"))] +fn random_init() -> usize { + struct FdGuard(libc::c_int); + impl Drop for FdGuard { + fn drop(&mut self) { + unsafe { libc::close(self.0) }; + } + } + + // Poll /dev/random to make sure it is ok to read from /dev/urandom. + let fd = match unsafe { open_readonly("/dev/random\0") } { + Some(fd) => FdGuard(fd), + None => return LazyUsize::UNINIT, + }; + let mut pfd = libc::pollfd { + fd: fd.0, + events: libc::POLLIN, + revents: 0, + }; + + loop { + // A negative timeout means an infinite timeout. + let res = unsafe { libc::poll(&mut pfd, 1, -1) }; + if res == 1 { + return 0; + } else if res < 0 { + let e = last_os_error().raw_os_error(); + if e == Some(libc::EINTR) || e == Some(libc::EAGAIN) { + continue; + } } + // We either hard failed, or poll() returned the wrong pfd. + return LazyUsize::UNINIT; } } diff --git a/src/util.rs b/src/util.rs index e0e930752..8dbd8ae80 100644 --- a/src/util.rs +++ b/src/util.rs @@ -35,8 +35,6 @@ impl LazyUsize { // The initialization is not completed. pub const UNINIT: usize = usize::max_value(); - // The initialization is currently running. - pub const ACTIVE: usize = usize::max_value() - 1; // Runs the init() function at least once, returning the value of some run // of init(). Multiple callers can run their init() functions in parallel. @@ -50,36 +48,6 @@ impl LazyUsize { } val } - - // Synchronously runs the init() function. Only one caller will have their - // init() function running at a time, and exactly one successful call will - // be run. init() returning UNINIT or ACTIVE will be considered a failure, - // and future calls to sync_init will rerun their init() function. - pub fn sync_init(&self, init: impl FnOnce() -> usize, mut wait: impl FnMut()) -> usize { - // Common and fast path with no contention. Don't wast time on CAS. - match self.0.load(Relaxed) { - Self::UNINIT | Self::ACTIVE => {} - val => return val, - } - // Relaxed ordering is fine, as we only have a single atomic variable. - loop { - match self.0.compare_and_swap(Self::UNINIT, Self::ACTIVE, Relaxed) { - Self::UNINIT => { - let val = init(); - self.0.store( - match val { - Self::UNINIT | Self::ACTIVE => Self::UNINIT, - val => val, - }, - Relaxed, - ); - return val; - } - Self::ACTIVE => wait(), - val => return val, - } - } - } } // Identical to LazyUsize except with bool instead of usize. diff --git a/src/util_libc.rs b/src/util_libc.rs index 5a0517011..b20052808 100644 --- a/src/util_libc.rs +++ b/src/util_libc.rs @@ -89,37 +89,6 @@ impl Weak { } } -pub struct LazyFd(LazyUsize); - -impl LazyFd { - pub const fn new() -> Self { - Self(LazyUsize::new()) - } - - // If init() returns Some(x), x should be nonnegative. - pub fn init(&self, init: impl FnOnce() -> Option) -> Option { - let fd = self.0.sync_init( - || match init() { - // OK as val >= 0 and val <= c_int::MAX < usize::MAX - Some(val) => val as usize, - None => LazyUsize::UNINIT, - }, - || unsafe { - // We are usually waiting on an open(2) syscall to complete, - // which typically takes < 10us if the file is a device. - // However, we might end up waiting much longer if the entropy - // pool isn't initialized, but even in that case, this loop will - // consume a negligible amount of CPU on most platforms. - libc::usleep(10); - }, - ); - match fd { - LazyUsize::UNINIT => None, - val => Some(val as libc::c_int), - } - } -} - cfg_if! { if #[cfg(any(target_os = "linux", target_os = "emscripten"))] { use libc::open64 as open;