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 <joerichey@google.com>

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<libc::c_int> {
-            // 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<libc::c_int> {
+        // 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<libc::c_int> {
-            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;
     }
 }

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.

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<libc::c_int>) -> Option<libc::c_int> {
-        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;