Re: [PATCH] nptl: fix potential merge of __rseq_* relro symbols

["H.J. Lu" <hjl.tools@gmail.com>]

On Mon, Jan 15, 2024 at 1:10 PM Michael Jeanson <mjeanson@efficios.com> wrote:
>
> While working on a patch to add support for the extensible rseq ABI, we
> came across an issue where a new 'const' variable would be merged with
> the existing '__rseq_size' variable. We tracked this to the use of
> '-fmerge-all-constants' which allows the compiler to merge identical
> constant variables. This means that all 'const' variables in a compile
> unit that are of the same size and are initialized to the same value can
> be merged.
>
> In this specific case, on 32 bit systems 'unsigned int' and 'ptrdiff_t'
> are both 4 bytes and initialized to 0 which should trigger the merge.
> However for reasons we haven't delved into when the attribute 'section
> (".data.rel.ro")' is added to the mix, only variables of the same exact
> types are merged. As far as we know this behavior is not specified
> anywhere and could change with a new compiler version, hence this patch.
>
> Declare both variables as regular static variables and add 'extern
> const' aliases for the ABI. This has the added bonus of removing the asm
> workaround to set the values on rseq registration.
>
> Tested on Debian 12 with GCC 12.2.
>
> Signed-off-by: Michael Jeanson <mjeanson@efficios.com>
> Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
>  sysdeps/nptl/dl-tls_init_tp.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
> index 092c274f36..7f66f6de32 100644
> --- a/sysdeps/nptl/dl-tls_init_tp.c
> +++ b/sysdeps/nptl/dl-tls_init_tp.c
> @@ -45,8 +45,15 @@ rtld_mutex_dummy (pthread_mutex_t *lock)
>  #endif
>
>  const unsigned int __rseq_flags;
> -const unsigned int __rseq_size attribute_relro;
> -const ptrdiff_t __rseq_offset attribute_relro;
> +
> +/* Due to the use of '-fmerge-all-constants' the compiler is allowed to merge
> +   all 'const' variables of the same size that are initialized to the same
> +   value.  To work around this, declare them as regular static variable and use
> +   an alias that is 'const'.  */
> +static unsigned int rseq_size attribute_relro;
> +extern const unsigned int __attribute__ ((alias ("rseq_size"))) __rseq_size;
> +static ptrdiff_t rseq_offset attribute_relro;
> +extern const ptrdiff_t __attribute__ ((alias ("rseq_offset"))) __rseq_offset;

Don't you want to mark them as hidden if they are internal to glibc?

>  void
>  __tls_pre_init_tp (void)
> @@ -105,10 +112,8 @@ __tls_init_tp (void)
>      do_rseq = TUNABLE_GET (rseq, int, NULL);
>      if (rseq_register_current_thread (pd, do_rseq))
>        {
> -        /* We need a writable view of the variables.  They are in
> -           .data.relro and are not yet write-protected.  */
> -        extern unsigned int size __asm__ ("__rseq_size");
> -        size = sizeof (pd->rseq_area);
> +       /* The variables are in .data.relro but are not yet write-protected.  */
> +        rseq_size = sizeof (pd->rseq_area);
>        }
>
>  #ifdef RSEQ_SIG
> @@ -117,8 +122,7 @@ __tls_init_tp (void)
>         all targets support __thread_pointer, so set __rseq_offset only
>         if the rseq registration may have happened because RSEQ_SIG is
>         defined.  */
> -    extern ptrdiff_t offset __asm__ ("__rseq_offset");
> -    offset = (char *) &pd->rseq_area - (char *) __thread_pointer ();
> +    rseq_offset = (char *) &pd->rseq_area - (char *) __thread_pointer ();
>  #endif
>    }
>
> --
> 2.34.1
>


-- 
H.J.