Re: [PATCH v2] linux: Add support for getrandom vDSO

["Jason A. Donenfeld" <Jason@zx2c4.com>]

> >> +            states = __mremap (grnd_alloc.states,
> >> +                               ALIGN_PAGE (sizeof (*grnd_alloc.states) *
> >> +                                           grnd_alloc.cap),
> >> +                               states_size, MREMAP_MAYMOVE);
> > 
> > I think if we fork after this point (after the mremap), the new process
> > will no longer have the pointer array at grnd_alloc.states.  Currently,
> > it is not safe to simply reset grnd_alloc.lock in the subprocess.
> > 
> > It may be easier to just use a list of pages, so that a fork at the
> > wrong time merely loses track of the newly allocated page.
> 
> The list of pages would have similar issues where it would need to put
> the address somewhere, either in the free-state itself (which adds
> extra complexity in the free-state tracking) or by adding a static array
> to track it.
> 
> Instead I think that since the idea it to keep the getrandom call 
> *consistent* after fork and not really avoid possible resource leakage
> (simiar to the thread stack cache), reallocate the free list with
> mmap, atomically replace the pointer, and munmap will be suffice.

Or, in order to avoid already allocated states from having invalid
addresses post-unmap, if you want more states, just mmap a new set of
states. Because we're already allocating lots per chunk, I don't think
practically speaking this should pose a real issue.


> > I thought that the kernel would drop those mappings on fork?  Or do they
> > stick around and just get replaced with the zero page?
> 
> The kernel does not drop, afaik MAP_DROPPABLE (used by getrandom vDSO
> pages) follows usual mmap semantic regarding fork().

Right, they get zeroed.

> > We need to discuss what we should do about getrandom as a cancellation
> > point.  If no longer want it to be a cancellation point, that should be
> > changed first, in a separate patch, I think.
> 
> Indeed, the vDSO currently always issues the syscall if it can not use
> the opaque state for any reason (invalid size, RNG is not ready, reentracy
> protection, etc) and this breaks the SYSCALL_CANCEL macro.
> 
> I don't have a strong opinion here, maybe a better option would to just
> avoid calling the syscall on the vDSO (but this would make it differ from
> other vDSO symbols).  Jason, what do you think?  Would current getrandom
> implementation block?

The vDSO will sometimes make the syscall, particularly in the case when
the RNG is not yet initialized. On modern kernels, the RNG gets
initialized very fast, and there's generally not an issue with "blocking
forever and ever" like there used to be. But with that said, at early
boot it still _might_ block for a quarter second or so, though it will
only do so once.

So I think we could preserve its cancelability, but we'll only need to
do so for the first invocation of it. So we could optimize it to
something like this:

diff --git a/sysdeps/unix/sysv/linux/getrandom.c b/sysdeps/unix/sysv/linux/getrandom.c
index c7806810..75a52c37 100644
--- a/sysdeps/unix/sysv/linux/getrandom.c
+++ b/sysdeps/unix/sysv/linux/getrandom.c
@@ -180,8 +180,21 @@ __getrandom_vdso (ssize_t *ret, void *buffer, size_t length,
         goto out;
     }
 
-  *ret = GLRO (dl_vdso_getrandom) (buffer, length, flags, state,
-                                   grnd_alloc.state_size);
+  static bool did_succeed = false;
+  bool could_block = !did_succeed && !(flags & (GRND_NONBLOCK | GRND_INSECURE));
+  if (NO_SYSCALL_CANCEL_CHECKING || !could_block)
+    *ret = glro (dl_vdso_getrandom) (buffer, length, flags, state,
+                                     grnd_alloc.state_size);
+  else
+    {
+      int sc_cancel_oldtype = LIBC_CANCEL_ASYNC ();
+      *ret = glro (dl_vdso_getrandom) (buffer, length, flags, state,
+                                       grnd_alloc.state_size);
+      LIBC_CANCEL_RESET (sc_cancel_oldtype);
+      if (!INTERNAL_SYSCALL_ERROR_P (*ret))
+        did_succeed = true;
+    }
+
   if (INTERNAL_SYSCALL_ERROR_P (*ret))
     {
       __set_errno (INTERNAL_SYSCALL_ERRNO (*ret));