[PATCH v7] linux: Add support for getrandom vDSO
Adhemerval Zanella Netto
adhemerval.zanella@linaro.org
Thu Sep 26 17:22:05 GMT 2024
On 26/09/24 07:28, Florian Weimer wrote:
> * Jason A. Donenfeld:
>
>> diff --git a/include/sys/random.h b/include/sys/random.h
>> index 6aa313d35d..35f64a0339 100644
>> --- a/include/sys/random.h
>> +++ b/include/sys/random.h
>> @@ -1,8 +1,12 @@
>> #ifndef _SYS_RANDOM_H
>> #include <stdlib/sys/random.h>
>>
>> +#include_next <sys/random.h>
>
> This #include_next seems brittle/wrong, especially after including
> <stdlib/sys/random.h>. I would just add a new internal header, say
> sysdeps/unix/sysv/linux/include/getrandom-fork.h, and include it in all
> the relevant places, and leave include/sys/random.h unchanged.
Ack.
>
>> diff --git a/sysdeps/nptl/_Fork.c b/sysdeps/nptl/_Fork.c
>> index ef199ddbc3..adb7c18b29 100644
>> --- a/sysdeps/nptl/_Fork.c
>> +++ b/sysdeps/nptl/_Fork.c
>> @@ -18,6 +18,7 @@
>>
>> #include <arch-fork.h>
>> #include <pthreadP.h>
>> +#include <sys/random.h>
>>
>> pid_t
>> _Fork (void)
>> @@ -43,6 +44,7 @@ _Fork (void)
>> self->robust_head.list = &self->robust_head;
>> INTERNAL_SYSCALL_CALL (set_robust_list, &self->robust_head,
>> sizeof (struct robust_list_head));
>> + call_function_static_weak (__getrandom_fork_subprocess);
>> }
>> return pid;
>> }
>
> This unusual change is required to make getrandom work after _Fork.
>
> There is a small window for deadlock if a signal arrives before
> __getrandom_fork_subprocess resets the lock. Nothing is supposed to
> send any signals at this point. This could be plugged by disabling
> signals around the fork system call. I think it's required to avoid
> problems with the robust list, too, so it probably should be a separate
> change prior to this one. Filed a bug:
>
> Signal handlers after fork may encounter missing robust mutex list
> <https://sourceware.org/bugzilla/show_bug.cgi?id=32215>
Ack, I will review this patch.
>
>> diff --git a/sysdeps/nptl/fork.h b/sysdeps/nptl/fork.h
>> index 7643926df9..106b2cf71d 100644
>> --- a/sysdeps/nptl/fork.h
>> +++ b/sysdeps/nptl/fork.h
>> @@ -26,6 +26,7 @@
>
>> @@ -128,9 +130,19 @@ reclaim_stacks (void)
>> curp->specific_used = true;
>> }
>> }
>> +
>> + call_function_static_weak (__getrandom_reset_state, curp);
>> }
>> }
>>
>> + /* Also reset stale getrandom states for user stack threads. */
>> + list_for_each (runp, &GL (dl_stack_user))
>> + {
>> + struct pthread *curp = list_entry (runp, struct pthread, list);
>> + if (curp != self)
>> + call_function_static_weak (__getrandom_reset_state, curp);
>> + }
>
> I've reviewed the concurrency management for dl_stack_user, and this
> iteration should be safe for the same reason that dl_stack_cache is safe
> (but I haven't reviewed the actual approach).
Ack.
>
>> diff --git a/sysdeps/unix/sysv/linux/dl-vdso-setup.c b/sysdeps/unix/sysv/linux/dl-vdso-setup.c
>> index 3a44944dbb..b117a25922 100644
>> --- a/sysdeps/unix/sysv/linux/dl-vdso-setup.c
>> +++ b/sysdeps/unix/sysv/linux/dl-vdso-setup.c
>> @@ -66,6 +66,18 @@ PROCINFO_CLASS int (*_dl_vdso_clock_getres) (clockid_t,
>> PROCINFO_CLASS int (*_dl_vdso_clock_getres_time64) (clockid_t,
>> struct __timespec64 *) RELRO;
>> # endif
>> +# ifdef HAVE_GETRANDOM_VSYSCALL
>> +PROCINFO_CLASS ssize_t (*_dl_vdso_getrandom) (void *buffer, size_t len,
>> + unsigned int flags, void *state,
>> + size_t state_len) RELRO;
>> +/* These values will be initialized at loading time by calling the
>> + _dl_vdso_getrandom with a special value. The 'state_size' is the opaque
>> + state size per-thread allocated with a mmap using 'mmap_prot' and
>> + 'mmap_flags' argument. */
>> +PROCINFO_CLASS uint32_t _dl_vdso_getrandom_state_size RELRO;
>> +PROCINFO_CLASS uint32_t _dl_vdso_getrandom_mmap_prot RELRO;
>> +PROCINFO_CLASS uint32_t _dl_vdso_getrandom_mmap_flags RELRO;
>> +# endif
>
> I think this should be in libc, not ld.so. Initialization can happen as
> part of __libc_early_init, but only for the initial libc. This is
> required to address a dlmopen incompatibility. With the current code,
> this test will result in a buffer overflow in __getrandom_vdso_release:
>
> #include <gnu/lib-names.h>
> #include <support/check.h>
> #include <support/xdlfcn.h>
> #include <support/xthread.h>
> #include <sys/random.h>
>
> static __typeof (getrandom) *getrandom_ptr;
>
> static void *
> threadfunc (void *ignored)
> {
> char buffer;
> TEST_COMPARE (getrandom_ptr (&buffer, 1, 0), 1);
> return NULL;
> }
>
> static int
> do_test (void)
> {
> void *handle = xdlmopen (LM_ID_NEWLM, LIBC_SO, RTLD_NOW);
> getrandom_ptr = xdlsym (handle, "getrandom");
> for (int i = 0; i < 1000; ++i)
> xpthread_join (xpthread_create (NULL, threadfunc, NULL));
> return 0;
> }
>
> #include <support/test-driver.c>
>
> We could probably safely use an already-allocated state from the TCB
> from an inner libc after dlmopen, but I'm not sure if it's worth the
> additional complexity.
Right, I have added this test on next version. The secondary libc.so is
indeed something that I have not factored in this implementation but I
also think there is no need to move the vDSO setup to libc.so.
Besides not following the other vDSO setup (and thus splitting it in multiple
parts, adding some complexity), there still the issue where
_dl_call_libc_early_init is called *after* _dl_relocate_object, which is
responsible to setup RELRO. It means that we will need to either refactor
on how __libc_early_init is called, or make the internal vDSO pointer not
RELRO (and maybe XOR mangling the pointer against). I don't really like
neither option.
So I added a __getrandom_early_init, which then sets a libc.so internal
variable that is checked to issue the vDSO instead of
GLRO (dl_vdso_getrandom_state_size).
>
>> --- a/sysdeps/unix/sysv/linux/dl-vdso-setup.h
>> +++ b/sysdeps/unix/sysv/linux/dl-vdso-setup.h
>
>> @@ -50,6 +54,19 @@ setup_vdso_pointers (void)
>> #ifdef HAVE_RISCV_HWPROBE
>> GLRO(dl_vdso_riscv_hwprobe) = dl_vdso_vsym (HAVE_RISCV_HWPROBE);
>> #endif
>> +#ifdef HAVE_GETRANDOM_VSYSCALL
>> + GLRO(dl_vdso_getrandom) = dl_vdso_vsym (HAVE_GETRANDOM_VSYSCALL);
>> + if (GLRO(dl_vdso_getrandom) != NULL)
>> + {
>> + struct vgetrandom_opaque_params params;
>> + if (GLRO(dl_vdso_getrandom) (NULL, 0, 0, ¶ms, ~0UL) == 0)
>> + {
>> + GLRO(dl_vdso_getrandom_state_size) = params.size_of_opaque_state;
>> + GLRO(dl_vdso_getrandom_mmap_prot) = params.mmap_prot;
>> + GLRO(dl_vdso_getrandom_mmap_flags) = params.mmap_flags;
>> + }
>> + }
>> +#endif
>
> On my test system, size_of_opaque_state is 144 bytes, which is not a
> multiple of the cache line size. I think we need to round it up in
> userspace.
Do you mean to avoid false sharing in multithread environment? I will check
if this really make any difference.
>
>> diff --git a/sysdeps/unix/sysv/linux/getrandom.c b/sysdeps/unix/sysv/linux/getrandom.c
>> index 777d1decf0..d6025199dc 100644
>> --- a/sysdeps/unix/sysv/linux/getrandom.c
>> +++ b/sysdeps/unix/sysv/linux/getrandom.c
>> @@ -21,12 +21,247 @@
>> #include <unistd.h>
>> #include <sysdep-cancel.h>
>>
>> +static inline ssize_t
>> +getrandom_syscall (void *buffer, size_t length, unsigned int flags,
>> + bool cancel)
>> +{
>> + return cancel
>> + ? SYSCALL_CANCEL (getrandom, buffer, length, flags)
>> + : INLINE_SYSCALL_CALL (getrandom, buffer, length, flags);
>> +}
>> +
>> +#ifdef HAVE_GETRANDOM_VSYSCALL
>> +# include <getrandom_vdso.h>
>> +# include <ldsodefs.h>
>> +# include <libc-lock.h>
>> +# include <list.h>
>> +# include <setvmaname.h>
>> +# include <sys/mman.h>
>> +# include <sys/sysinfo.h>
>> +# include <tls-internal.h>
>> +
>> +# define ALIGN_PAGE(p) PTR_ALIGN_UP (p, GLRO (dl_pagesize))
>
> Not sure if this macro adds value.
It does not, I will remove it.
>
>> +# define READ_ONCE(p) (*((volatile typeof (p) *) (&(p))))
>> +# define WRITE_ONCE(p, v) (*((volatile typeof (p) *) (&(p))) = (v))
>
> Please use atomic_load_relaxed and atomic_store_relaxed.
Ack.
>
>> +# define RESERVE_PTR(p) ((void *) ((uintptr_t) (p) | 1UL))
>> +# define RELEASE_PTR(p) ((void *) ((uintptr_t) (p) & ~1UL))
>> +# define IS_RESERVED_PTR(p) (!!((uintptr_t) (p) & 1UL))
>
> Please turn those into type-safe inline functions, with a comment how
> the LSB is used and why.
Ack.
>
>> +static struct
>> +{
>> + __libc_lock_define (, lock);
>> +
>> + void **states; /* Queue of opaque states allocated with the kernel
>> + provided flags and used on getrandom vDSO call. */
>> + size_t len; /* Number of available free states in the queue. */
>> + size_t total; /* Number of states allocated from the kernel. */
>> + size_t cap; /* Total numver of states that 'states' can hold before
>> + needed to be resized. */
>> +} grnd_alloc = {
>> + .lock = LLL_LOCK_INITIALIZER
>> +};
>> +
>> +static bool
>> +vgetrandom_get_state_alloc (void)
>> +{
>> + size_t num = __get_nprocs (); /* Just a decent heuristic. */
>
> Isn't scaling by CPU count problematic if the system as a whole doesn't
> do thread-based scaling, but process-based scaling? Then you could end
> up allocating states quadratic in the number of CPUs in the system.
> Maybe start out with one page of pointers and one page of states?
>
> Call __get_nprocs is likely to cause weird issues with browser sandboxes
> because signals (including SIGSYS) are disabled at this point, and
> __get_nprocs calls open internally.
Indeed, starting with one page and avoiding __get_nproc seems more reasonable.
>
>> + size_t block_size = ALIGN_PAGE (num * GLRO(dl_vdso_getrandom_state_size));
>> + num = (GLRO (dl_pagesize) / GLRO(dl_vdso_getrandom_state_size)) *
>> + (block_size / GLRO (dl_pagesize));
>> + void *block = __mmap (NULL, block_size, GLRO(dl_vdso_getrandom_mmap_prot),
>> + GLRO(dl_vdso_getrandom_mmap_flags), -1, 0);
>> + if (block == MAP_FAILED)
>> + return false;
>> + __set_vma_name (block, block_size, " glibc: getrandom");
>> +
>> + if (grnd_alloc.total + num > grnd_alloc.cap)
>> + {
>> + /* Use a new mmap instead of trying to mremap. It avoids a
>> + potential multithread fork issue where fork is called just after
>> + mremap returns but before assigning to the grnd_alloc.states,
>> + thus making the its value invalid in the child. */
>> + void *old_states = grnd_alloc.states;
>> + size_t old_states_size = ALIGN_PAGE (sizeof (*grnd_alloc.states) *
>> + grnd_alloc.total + num);
>> + size_t states_size;
>> + if (grnd_alloc.states == NULL)
>> + states_size = old_states_size;
>> + else
>> + states_size = ALIGN_PAGE (sizeof (*grnd_alloc.states)
>> + * grnd_alloc.cap);
>> +
>> + void **states = __mmap (NULL, states_size, PROT_READ | PROT_WRITE,
>> + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
>> + if (states == MAP_FAILED)
>> + {
>> + __munmap (block, block_size);
>> + return false;
>> + }
>
> This could have a comment somewhere why memcpy is not required here. I
> think it's because grnd_alloc.len is 0. It's still necessary to
> allocate extra pointers because the deallocation routine needs to have
> space to store a pointer, and it cannot allocate itself.
The memcpy is not required because all the allocated opaque states are
assigned to running threads (meaning that if we iterate over them we can
reconstruct the state list).
And I don't think there is need to allocate any extra space: the initial
mmap already takes in account for all allocate opaque states.
>
>> + /* Atomically replace the old state, so if a fork happens the child
>> + process will see a consistent free state buffer. The size might
>> + not be updated, but it does not really matter since the buffer is
>> + always increased. */
>> + atomic_store_relaxed (&grnd_alloc.states, states);
>
> The comment is good, but I think it means you should use release MO
> here.
Ack, I was not really sure about it but it make sense (the NPTL stack
cache is not clear about the memory semantic it uses for fork synchronization).
>
>> + if (old_states != NULL)
>> + __munmap (old_states, old_states_size);
>> +
>> + __set_vma_name (states, states_size, " glibc: getrandom states");
>> + grnd_alloc.cap = states_size / sizeof (*grnd_alloc.states);
>> + }
>> +
>> + for (size_t i = 0; i < num; ++i)
>> + {
>> + /* States should not straddle a page. */
>> + if (((uintptr_t) block & (GLRO (dl_pagesize) - 1)) +
>> + GLRO(dl_vdso_getrandom_state_size) > GLRO (dl_pagesize))
>> + block = ALIGN_PAGE (block);
>> + grnd_alloc.states[i] = block;
>> + block += GLRO(dl_vdso_getrandom_state_size);
>> + }
>> + grnd_alloc.len = num;
>
> Probably should have release MO here as well due to potentially
> concurrent fork that shouldn't observe the previous pointer values in
> the array.
Ack, makes sense.
>
>> + grnd_alloc.total += num;
>> +
>> + return true;
>> +}
>> +
>> +/* Allocate an opaque state for vgetrandom. If the grnd_alloc does not have
>> + any, mmap() another page of them using the vgetrandom parameters. */
>> +static void *
>> +vgetrandom_get_state (void)
>> +{
>> + void *state = NULL;
>> +
>> + /* The signal blocking avoid the potential issue where _Fork() (which is
>> + async-signal-safe) is called with the lock taken. The function is
>> + called only once during thread lifetime, so the overhead should be
>> + minimal. */
>> + internal_sigset_t set;
>> + internal_signal_block_all (&set);
>> + __libc_lock_lock (grnd_alloc.lock);
>> +
>> + if (grnd_alloc.len > 0 || vgetrandom_get_state_alloc ())
>> + state = grnd_alloc.states[--grnd_alloc.len];
>> +
>> + __libc_lock_unlock (grnd_alloc.lock);
>> + internal_signal_restore_set (&set);
>> +
>> + return state;
>> +}
>
> This async-signal-safe locking looks okay to me (assuming the fix in
> _Fork discussed above).
>
>> +/* Returns true when vgetrandom is used successfully. Returns false if the
>> + syscall fallback should be issued in the case the vDSO is not present, in
>> + the case of reentrancy, or if any memory allocation fails. */
>> +static ssize_t
>> +getrandom_vdso (void *buffer, size_t length, unsigned int flags, bool cancel)
>> +{
>> + if (GLRO (dl_vdso_getrandom_state_size) == 0)
>> + return getrandom_syscall (buffer, length, flags, cancel);
>> +
>> + struct pthread *self = THREAD_SELF;
>
>> + /* Since the vDSO fallback does not issue the syscall with the cancellation
>
> The vDSO *implementation*?
Ack.
>
>> + bridge (__syscall_cancel_arch), use GRND_NONBLOCK so there is no
>> + potential unbounded blocking in the kernel. It should be a rare
>> + situation, only at system startup when RNG is not initialized. */
>> + ssize_t ret = GLRO (dl_vdso_getrandom) (buffer,
>> + length,
>> + flags | GRND_NONBLOCK,
>> + state,
>> + GLRO(dl_vdso_getrandom_state_size));
>> + if (INTERNAL_SYSCALL_ERROR_P (ret))
>> + {
>> + /* Fallback to the syscall if the kernel would block. */
>> + int err = INTERNAL_SYSCALL_ERRNO (ret);
>> + if (err == EAGAIN && !(flags & GRND_NONBLOCK))
>> + goto out;
>> +
>> + __set_errno (err);
>> + ret = -1;
>> + }
>> + r = true;
>> +
>> +out:
>> + WRITE_ONCE (self->getrandom_buf, state);
>> + return r ? ret : getrandom_syscall (buffer, length, flags, cancel);
>> +}
>
> I think we should conditionally invoke __pthread_testcancel if
> getrandom_syscall is not called. If performance is a concern, we can
> provide an internal inlineable implementation of its fast path. It's
> just a memory load, compare and well-predictable conditional branch, so
> it really shouldn't matter.
I think we already have addressed why __pthread_testcancel is not required here,
from the commit message:
The cancellation is handled by always using GRND_NONBLOCK flags while
calling the vDSO, and falling back to the cancellable syscall if the
kernel returns EAGAIN (would block). Since getrandom is not defined by
POSIX and cancellation is supported as an extension, the cancellation is
handled as 'may occur' instead of 'shall occur' [1], meaning that if
vDSO does not block (the expected behavior) getrandom will not act as a
cancellation entrypoint. It avoids a pthread_testcancel call on the fast
path (different than 'shall occur' functions, like sem_wait()).
I still think we should not make getrandom a 'shall occur' cancellation
entrypoint, on the grounds that recent kernel blocking is really rare
and it simplifies the vDSO implementation.
>
>> +#endif
>> +
>> +/* Re-add the state state from CURP on the free list. */
>> +void
>> +__getrandom_reset_state (struct pthread *curp)
>> +{
>> +#ifdef HAVE_GETRANDOM_VSYSCALL
>> + if (grnd_alloc.states == NULL || curp->getrandom_buf == NULL)
>> + return;
>> + grnd_alloc.states[grnd_alloc.len++] = RELEASE_PTR (curp->getrandom_buf);
>> + curp->getrandom_buf = NULL;
>> +#endif
>> +}
>
> This could have an assert for grnd_alloc.len, to make sure it's within
> bounds.
>
Ack.
> I think it's missing locking. It's tempting to push that into the
> caller, like for __getrandom_vdso_release.
>
I don't think there is need for locking here, __getrandom_reset_state is only
called *after* fork returns and thus in single-thread mode. I will add a
comment.
>> +/* Called when a thread terminates, and adds its random buffer back into the
>> + allocator pool for use in a future thread. */
>> +void
>> +__getrandom_vdso_release (struct pthread *curp)
>> +{
>> +#ifdef HAVE_GETRANDOM_VSYSCALL
>> + if (curp->getrandom_buf == NULL)
>> + return;
>> +
>
> Should have a comment here that signals have already been disabled at
> this point during thread exit.
Ack.
>
>> + __libc_lock_lock (grnd_alloc.lock);
>> + grnd_alloc.states[grnd_alloc.len++] = curp->getrandom_buf;
>> + __libc_lock_unlock (grnd_alloc.lock);
>> +#endif
>> +}
>
> Maybe put in an assert here, considering the buffer overflow described
> above? So that we see that there's another buffer management issue?
Not sure if this worths the checking, it means that something is really wrong
on the initial state allocation. The opaque state buffer is essentially a stack.
>
>> diff --git a/sysdeps/unix/sysv/linux/include/sys/random.h b/sysdeps/unix/sysv/linux/include/sys/random.h
>> new file mode 100644
>> index 0000000000..5a48de2d29
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/include/sys/random.h
>
> As discussed above, this should go into a separate header that does not
> mess with <sys/random.h>.
Ack.
>
> Performance is very good, not too far off from the random function
> actually.
>
> Thanks,
> Florian
>
More information about the Libc-alpha
mailing list