[PATCH v2 0/4] fix ifunc with static pie [BZ #27072]

[PATCH v2 0/4] fix ifunc with static pie [BZ #27072]

[Szabolcs Nagy]

v2:
- check PI_STATIC_AND_HIDDEN for --enable-static-pie
- change string buffer sizes in the tunables
- fix env_alias == NULL logic in __tunables_init
- move __ehdr_start processing after self relocation

I think this is in a reasonable shape now, but there are still
some issues:
- tunables try to allocate memory (tunable_strdup) even if
  that's not necessary: only setuid binaries need this (in
  case there is a TUNABLE_SECLEVEL_SXID_ERASE tunable).
  this adds a lot of complexity and a failure path to the early
  init code. i think that if there is any such tunable then the
  entire GLIBC_TUNABLE= should just be dropped.
- tunable strings could be stored more compactly (and without
  arbitrary size limits) as an optimization. (can be done once
  there are too many tunables.)
- all symbols are forced hidden in libc.a, but i think lib*.a
  should do the same. (other than lib*_nonshared.a)

Szabolcs Nagy (4):
  configure: Require PI_STATIC_AND_HIDDEN for static pie
  Make libc symbols hidden in static PIE
  elf: Avoid RELATIVE relocs in __tunables_init
  csu: Move static pie self relocation later [BZ #27072]

 configure                | 14 +++++++++++++
 configure.ac             |  5 +++++
 csu/libc-start.c         | 44 +++++++++++++++++++++++-----------------
 elf/dl-tunables.c        |  2 +-
 elf/dl-tunables.h        |  4 ++--
 include/libc-symbols.h   |  8 ++++++--
 scripts/gen-tunables.awk |  2 +-
 7 files changed, 54 insertions(+), 25 deletions(-)

-- 
2.17.1

[PATCH v2 1/4] configure: Require PI_STATIC_AND_HIDDEN for static pie

[Szabolcs Nagy]

The glibc static pie self relocation code relies on that local
symbols can be accessed without dynamic relocations in position
independent code.
---
 configure    | 14 ++++++++++++++
 configure.ac |  5 +++++
 2 files changed, 19 insertions(+)

diff --git a/configure b/configure
index 6a35553805..e9d88f007c 100755
--- a/configure
+++ b/configure
@@ -6837,6 +6837,20 @@ if test "$static_pie" = yes; then
   if test "$libc_cv_no_dynamic_linker" != yes; then
     as_fn_error $? "linker support for --no-dynamic-linker needed" "$LINENO" 5
   fi
+
+  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+#ifndef PI_STATIC_AND_HIDDEN
+# error static pie depends on PI_STATIC_AND_HIDDEN
+#endif
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+
+else
+  as_fn_error $? "the target does not support static pie" "$LINENO" 5
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+
   # Default to PIE.
   libc_cv_pie_default=yes
   $as_echo "#define ENABLE_STATIC_PIE 1" >>confdefs.h
diff --git a/configure.ac b/configure.ac
index 43cfac9d48..aead4c44d7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1846,6 +1846,11 @@ if test "$static_pie" = yes; then
   if test "$libc_cv_no_dynamic_linker" != yes; then
     AC_MSG_ERROR([linker support for --no-dynamic-linker needed])
   fi
+
+  AC_COMPILE_IFELSE([AC_LANG_SOURCE([[#ifndef PI_STATIC_AND_HIDDEN
+# error static pie depends on PI_STATIC_AND_HIDDEN
+#endif]])], , AC_MSG_ERROR([the target does not support static pie]))
+
   # Default to PIE.
   libc_cv_pie_default=yes
   AC_DEFINE(ENABLE_STATIC_PIE)
-- 
2.17.1

[PATCH v2 2/4] Make libc symbols hidden in static PIE

[Szabolcs Nagy]

Hidden matters with static PIE: extern symbol access in position
independent code usually involves GOT indirections which needs
RELATIVE relocs in a static linked PIE. Using So hidden avoids
indirections and RELATIVE relocs on targets that can access symbols
pc-relative.

The check should use IS_IN_LIB instead of IS_IN(libc) since all
static libraries can use hidden visibility to avoid indirections,
however the test system links objects from libcrypt.a into dynamic
linked test binaries so hidden does not work there.  I think mixing
static and shared libc components in the same binary should not be
supported usage, but to be safe only use hidden in libc.a.

From -static-pie linked 'int main(){}' this shaves off 73 relative
relocs on aarch64 and reduces code size too.
---
 include/libc-symbols.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/libc-symbols.h b/include/libc-symbols.h
index ea126ae70c..93e63ee889 100644
--- a/include/libc-symbols.h
+++ b/include/libc-symbols.h
@@ -434,13 +434,17 @@ for linking")
   strong_alias(real, name)
 #endif
 
-#if defined SHARED || defined LIBC_NONSHARED \
-  || (BUILD_PIE_DEFAULT && IS_IN (libc))
+#if defined SHARED || defined LIBC_NONSHARED
 # define attribute_hidden __attribute__ ((visibility ("hidden")))
 #else
 # define attribute_hidden
 #endif
 
+/* Mark all symbols hidden in static PIE libc to avoid GOT indirections.  */
+#if BUILD_PIE_DEFAULT && IS_IN (libc) && !defined LIBC_NONSHARED
+# pragma GCC visibility push(hidden)
+#endif
+
 #define attribute_tls_model_ie __attribute__ ((tls_model ("initial-exec")))
 
 #define attribute_relro __attribute__ ((section (".data.rel.ro")))
-- 
2.17.1

[PATCH v2 3/4] elf: Avoid RELATIVE relocs in __tunables_init

[Szabolcs Nagy]

With static pie linking pointers in the tunables list need
RELATIVE relocs since the absolute address is not known at link
time. We want to avoid relocations so the static pie self
relocation can be done after tunables are initialized.

This is a quick fix that increases the tunable list size a bit.
---
 elf/dl-tunables.c        | 2 +-
 elf/dl-tunables.h        | 4 ++--
 scripts/gen-tunables.awk | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 9b4d737fb8..3845b2c04e 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -350,7 +350,7 @@ __tunables_init (char **envp)
 
 	  /* Skip over tunables that have either been set already or should be
 	     skipped.  */
-	  if (cur->initialized || cur->env_alias == NULL)
+	  if (cur->initialized || cur->env_alias[0] == '\0')
 	    continue;
 
 	  const char *name = cur->env_alias;
diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
index 518342a300..05997d028a 100644
--- a/elf/dl-tunables.h
+++ b/elf/dl-tunables.h
@@ -38,7 +38,7 @@ __tunables_init (char **unused __attribute__ ((unused)))
 /* A tunable.  */
 struct _tunable
 {
-  const char *name;			/* Internal name of the tunable.  */
+  const char name[64];			/* Internal name of the tunable.  */
   tunable_type_t type;			/* Data type of the tunable.  */
   tunable_val_t val;			/* The value.  */
   bool initialized;			/* Flag to indicate that the tunable is
@@ -54,7 +54,7 @@ struct _tunable
 					   target module if the value is
 					   considered unsafe.  */
   /* Compatibility elements.  */
-  const char *env_alias;		/* The compatibility environment
+  const char env_alias[24];		/* The compatibility environment
 					   variable name.  */
 };
 
diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
index 622199061a..9e7bd24e13 100644
--- a/scripts/gen-tunables.awk
+++ b/scripts/gen-tunables.awk
@@ -57,7 +57,7 @@ $1 == "}" {
       maxvals[top_ns,ns,tunable] = max_of[types[top_ns,ns,tunable]]
     }
     if (!env_alias[top_ns,ns,tunable]) {
-      env_alias[top_ns,ns,tunable] = "NULL"
+      env_alias[top_ns,ns,tunable] = "{0}"
     }
     if (!security_level[top_ns,ns,tunable]) {
       security_level[top_ns,ns,tunable] = "SXID_ERASE"
-- 
2.17.1

[PATCH v2 4/4] csu: Move static pie self relocation later [BZ #27072]

[Szabolcs Nagy]

IFUNC resolvers may depend on tunables and cpu feature setup so
move static pie self relocation after those.

It is hard to guarantee that the ealy startup code does not rely
on relocations so this is a bit fragile. It would be more robust
to handle RELATIVE relocs early and only IRELATIVE relocs later,
but the current relocation processing code cannot do that.

The early startup code before relocation processing includes

  _dl_aux_init (auxvec);
  __libc_init_secure ();
  __tunables_init (__environ);
  ARCH_INIT_CPU_FEATURES ();

These are simple enough that RELATIVE relocs can be avoided.

__ehdr_start may require RELATIVE relocation so it was moved
later, fortunately ehdr and phdr are not used in the early code.
---
 csu/libc-start.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/csu/libc-start.c b/csu/libc-start.c
index db859c3bed..fb64cdb2c9 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -142,8 +142,6 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
   int result;
 
 #ifndef SHARED
-  _dl_relocate_static_pie ();
-
   char **ev = &argv[argc + 1];
 
   __environ = ev;
@@ -165,24 +163,7 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
   }
 #  endif
   _dl_aux_init (auxvec);
-  if (GL(dl_phdr) == NULL)
 # endif
-    {
-      /* Starting from binutils-2.23, the linker will define the
-         magic symbol __ehdr_start to point to our own ELF header
-         if it is visible in a segment that also includes the phdrs.
-         So we can set up _dl_phdr and _dl_phnum even without any
-         information from auxv.  */
-
-      extern const ElfW(Ehdr) __ehdr_start
-	__attribute__ ((weak, visibility ("hidden")));
-      if (&__ehdr_start != NULL)
-        {
-          assert (__ehdr_start.e_phentsize == sizeof *GL(dl_phdr));
-          GL(dl_phdr) = (const void *) &__ehdr_start + __ehdr_start.e_phoff;
-          GL(dl_phnum) = __ehdr_start.e_phnum;
-        }
-    }
 
   /* Initialize very early so that tunables can use it.  */
   __libc_init_secure ();
@@ -191,6 +172,11 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
 
   ARCH_INIT_CPU_FEATURES ();
 
+  /* Do static pie self relocation after tunables and cpu features
+     are setup for ifunc resolvers. Before this point relocations
+     must be avoided.  */
+  _dl_relocate_static_pie ();
+
   /* Perform IREL{,A} relocations.  */
   ARCH_SETUP_IREL ();
 
@@ -202,6 +188,26 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
      hwcap and platform fields available in the TCB.  */
   ARCH_APPLY_IREL ();
 
+# ifdef HAVE_AUX_VECTOR
+  if (GL(dl_phdr) == NULL)
+# endif
+    {
+      /* Starting from binutils-2.23, the linker will define the
+         magic symbol __ehdr_start to point to our own ELF header
+         if it is visible in a segment that also includes the phdrs.
+         So we can set up _dl_phdr and _dl_phnum even without any
+         information from auxv.  */
+
+      extern const ElfW(Ehdr) __ehdr_start
+	__attribute__ ((weak, visibility ("hidden")));
+      if (&__ehdr_start != NULL)
+        {
+          assert (__ehdr_start.e_phentsize == sizeof *GL(dl_phdr));
+          GL(dl_phdr) = (const void *) &__ehdr_start + __ehdr_start.e_phoff;
+          GL(dl_phnum) = __ehdr_start.e_phnum;
+        }
+    }
+
   /* Set up the stack checker's canary.  */
   uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
 # ifdef THREAD_SET_STACK_GUARD
-- 
2.17.1

Re: [PATCH v2 0/4] fix ifunc with static pie [BZ #27072]

[H.J. Lu]

On Fri, Jan 8, 2021 at 8:22 AM Szabolcs Nagy via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> v2:
> - check PI_STATIC_AND_HIDDEN for --enable-static-pie
> - change string buffer sizes in the tunables
> - fix env_alias == NULL logic in __tunables_init
> - move __ehdr_start processing after self relocation
>
> I think this is in a reasonable shape now, but there are still
> some issues:
> - tunables try to allocate memory (tunable_strdup) even if
>   that's not necessary: only setuid binaries need this (in
>   case there is a TUNABLE_SECLEVEL_SXID_ERASE tunable).
>   this adds a lot of complexity and a failure path to the early
>   init code. i think that if there is any such tunable then the
>   entire GLIBC_TUNABLE= should just be dropped.
> - tunable strings could be stored more compactly (and without
>   arbitrary size limits) as an optimization. (can be done once
>   there are too many tunables.)
> - all symbols are forced hidden in libc.a, but i think lib*.a
>   should do the same. (other than lib*_nonshared.a)
>
> Szabolcs Nagy (4):
>   configure: Require PI_STATIC_AND_HIDDEN for static pie
>   Make libc symbols hidden in static PIE
>   elf: Avoid RELATIVE relocs in __tunables_init
>   csu: Move static pie self relocation later [BZ #27072]
>

Can you push your patches into a branch? I'd like to add an x86 test
on top of your patches.

Thanks.

-- 
H.J.

Re: [PATCH v2 3/4] elf: Avoid RELATIVE relocs in __tunables_init

[Adhemerval Zanella]

On 08/01/2021 13:20, Szabolcs Nagy via Libc-alpha wrote:
> With static pie linking pointers in the tunables list need
> RELATIVE relocs since the absolute address is not known at link
> time. We want to avoid relocations so the static pie self
> relocation can be done after tunables are initialized.
> 
> This is a quick fix that increases the tunable list size a bit.
> ---
>  elf/dl-tunables.c        | 2 +-
>  elf/dl-tunables.h        | 4 ++--
>  scripts/gen-tunables.awk | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 9b4d737fb8..3845b2c04e 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -350,7 +350,7 @@ __tunables_init (char **envp)
>  
>  	  /* Skip over tunables that have either been set already or should be
>  	     skipped.  */
> -	  if (cur->initialized || cur->env_alias == NULL)
> +	  if (cur->initialized || cur->env_alias[0] == '\0')
>  	    continue;
>  
>  	  const char *name = cur->env_alias;
> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
> index 518342a300..05997d028a 100644
> --- a/elf/dl-tunables.h
> +++ b/elf/dl-tunables.h
> @@ -38,7 +38,7 @@ __tunables_init (char **unused __attribute__ ((unused)))
>  /* A tunable.  */
>  struct _tunable
>  {
> -  const char *name;			/* Internal name of the tunable.  */
> +  const char name[64];			/* Internal name of the tunable.  */
>    tunable_type_t type;			/* Data type of the tunable.  */
>    tunable_val_t val;			/* The value.  */
>    bool initialized;			/* Flag to indicate that the tunable is
> @@ -54,7 +54,7 @@ struct _tunable
>  					   target module if the value is
>  					   considered unsafe.  */
>    /* Compatibility elements.  */
> -  const char *env_alias;		/* The compatibility environment
> +  const char env_alias[24];		/* The compatibility environment
>  					   variable name.  */
>  };
>  
> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> index 622199061a..9e7bd24e13 100644
> --- a/scripts/gen-tunables.awk
> +++ b/scripts/gen-tunables.awk
> @@ -57,7 +57,7 @@ $1 == "}" {
>        maxvals[top_ns,ns,tunable] = max_of[types[top_ns,ns,tunable]]
>      }
>      if (!env_alias[top_ns,ns,tunable]) {
> -      env_alias[top_ns,ns,tunable] = "NULL"
> +      env_alias[top_ns,ns,tunable] = "{0}"
>      }
>      if (!security_level[top_ns,ns,tunable]) {
>        security_level[top_ns,ns,tunable] = "SXID_ERASE"
> 

The change is ok, although I think we can at least not make the maximum r
equired size being automatically generated at build time:

diff --git a/Makeconfig b/Makeconfig
index 0a4811b5e5..a291f90719 100644
--- a/Makeconfig
+++ b/Makeconfig
@@ -1160,8 +1160,10 @@ endif
 # Build the tunables list header early since it could be used by any module in
 # glibc.
 ifneq (no,$(have-tunables))
-before-compile += $(common-objpfx)dl-tunable-list.h
-common-generated += dl-tunable-list.h dl-tunable-list.stmp
+before-compile += $(common-objpfx)dl-tunable-list.h \
+		  $(common-objpfx)dl-tunable-list-max.h
+common-generated += dl-tunable-list.h dl-tunable-list.stmp \
+		    dl-tunable-list-max.h dl-tunable-list-max.stmp \
 
 $(common-objpfx)dl-tunable-list.h: $(common-objpfx)dl-tunable-list.stmp; @:
 $(common-objpfx)dl-tunable-list.stmp: \
@@ -1169,7 +1171,17 @@ $(common-objpfx)dl-tunable-list.stmp: \
 		$(..)elf/dl-tunables.list \
 		$(wildcard $(subdirs:%=$(..)%/dl-tunables.list)) \
 		$(wildcard $(sysdirs:%=%/dl-tunables.list))
-	$(AWK) -f $^ > ${@:stmp=T}
+	$(AWK) -v mode=tunables -f $^ > ${@:stmp=T}
+	$(move-if-change) ${@:stmp=T} ${@:stmp=h}
+	touch $@
+
+$(common-objpfx)dl-tunable-list-max.h: $(common-objpfx)dl-tunable-list-max.stmp; @:
+$(common-objpfx)dl-tunable-list-max.stmp: \
+		$(..)scripts/gen-tunables.awk \
+		$(..)elf/dl-tunables.list \
+		$(wildcard $(subdirs:%=$(..)%/dl-tunables.list)) \
+		$(wildcard $(sysdirs:%=%/dl-tunables.list))
+	$(AWK) -v mode=max -f $^ > ${@:stmp=T}
 	$(move-if-change) ${@:stmp=T} ${@:stmp=h}
 	touch $@
 endif
diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
index 518342a300..daeb6cf115 100644
--- a/elf/dl-tunables.h
+++ b/elf/dl-tunables.h
@@ -34,11 +34,12 @@ __tunables_init (char **unused __attribute__ ((unused)))
 
 # include <stddef.h>
 # include "dl-tunable-types.h"
+# include "dl-tunable-list-max.h"
 
 /* A tunable.  */
 struct _tunable
 {
-  const char *name;			/* Internal name of the tunable.  */
+  const char name[TUNABLES_NAME_MAX];	/* Internal name of the tunable.  */
   tunable_type_t type;			/* Data type of the tunable.  */
   tunable_val_t val;			/* The value.  */
   bool initialized;			/* Flag to indicate that the tunable is
@@ -54,7 +55,7 @@ struct _tunable
 					   target module if the value is
 					   considered unsafe.  */
   /* Compatibility elements.  */
-  const char *env_alias;		/* The compatibility environment
+  const char env_alias[TUNABLES_ALIAS_MAX];/* The compatibility environment
 					   variable name.  */
 };
 
diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
index 622199061a..a4174b61f5 100644
--- a/scripts/gen-tunables.awk
+++ b/scripts/gen-tunables.awk
@@ -12,6 +12,8 @@ BEGIN {
   tunable=""
   ns=""
   top_ns=""
+  max_name_len=0
+  max_alias_len=0
 }
 
 # Skip over blank lines and comments.
@@ -46,6 +48,14 @@ $2 == "{" {
 # End of either a top namespace, tunable namespace or a tunable.
 $1 == "}" {
   if (tunable != "") {
+    name_len = length(top_ns"."ns"."tunable)
+    if (name_len > max_name_len)
+      max_name_len = name_len
+
+    alias_len = length(env_alias[top_ns,ns,tunable])
+    if (alias_len > max_alias_len)
+      max_alias_len = alias_len
+
     # Tunables definition ended, now fill in default attributes.
     if (!types[top_ns,ns,tunable]) {
       types[top_ns,ns,tunable] = "STRING"
@@ -57,7 +67,7 @@ $1 == "}" {
       maxvals[top_ns,ns,tunable] = max_of[types[top_ns,ns,tunable]]
     }
     if (!env_alias[top_ns,ns,tunable]) {
-      env_alias[top_ns,ns,tunable] = "NULL"
+      env_alias[top_ns,ns,tunable] = "{0}"
     }
     if (!security_level[top_ns,ns,tunable]) {
       security_level[top_ns,ns,tunable] = "SXID_ERASE"
@@ -131,7 +141,7 @@ $1 == "}" {
   }
 }
 
-END {
+function print_tunables() {
   if (ns != "") {
     print "Unterminated namespace.  Is a closing brace missing?"
     exit 1
@@ -172,3 +182,20 @@ END {
   print "};"
   print "#endif"
 }
+
+function print_name_max() {
+  print "/* AUTOGENERATED by gen-tunables.awk.  */"
+  print "#ifndef _TUNABLES_H_"
+  print "# error \"Do not include this file directly.\""
+  print "# error \"Include tunables.h instead.\""
+  print "#endif"
+  printf ("#define TUNABLES_NAME_MAX %d\n", max_name_len)
+  printf ("#define TUNABLES_ALIAS_MAX %d\n", max_alias_len)
+}
+
+END {
+  if (mode == "tunables")
+    print_tunables()
+  else (mode == "max")
+    print_name_max()
+}

Re: [PATCH v2 3/4] elf: Avoid RELATIVE relocs in __tunables_init

[Siddhesh Poyarekar]

On 1/8/21 11:29 PM, Adhemerval Zanella via Libc-alpha wrote:
>> @@ -38,7 +38,7 @@ __tunables_init (char **unused __attribute__ ((unused)))
>>   /* A tunable.  */
>>   struct _tunable
>>   {
>> -  const char *name;			/* Internal name of the tunable.  */
>> +  const char name[64];			/* Internal name of the tunable.  */
>>     tunable_type_t type;			/* Data type of the tunable.  */
>>     tunable_val_t val;			/* The value.  */
>>     bool initialized;			/* Flag to indicate that the tunable is
>> @@ -54,7 +54,7 @@ struct _tunable
>>   					   target module if the value is
>>   					   considered unsafe.  */
>>     /* Compatibility elements.  */
>> -  const char *env_alias;		/* The compatibility environment
>> +  const char env_alias[24];		/* The compatibility environment
>>   					   variable name.  */
>>   };
>>   
<snip>
> The change is ok, although I think we can at least not make the maximum r
> equired size being automatically generated at build time:

I don't have a strong preference either way, but the nice thing about 
the above hardcoded sizes is that it makes the struct size 128 bytes, 
which means that the array lines up nicely with each element in a pair 
of cachelines on 64-bit architectures.

The code is not performance sensitive, so it's probably just an 
asthaetic thing :)

Siddhesh

Re: [PATCH v2 3/4] elf: Avoid RELATIVE relocs in __tunables_init

[Adhemerval Zanella]

On 08/01/2021 15:22, Siddhesh Poyarekar wrote:
> On 1/8/21 11:29 PM, Adhemerval Zanella via Libc-alpha wrote:
>>> @@ -38,7 +38,7 @@ __tunables_init (char **unused __attribute__ ((unused)))
>>>   /* A tunable.  */
>>>   struct _tunable
>>>   {
>>> -  const char *name;            /* Internal name of the tunable.  */
>>> +  const char name[64];            /* Internal name of the tunable.  */
>>>     tunable_type_t type;            /* Data type of the tunable.  */
>>>     tunable_val_t val;            /* The value.  */
>>>     bool initialized;            /* Flag to indicate that the tunable is
>>> @@ -54,7 +54,7 @@ struct _tunable
>>>                          target module if the value is
>>>                          considered unsafe.  */
>>>     /* Compatibility elements.  */
>>> -  const char *env_alias;        /* The compatibility environment
>>> +  const char env_alias[24];        /* The compatibility environment
>>>                          variable name.  */
>>>   };
>>>   
> <snip>
>> The change is ok, although I think we can at least not make the maximum r
>> equired size being automatically generated at build time:
> 
> I don't have a strong preference either way, but the nice thing about the above hardcoded sizes is that it makes the struct size 128 bytes, which means that the array lines up nicely with each element in a pair of cachelines on 64-bit architectures.
> 
> The code is not performance sensitive, so it's probably just an asthaetic thing :)

The struct is maked as read-only, so I doubt we will need to optimize
for cacheline to avoid false-sharing.  And coupling the size with the
tunable definitions itself is one less required change if tunables are
modified.

Re: [PATCH v2 0/4] fix ifunc with static pie [BZ #27072]

[Szabolcs Nagy]

The 01/08/2021 09:04, H.J. Lu wrote:
> On Fri, Jan 8, 2021 at 8:22 AM Szabolcs Nagy via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> > v2:
> > - check PI_STATIC_AND_HIDDEN for --enable-static-pie
> > - change string buffer sizes in the tunables
> > - fix env_alias == NULL logic in __tunables_init
> > - move __ehdr_start processing after self relocation
> >
> > I think this is in a reasonable shape now, but there are still
> > some issues:
> > - tunables try to allocate memory (tunable_strdup) even if
> >   that's not necessary: only setuid binaries need this (in
> >   case there is a TUNABLE_SECLEVEL_SXID_ERASE tunable).
> >   this adds a lot of complexity and a failure path to the early
> >   init code. i think that if there is any such tunable then the
> >   entire GLIBC_TUNABLE= should just be dropped.
> > - tunable strings could be stored more compactly (and without
> >   arbitrary size limits) as an optimization. (can be done once
> >   there are too many tunables.)
> > - all symbols are forced hidden in libc.a, but i think lib*.a
> >   should do the same. (other than lib*_nonshared.a)
> >
> > Szabolcs Nagy (4):
> >   configure: Require PI_STATIC_AND_HIDDEN for static pie
> >   Make libc symbols hidden in static PIE
> >   elf: Avoid RELATIVE relocs in __tunables_init
> >   csu: Move static pie self relocation later [BZ #27072]
> >
> 
> Can you push your patches into a branch? I'd like to add an x86 test
> on top of your patches.

i rebased and pushed them into nsz/bug27072

Re: [PATCH v2 3/4] elf: Avoid RELATIVE relocs in __tunables_init

[Szabolcs Nagy]

The 01/08/2021 14:59, Adhemerval Zanella wrote:
> On 08/01/2021 13:20, Szabolcs Nagy via Libc-alpha wrote:
> > With static pie linking pointers in the tunables list need
> > RELATIVE relocs since the absolute address is not known at link
> > time. We want to avoid relocations so the static pie self
> > relocation can be done after tunables are initialized.
> > 
> > This is a quick fix that increases the tunable list size a bit.
...
> > --- a/elf/dl-tunables.h
> > +++ b/elf/dl-tunables.h
> > @@ -38,7 +38,7 @@ __tunables_init (char **unused __attribute__ ((unused)))
> >  /* A tunable.  */
> >  struct _tunable
> >  {
> > -  const char *name;			/* Internal name of the tunable.  */
> > +  const char name[64];			/* Internal name of the tunable.  */
> >    tunable_type_t type;			/* Data type of the tunable.  */
> >    tunable_val_t val;			/* The value.  */
> >    bool initialized;			/* Flag to indicate that the tunable is
> > @@ -54,7 +54,7 @@ struct _tunable
> >  					   target module if the value is
> >  					   considered unsafe.  */
> >    /* Compatibility elements.  */
> > -  const char *env_alias;		/* The compatibility environment
> > +  const char env_alias[24];		/* The compatibility environment
> >  					   variable name.  */
> >  };
> 
> The change is ok, although I think we can at least not make the maximum r
> equired size being automatically generated at build time:
> 
...
> --- a/elf/dl-tunables.h
> +++ b/elf/dl-tunables.h
> @@ -34,11 +34,12 @@ __tunables_init (char **unused __attribute__ ((unused)))
>  
>  # include <stddef.h>
>  # include "dl-tunable-types.h"
> +# include "dl-tunable-list-max.h"
>  
>  /* A tunable.  */
>  struct _tunable
>  {
> -  const char *name;			/* Internal name of the tunable.  */
> +  const char name[TUNABLES_NAME_MAX];	/* Internal name of the tunable.  */
>    tunable_type_t type;			/* Data type of the tunable.  */
>    tunable_val_t val;			/* The value.  */
>    bool initialized;			/* Flag to indicate that the tunable is
> @@ -54,7 +55,7 @@ struct _tunable
>  					   target module if the value is
>  					   considered unsafe.  */
>    /* Compatibility elements.  */
> -  const char *env_alias;		/* The compatibility environment
> +  const char env_alias[TUNABLES_ALIAS_MAX];/* The compatibility environment
>  					   variable name.  */
>  };

yeah i can do this,
it's also possible to collect all strings
in one char array and iterate over them or
keep offsets to them in struct _tunable
instead of pointers.

Re: [PATCH v2 3/4] elf: Avoid RELATIVE relocs in __tunables_init

[Adhemerval Zanella]

On 11/01/2021 07:56, Szabolcs Nagy wrote:
> The 01/08/2021 14:59, Adhemerval Zanella wrote:
>> On 08/01/2021 13:20, Szabolcs Nagy via Libc-alpha wrote:
>>> With static pie linking pointers in the tunables list need
>>> RELATIVE relocs since the absolute address is not known at link
>>> time. We want to avoid relocations so the static pie self
>>> relocation can be done after tunables are initialized.
>>>
>>> This is a quick fix that increases the tunable list size a bit.
> ...
>>> --- a/elf/dl-tunables.h
>>> +++ b/elf/dl-tunables.h
>>> @@ -38,7 +38,7 @@ __tunables_init (char **unused __attribute__ ((unused)))
>>>  /* A tunable.  */
>>>  struct _tunable
>>>  {
>>> -  const char *name;			/* Internal name of the tunable.  */
>>> +  const char name[64];			/* Internal name of the tunable.  */
>>>    tunable_type_t type;			/* Data type of the tunable.  */
>>>    tunable_val_t val;			/* The value.  */
>>>    bool initialized;			/* Flag to indicate that the tunable is
>>> @@ -54,7 +54,7 @@ struct _tunable
>>>  					   target module if the value is
>>>  					   considered unsafe.  */
>>>    /* Compatibility elements.  */
>>> -  const char *env_alias;		/* The compatibility environment
>>> +  const char env_alias[24];		/* The compatibility environment
>>>  					   variable name.  */
>>>  };
>>
>> The change is ok, although I think we can at least not make the maximum r
>> equired size being automatically generated at build time:
>>
> ...
>> --- a/elf/dl-tunables.h
>> +++ b/elf/dl-tunables.h
>> @@ -34,11 +34,12 @@ __tunables_init (char **unused __attribute__ ((unused)))
>>  
>>  # include <stddef.h>
>>  # include "dl-tunable-types.h"
>> +# include "dl-tunable-list-max.h"
>>  
>>  /* A tunable.  */
>>  struct _tunable
>>  {
>> -  const char *name;			/* Internal name of the tunable.  */
>> +  const char name[TUNABLES_NAME_MAX];	/* Internal name of the tunable.  */
>>    tunable_type_t type;			/* Data type of the tunable.  */
>>    tunable_val_t val;			/* The value.  */
>>    bool initialized;			/* Flag to indicate that the tunable is
>> @@ -54,7 +55,7 @@ struct _tunable
>>  					   target module if the value is
>>  					   considered unsafe.  */
>>    /* Compatibility elements.  */
>> -  const char *env_alias;		/* The compatibility environment
>> +  const char env_alias[TUNABLES_ALIAS_MAX];/* The compatibility environment
>>  					   variable name.  */
>>  };
> 
> yeah i can do this,
> it's also possible to collect all strings
> in one char array and iterate over them or
> keep offsets to them in struct _tunable
> instead of pointers.
> 

Yes, this is pretty much what stdio-common/errlist.c does for errno names.