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

  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(-)
  

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()
+}
  

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
  

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.
  

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.
  

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.