[PATCH v12 15/17] nptl: Move the rseq area to the 'extra TLS' block

[Florian Weimer]

* Michael Jeanson:

> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 1d3665d5ed..9b49ee7121 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -691,7 +691,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
>  
>    /* Inherit rseq registration state.  Without seccomp filters, rseq
>       registration will either always fail or always succeed.  */
> -  if ((int) THREAD_GETMEM_VOLATILE (self, rseq_area.cpu_id) >= 0)
> +  if ((int) RSEQ_GETMEM_VOLATILE (rseq_get_area(), cpu_id) >= 0)
>      pd->flags |= ATTR_FLAG_DO_RSEQ;

Style nit: space in rseq_get_area ()


> diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
> index b2abb2762d..46dc666cbc 100644
> --- a/sysdeps/nptl/dl-tls_init_tp.c
> +++ b/sysdeps/nptl/dl-tls_init_tp.c
> @@ -101,19 +101,17 @@ __tls_init_tp (void)
>    }

> +    if (!rseq_register_current_thread (pd, do_rseq))
> +      {
> +        _rseq_size = 0;
> +      }

Maybe drop the extra braces.

> diff --git a/sysdeps/unix/sysv/linux/rseq-internal.h b/sysdeps/unix/sysv/linux/rseq-internal.h
> index 2aae447b60..d26d69fd6a 100644
> --- a/sysdeps/unix/sysv/linux/rseq-internal.h
> +++ b/sysdeps/unix/sysv/linux/rseq-internal.h
> @@ -24,6 +24,26 @@

> +/* rseq area registered with the kernel.  Use a custom definition here to
> +   isolate from the system provided header which could lack some fields of the
> +   Extended ABI.
> +
> +   Access to fields of the Extended ABI beyond the 20 bytes of the original ABI
> +   (after 'flags') must be gated by a check of the feature size.  */
> +struct rseq_area
> +{
> +  /* Original ABI.  */
> +  uint32_t cpu_id_start;
> +  uint32_t cpu_id;
> +  uint64_t rseq_cs;
> +  uint32_t flags;
> +  /* Extended ABI.  */
> +  uint32_t node_id;
> +  uint32_t mm_cid;
> +};

(Note: UAPI uses struct rseq, so no conflict here.)

It's architecture-dependent whether this struct has 4 byte padding at
the end.  If this struct is only used to get the field offsets and
sizes, please add a comment to this effect, and maybe a flexible array
member at the end to discourage direct object allocations.

> +/* Returns a pointer to the current thread rseq area.  */
> +static inline struct rseq_area *
> +rseq_get_area(void)
> +{
> +#if IS_IN (rtld)
> +  /* Use the hidden symbol in ld.so.  */
> +  return (struct rseq_area *) ((char *) __thread_pointer() + _rseq_offset);
> +#else
> +  return (struct rseq_area *) ((char *) __thread_pointer() + __rseq_offset);
> +#endif
> +}

I wonder if this should go into the patch that adds RSEQ_GETMEM_VOLATILE
etc. because it's required by the generic macros.  And maybe name it
RSEQ_SELF for alignment with THREAD_SELF (but it can remain an inline
function).

The _rseq_offset/__rseq_offset gate here is ugly.  I believe we can
avoid it for read access if we also add _GI___rseq_size as an alias in
the .S file and add rtld_hidden_proto to the declaration of __rseq_size.
Then in ld.so, we automatically use a hidden symbol for access with
__rseq_size, and we won't need the #ifdefs.  The _rseq_size alias will
have to remain because it's not defined as const and used during
initialization.

> +      /* The feature size can be smaller than the minimum rseq area size of 32
> +	 bytes accepted by the syscall, if this is the case, bump the size of
> +	 the registration to the minimum. The 'extra TLS' block is always at
> +	 least 32 bytes. */
>        if (size < RSEQ_AREA_SIZE_INITIAL)
> -        /* The initial implementation used only 20 bytes out of 32,
> -           but still expected size 32.  */
>          size = RSEQ_AREA_SIZE_INITIAL;
> -      int ret = INTERNAL_SYSCALL_CALL (rseq, &self->rseq_area,
> -                                       size, 0, RSEQ_SIG);
> +
> +      /* The kernel expects 'rseq_area->rseq_cs == NULL' on registration, zero
> +         the whole rseq area.  */
> +      memset(rseq_get_area(), 0, size);
> +
> +      int ret = INTERNAL_SYSCALL_CALL (rseq, rseq_get_area(), size, 0,
> +		      RSEQ_SIG);
>        if (!INTERNAL_SYSCALL_ERROR_P (ret))
>          return true;

Style nits: missing spaces before '(' in function macro/calls.

This added memset may actually fix an existing bug, but it is hard to
tell.  On some architectures, the generic TLS code in combination with
the NPTL code zeroes struct pthread, and thus the rseq area embedded in
it.  But I'm not sure that this is what happens on all architectures.
Perhaps we should add a test for this that exits are thread with a
pollutted rseq area.  In any case, the addition of the memset should go
into a separate patch, so that we can backport it to all releases with
rseq support.

> diff --git a/sysdeps/unix/sysv/linux/tst-rseq-disable.c b/sysdeps/unix/sysv/linux/tst-rseq-disable.c
> index bbc655bec4..b1f4e894f1 100644
> --- a/sysdeps/unix/sysv/linux/tst-rseq-disable.c
> +++ b/sysdeps/unix/sysv/linux/tst-rseq-disable.c
> @@ -26,32 +26,65 @@

> +static __thread struct rseq local_rseq = {
> +  .cpu_id = RSEQ_CPU_ID_REGISTRATION_FAILED,
> +};

Maybe add a comment: Used to test private registration with the rseq
system call because glibc rseq is disabled.

>  
>  /* Check that rseq can be registered and has not been taken by glibc.  */
>  static void
>  check_rseq_disabled (void)
>  {
> -  struct pthread *pd = THREAD_SELF;
> +  struct rseq *rseq_area = (struct rseq *) ((char *) __thread_pointer () + __rseq_offset);

Style nit: Line length (also below).

> --- a/sysdeps/unix/sysv/linux/tst-rseq.c
> +++ b/sysdeps/unix/sysv/linux/tst-rseq.c
> @@ -19,6 +19,8 @@
>     not linked against libpthread.  */
>  
>  #include <support/check.h>
> +#include <support/namespace.h>
> +#include <support/xthread.h>
>  #include <stdio.h>
>  #include <sys/rseq.h>
>  #include <unistd.h>
> @@ -32,23 +34,66 @@
>  # include <sys/auxv.h>
>  # include <thread_pointer.h>
>  # include <tls.h>
> +# include <sys/auxv.h>
>  # include "tst-rseq.h"
>  
>  static void
>  do_rseq_main_test (void)
>  {
> -  struct pthread *pd = THREAD_SELF;
> +  size_t rseq_align = MAX (getauxval (AT_RSEQ_ALIGN), RSEQ_TEST_MIN_ALIGN);
> +  size_t rseq_feature_size = MAX (getauxval (AT_RSEQ_FEATURE_SIZE), RSEQ_TEST_MIN_FEATURE_SIZE);
> +  size_t rseq_alloc_size = roundup (MAX (rseq_feature_size, RSEQ_TEST_MIN_SIZE), rseq_align);

I think this assumes that getauxval returns 0 if the kernel doesn't have
these in auxv?

>  #include <support/test-driver.c>
> diff --git a/sysdeps/unix/sysv/linux/tst-rseq.h b/sysdeps/unix/sysv/linux/tst-rseq.h
> index dc603327d3..7a2e19b07f 100644
> --- a/sysdeps/unix/sysv/linux/tst-rseq.h
> +++ b/sysdeps/unix/sysv/linux/tst-rseq.h
> @@ -23,11 +23,16 @@
>  #include <syscall.h>
>  #include <sys/rseq.h>
>  #include <tls.h>
> +#include <rseq-internal.h>

Note: This obtains the struct rseq_area definition (the glibc variant of
the UAPI struct rseq).

Thanks,
Florian