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

[Michael Jeanson]

On 2024-09-14 13:51, Florian Weimer wrote:
> * 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 ()

Ack.

> 
> 
>> 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.

Ack.

> 
>> 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.

Ack, I'll add both.

> 
>> +/* 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).

Ack.

> 
> 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.

Ack.

> 
>> +      /* 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.

Ack.

> 
> 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.

Ack, I'll split the memset in a separate patch. I wonder if instead of a memset
of the whole area, we should explicitly clear the fields that are required to
be by the syscall?

> 
>> 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.

Ack.

> 
>>  
>>  /* 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).

Ack.

> 
>> --- 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?

Yes this is correct.

> 
>>  #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
>