Re: git: 8271d9b99a3b - main - libsys: remove usage of pthread_once and _once_stub

On 21 Feb 2024, at 00:29, Konstantin Belousov <kib@FreeBSD.org> wrote:
> 
> The branch main has been updated by kib:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=8271d9b99a3b98c662ee9a6257a144284b7e1728
> 
> commit 8271d9b99a3b98c662ee9a6257a144284b7e1728
> Author:     Konstantin Belousov <kib@FreeBSD.org>
> AuthorDate: 2024-02-20 14:45:29 +0000
> Commit:     Konstantin Belousov <kib@FreeBSD.org>
> CommitDate: 2024-02-21 00:26:11 +0000
> 
>    libsys: remove usage of pthread_once and _once_stub
> 
>    that existed in auxv.c, use simple bool gate instead. This leaves a
>    small window if two threads try to call _elf_aux_info(3) simultaneously.
>    The situation is safe because auxv parsing is really idempotent. The
>    parsed data is the same, and we store atomic types (int/long/ptr) so
>    double-init does not matter.

You still need to load acquire and store release aux_once though,
otherwise you can see aux_once as true yet read the pre-initialised
data. In practice that’s surely very hard to hit, but the code as
written is now wrong. Also, idempotence should probably be made
unnecessary by using 0/1/2 state for uninitialised/initialising/
initialised, as it’s still technically UB from a C AM perspective due
to not being data race free if two threads initialise at the same time.
Better to just do the correct thing rather than risk things going wrong.

Jess

>    Reviewed by:    brooks, imp
>    Sponsored by:   The FreeBSD Foundation
>    Differential revision:  https://reviews.freebsd.org/D43985
> ---
> lib/libc/gen/Makefile.inc             |  1 +
> lib/{libsys => libc/gen}/_once_stub.c |  0
> lib/libsys/Makefile.sys               |  1 -
> lib/libsys/auxv.c                     | 20 ++++++++++++++++++--
> 4 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/libc/gen/Makefile.inc b/lib/libc/gen/Makefile.inc
> index 8d30e06cfed9..ce7a34d0e58e 100644
> --- a/lib/libc/gen/Makefile.inc
> +++ b/lib/libc/gen/Makefile.inc
> @@ -9,6 +9,7 @@ CONFSPACKAGE=   runtime
> SRCS+= \
> __pthread_mutex_init_calloc_cb_stub.c \
> __xuname.c \
> + _once_stub.c \
> _pthread_stubs.c \
> _rand48.c \
> _spinlock_stub.c \
> diff --git a/lib/libsys/_once_stub.c b/lib/libc/gen/_once_stub.c
> similarity index 100%
> rename from lib/libsys/_once_stub.c
> rename to lib/libc/gen/_once_stub.c
> diff --git a/lib/libsys/Makefile.sys b/lib/libsys/Makefile.sys
> index cb9ca1749ba8..e33a11bacb57 100644
> --- a/lib/libsys/Makefile.sys
> +++ b/lib/libsys/Makefile.sys
> @@ -33,7 +33,6 @@ PSEUDO+= _clock_gettime.o _gettimeofday.o
> SRCS+= \
> __error.c \
> __getosreldate.c \
> - _once_stub.c \
> getpagesize.c \
> getpagesizes.c \
> interposing_table.c
> diff --git a/lib/libsys/auxv.c b/lib/libsys/auxv.c
> index b0b3a8ed708b..88f49ef53be1 100644
> --- a/lib/libsys/auxv.c
> +++ b/lib/libsys/auxv.c
> @@ -31,6 +31,7 @@
> #include <errno.h>
> #include <link.h>
> #include <pthread.h>
> +#include <stdbool.h>
> #include <string.h>
> #include <sys/auxv.h>
> #include "un-namespace.h"
> @@ -40,6 +41,8 @@ extern int _DYNAMIC;
> #pragma weak _DYNAMIC
> 
> void *__elf_aux_vector;
> +
> +#ifndef PIC
> static pthread_once_t aux_vector_once = PTHREAD_ONCE_INIT;
> 
> static void
> @@ -61,8 +64,9 @@ __init_elf_aux_vector(void)
> return;
> _once(&aux_vector_once, init_aux_vector_once);
> }
> +#endif
> 
> -static pthread_once_t aux_once = PTHREAD_ONCE_INIT;
> +static bool aux_once = false;
> static int pagesize, osreldate, canary_len, ncpus, pagesizes_len, bsdflags;
> static int hwcap_present, hwcap2_present;
> static char *canary, *pagesizes, *execpath;
> @@ -77,11 +81,19 @@ static void _init_aux_powerpc_fixup(void);
> int _powerpc_elf_aux_info(int, void *, int);
> #endif
> 
> +/*
> + * This function might be called and actual body executed more than
> + * once in multithreading environment.  Due to this, it is and must
> + * continue to be idempotent.  All stores are atomic (no store
> + * tearing), because we only assign to int/long/ptr.
> + */
> static void
> init_aux(void)
> {
> Elf_Auxinfo *aux;
> 
> + if (aux_once)
> + return;
> for (aux = __elf_aux_vector; aux->a_type != AT_NULL; aux++) {
> switch (aux->a_type) {
> case AT_BSDFLAGS:
> @@ -166,6 +178,8 @@ init_aux(void)
> if (!powerpc_new_auxv_format)
> _init_aux_powerpc_fixup();
> #endif
> +
> + aux_once = true;
> }
> 
> #ifdef __powerpc__
> @@ -256,10 +270,12 @@ _elf_aux_info(int aux, void *buf, int buflen)
> {
> int res;
> 
> +#ifndef PIC
> __init_elf_aux_vector();
> +#endif
> if (__elf_aux_vector == NULL)
> return (ENOSYS);
> - _once(&aux_once, init_aux);
> + init_aux(); /* idempotent */
> 
> if (buflen < 0)
> return (EINVAL);