[PATCH v1.1 0/7] aarch64: Allow overriding HWCAP_CPUID feature check

[PATCH v1.1 0/7] aarch64: Allow overriding HWCAP_CPUID feature check

[Siddhesh Poyarekar]

Hi,

This set of patches moves LD_HWCAP_MASK environment variable to tunables
as glibc.tune.hwcap_mask and gets all of its uses using tunables instead.
This is needed to override the HWCAP_CPUID bit for aarch64 to allow
disabling of the multiarch MIDR check if needed.

The patchset enhances the tunables framework further for this and as a
result, the tunable_list is now laid out in the .relro section so that it
cannot be modified after relocations have been processed.  Also, tunables
can now be set to have a default value, and can also take unsigned int64
values.

I have done a sanity test of this patchset on aarch64 and seems to work
just fine.  x86 would perhaps benefit from a similar delaying of
cpu_features reading for static binaries so that they can be overriden by
tunables in future (an aarch64 patch coming up; spoiler alert: the tunable
name proposed is glibc.tune.mcpu) but I haven't done that at this time
since it is not necessary for this patchset.

Update from last set:

 - Fixed up removal of dl_hwcap_mask in one case I missed during rebase.

Siddhesh


Siddhesh Poyarekar (7):
  tunables: Specify a default value for tunables
  tunables: Add support for tunables of uint64_t type
  tunables: Add hooks to get and update tunables
  tunables: Add LD_HWCAP_MASK to tunables
  tunables: Use glibc.tune.hwcap_mask tunable instead of _dl_hwcap_mask
  Delay initialization of CPU features struct in static binaries
  aarch64: Allow overriding HWCAP_CPUID feature check using HWCAP_MASK

 csu/libc-start.c                               |   6 ++
 elf/dl-cache.c                                 |   9 +-
 elf/dl-hwcaps.c                                |  15 ++-
 elf/dl-support.c                               |   2 +
 elf/dl-tunable-types.h                         |   1 +
 elf/dl-tunables.c                              | 142 ++++++++++++++-----------
 elf/dl-tunables.h                              |  43 +++++---
 elf/dl-tunables.list                           |   7 ++
 elf/rtld.c                                     |   4 +
 scripts/gen-tunables.awk                       |  13 ++-
 sysdeps/generic/ldsodefs.h                     |   2 +
 sysdeps/sparc/sparc32/dl-machine.h             |   7 +-
 sysdeps/unix/sysv/linux/aarch64/cpu-features.c |  15 ++-
 sysdeps/unix/sysv/linux/aarch64/libc-start.c   |  23 +---
 sysdeps/x86/cpu-features.c                     |   4 +
 sysdeps/x86/libc-start.c                       |  23 +---
 16 files changed, 192 insertions(+), 124 deletions(-)

-- 
2.7.4

[PATCH 3/7] tunables: Add hooks to get and update tunables

[Siddhesh Poyarekar]

This change adds two new tunable macros to get and update tunables
respectively called TUNABLE_GET and TUNABLE_UPDATE_VAL.

TUNABLE_GET accepts the top namespace, tunable namespace and tunable
id along with the target type as arguments and returns a value of the
target type.  For example:

    TUNABLE_GET (glibc, malloc, check, int32_t)

will return the tunable value for glibc.malloc.check in an int32_t.

TUNABLE_UPDATE_VAL accepts the top namespace, tunable namespace and
tunable id along with a pointer to the value to be stored into the
tunable structure.  For example:

  int32_t val = 1;
  TUNABLE_UPDATE_VAL (glibc, malloc, check, &val);

will update the glibc.malloc.check tunable.

Given that the tunable structure is now relro, this is only really
possible from within the dynamic linker before it is relocated.  In
future the tunable list may get split into mutable and immutable
tunables where mutable tunables can be modified by the library and
userspace after relocation as well.  However whenever we actually do
that, we will have to ensure that the mutable tunables are protected
with locks.

	* elf/dl-tunables.h (strval): Replace with a bool initialized.
	(TUNABLE_ENUM_NAME, TUNABLE_ENUM_NAME1): Adjust names to
	prevent collision.
	(__tunable_update_val): New function.
	(TUNABLE_GET): New macro.
	(TUNABLE_SET_VAL_FULL_WITH_CALLBACK): New macro.
	(TUNABLE_SET_VAL): Use it.
	(TUNABLE_SET_VAL_WITH_CALLBACK): Likewise.
	(TUNABLE_UPDATE_VAL): New macro.
	(tunables_strtoul): Replace strval with initialized.
	(__tunables_init): Likewise.
	(__tunable_set_val): Likewise.
	(do_tunable_update_val): New function.
	(tunables_initialize): Use it.
	(__tunable_update_val): New function.
---
 elf/dl-tunables.c | 43 +++++++++++++++++++++++++++++++++++--------
 elf/dl-tunables.h | 41 +++++++++++++++++++++++++++--------------
 2 files changed, 62 insertions(+), 22 deletions(-)

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 8d72e26..abf10e5 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -184,19 +184,17 @@ tunables_strtoul (const char *nptr)
   if ((__type) (__val) >= min && (__type) (val) <= max)			      \
     {									      \
       (__cur)->val.numval = val;					      \
-      (__cur)->strval = strval;						      \
+      (__cur)->initialized = true;					      \
     }									      \
 })
 
-/* Validate range of the input value and initialize the tunable CUR if it looks
-   good.  */
 static void
-tunable_initialize (tunable_t *cur, const char *strval)
+do_tunable_update_val (tunable_t *cur, const void *valp)
 {
   uint64_t val;
 
   if (cur->type.type_code != TUNABLE_TYPE_STRING)
-    val = tunables_strtoul (strval);
+    val = *((int64_t *) valp);
 
   switch (cur->type.type_code)
     {
@@ -217,7 +215,7 @@ tunable_initialize (tunable_t *cur, const char *strval)
 	}
     case TUNABLE_TYPE_STRING:
 	{
-	  cur->val.strval = cur->strval = strval;
+	  cur->val.strval = valp;
 	  break;
 	}
     default:
@@ -225,6 +223,35 @@ tunable_initialize (tunable_t *cur, const char *strval)
     }
 }
 
+/* Validate range of the input value and initialize the tunable CUR if it looks
+   good.  */
+static void
+tunable_initialize (tunable_t *cur, const char *strval)
+{
+  uint64_t val;
+  const void *valp;
+
+  if (cur->type.type_code != TUNABLE_TYPE_STRING)
+    {
+      val = tunables_strtoul (strval);
+      valp = &val;
+    }
+  else
+    {
+      cur->initialized = true;
+      valp = strval;
+    }
+  do_tunable_update_val (cur, valp);
+}
+
+void
+__tunable_update_val (tunable_id_t id, void *valp)
+{
+  tunable_t *cur = &tunable_list[id];
+
+  do_tunable_update_val (cur, valp);
+}
+
 #if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring
 /* Parse the tunable string TUNESTR and adjust it to drop any tunables that may
    be unsafe for AT_SECURE processes so that it can be used as the new
@@ -375,7 +402,7 @@ __tunables_init (char **envp)
 
 	  /* Skip over tunables that have either been set already or should be
 	     skipped.  */
-	  if (cur->strval != NULL || cur->env_alias == NULL)
+	  if (cur->initialized || cur->env_alias == NULL)
 	    continue;
 
 	  const char *name = cur->env_alias;
@@ -432,7 +459,7 @@ __tunable_set_val (tunable_id_t id, void *valp, tunable_callback_t callback)
 
   /* Don't do anything if our tunable was not set during initialization or if
      it failed validation.  */
-  if (cur->strval == NULL)
+  if (!cur->initialized)
     return;
 
   /* Caller does not need the value, just call the callback with our tunable
diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
index f33adfb..7a1124a 100644
--- a/elf/dl-tunables.h
+++ b/elf/dl-tunables.h
@@ -39,8 +39,8 @@ struct _tunable
   const char *name;			/* Internal name of the tunable.  */
   tunable_type_t type;			/* Data type of the tunable.  */
   tunable_val_t val;			/* The value.  */
-  const char *strval;			/* The string containing the value,
-					   points into envp.  */
+  bool initialized;			/* Flag to indicate that the tunable is
+					   initialized.  */
   tunable_seclevel_t security_level;	/* Specify the security level for the
 					   tunable with respect to AT_SECURE
 					   programs.  See description of
@@ -61,30 +61,43 @@ typedef struct _tunable tunable_t;
 /* Full name for a tunable is top_ns.tunable_ns.id.  */
 # define TUNABLE_NAME_S(top,ns,id) #top "." #ns "." #id
 
-# define TUNABLE_ENUM_NAME(top,ns,id) TUNABLE_ENUM_NAME1 (top,ns,id)
-# define TUNABLE_ENUM_NAME1(top,ns,id) top ## _ ## ns ## _ ## id
+# define TUNABLE_ENUM_NAME(__top,__ns,__id) TUNABLE_ENUM_NAME1 (__top,__ns,__id)
+# define TUNABLE_ENUM_NAME1(__top,__ns,__id) __top ## _ ## __ns ## _ ## __id
 
 # include "dl-tunable-list.h"
 
 extern void __tunables_init (char **);
 extern void __tunable_set_val (tunable_id_t, void *, tunable_callback_t);
+extern void __tunable_update_val (tunable_id_t, void *);
+
+/* Get and return a tunable value.  */
+# define TUNABLE_GET(__top, __ns, __id, __type) \
+({									      \
+  tunable_id_t id = TUNABLE_ENUM_NAME (__top, __ns, __id);		      \
+  __type ret;								      \
+  TUNABLE_SET_VAL_FULL_WITH_CALLBACK (id, &ret, NULL);			      \
+  ret;									      \
+})
 
 /* Check if the tunable has been set to a non-default value and if it is, copy
    it over into __VAL.  */
 # define TUNABLE_SET_VAL(__id,__val) \
-({									      \
-  __tunable_set_val							      \
-   (TUNABLE_ENUM_NAME (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id), (__val),      \
-    NULL);								      \
-})
+  TUNABLE_SET_VAL_FULL_WITH_CALLBACK (TUNABLE_ENUM_NAME (TOP_NAMESPACE,       \
+							 TUNABLE_NAMESPACE,   \
+							 __id),	(__val), NULL)
 
 /* Same as TUNABLE_SET_VAL, but also call the callback function __CB.  */
 # define TUNABLE_SET_VAL_WITH_CALLBACK(__id,__val,__cb) \
-({									      \
-  __tunable_set_val							      \
-   (TUNABLE_ENUM_NAME (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id), (__val),      \
-    DL_TUNABLE_CALLBACK (__cb));					      \
-})
+  TUNABLE_SET_VAL_FULL_WITH_CALLBACK (TUNABLE_ENUM_NAME (TOP_NAMESPACE,       \
+							 TUNABLE_NAMESPACE,   \
+							 __id),		      \
+				      (__val), DL_TUNABLE_CALLBACK (__cb))
+
+# define TUNABLE_SET_VAL_FULL_WITH_CALLBACK(__id,__val,__cb)\
+  __tunable_set_val (__id, (__val), (__cb))
+
+# define TUNABLE_UPDATE_VAL(__top, __ns, __id, __val) \
+  __tunable_update_val (TUNABLE_ENUM_NAME (__top, __ns, __id), __val)
 
 /* Namespace sanity for callback functions.  Use this macro to keep the
    namespace of the modules clean.  */
-- 
2.7.4

[PATCH 5/7] tunables: Use glibc.tune.hwcap_mask tunable instead of _dl_hwcap_mask

[Siddhesh Poyarekar]

Drop _dl_hwcap_mask when building with tunables.  This completes the
transition of hwcap_mask reading from _dl_hwcap_mask to tunables.

	* elf/dl-cache.c: Include dl-tunables.h.
	(_dl_load_cache_lookup)[HAVE_TUNABLES]: Read
	glibc.tune.hwcap_mask.
	* elf/dl-hwcaps.c: Include dl-tunables.h.
	(_dl_important_hwcaps)[HAVE_TUNABLES]: Read and update
	glibc.tune.hwcap_mask.
	* sysdeps/sparc/sparc32/dl-machine.h: Likewise.
	* elf/dl-support.c (_dl_hwcap2)[HAVE_TUNABLES]: Drop
	_dl_hwcap_mask.
	* elf/dl-tunables.c (__tunable_set_val): Make a hidden alias.
	* elf/dl-tunables.h (__tunable_set_val): Likewise.
	* elf/rtld.c (rtld_global_ro)[HAVE_TUNABLES]: Drop
	_dl_hwcap_mask.
	(process_envvars)[HAVE_TUNABLES]: Likewise.
	* sysdeps/generic/ldsodefs.h (rtld_global_ro)[HAVE_TUNABLES]:
	Likewise.
	* sysdeps/x86/cpu-features.c (init_cpu_features): Don't
	initialize dl_hwcap_mask when tunables are enabled.
---
 elf/dl-cache.c                     |  9 ++++++++-
 elf/dl-hwcaps.c                    | 15 +++++++++++++--
 elf/dl-support.c                   |  2 ++
 elf/dl-tunables.c                  |  1 +
 elf/dl-tunables.h                  |  2 ++
 elf/rtld.c                         |  4 ++++
 sysdeps/generic/ldsodefs.h         |  2 ++
 sysdeps/sparc/sparc32/dl-machine.h |  7 ++++++-
 sysdeps/x86/cpu-features.c         |  4 ++++
 9 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/elf/dl-cache.c b/elf/dl-cache.c
index 017c78a..b7ae05c 100644
--- a/elf/dl-cache.c
+++ b/elf/dl-cache.c
@@ -24,6 +24,7 @@
 #include <dl-procinfo.h>
 #include <stdint.h>
 #include <_itoa.h>
+#include <dl-tunables.h>
 
 #ifndef _DL_PLATFORMS_COUNT
 # define _DL_PLATFORMS_COUNT 0
@@ -258,8 +259,14 @@ _dl_load_cache_lookup (const char *name)
       if (platform != (uint64_t) -1)
 	platform = 1ULL << platform;
 
+#if HAVE_TUNABLES
+      uint64_t hwcap_mask = TUNABLE_GET (glibc, tune, hwcap_mask, uint64_t);
+#else
+      uint64_t hwcap_mask = GLRO(dl_hwcap_mask);
+#endif
+
 #define _DL_HWCAP_TLS_MASK (1LL << 63)
-      uint64_t hwcap_exclude = ~((GLRO(dl_hwcap) & GLRO(dl_hwcap_mask))
+      uint64_t hwcap_exclude = ~((GLRO(dl_hwcap) & hwcap_mask)
 				 | _DL_HWCAP_PLATFORM | _DL_HWCAP_TLS_MASK);
 
       /* Only accept hwcap if it's for the right platform.  */
diff --git a/elf/dl-hwcaps.c b/elf/dl-hwcaps.c
index c437397..6a46441a 100644
--- a/elf/dl-hwcaps.c
+++ b/elf/dl-hwcaps.c
@@ -24,6 +24,7 @@
 #include <ldsodefs.h>
 
 #include <dl-procinfo.h>
+#include <dl-tunables.h>
 
 #ifdef _DL_FIRST_PLATFORM
 # define _DL_FIRST_EXTRA (_DL_FIRST_PLATFORM + _DL_PLATFORMS_COUNT)
@@ -37,8 +38,13 @@ internal_function
 _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
 		      size_t *max_capstrlen)
 {
+#if HAVE_TUNABLES
+  uint64_t hwcap_mask = TUNABLE_GET (glibc, tune, hwcap_mask, uint64_t);
+#else
+  uint64_t hwcap_mask = GLRO(dl_hwcap_mask);
+#endif
   /* Determine how many important bits are set.  */
-  uint64_t masked = GLRO(dl_hwcap) & GLRO(dl_hwcap_mask);
+  uint64_t masked = GLRO(dl_hwcap) & hwcap_mask;
   size_t cnt = platform != NULL;
   size_t n, m;
   size_t total;
@@ -125,7 +131,12 @@ _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
 	 LD_HWCAP_MASK environment variable (or default HWCAP_IMPORTANT).
 	 So there is no way to request ignoring an OS-supplied dsocap
 	 string and bit like you can ignore an OS-supplied HWCAP bit.  */
-      GLRO(dl_hwcap_mask) |= (uint64_t) mask << _DL_FIRST_EXTRA;
+      hwcap_mask |= (uint64_t) mask << _DL_FIRST_EXTRA;
+#if HAVE_TUNABLES
+      TUNABLE_UPDATE_VAL (glibc, tune, hwcap_mask, &hwcap_mask);
+#else
+      GLRO(dl_hwcap_mask) = hwcap_mask;
+#endif
       size_t len;
       for (const char *p = dsocaps; p < dsocaps + dsocapslen; p += len + 1)
 	{
diff --git a/elf/dl-support.c b/elf/dl-support.c
index 3c46a7a..c22be85 100644
--- a/elf/dl-support.c
+++ b/elf/dl-support.c
@@ -164,6 +164,7 @@ uint64_t _dl_hwcap2 __attribute__ ((nocommon));
 /* The value of the FPU control word the kernel will preset in hardware.  */
 fpu_control_t _dl_fpu_control = _FPU_DEFAULT;
 
+#if !HAVE_TUNABLES
 /* This is not initialized to HWCAP_IMPORTANT, matching the definition
    of _dl_important_hwcaps, below, where no hwcap strings are ever
    used.  This mask is still used to mediate the lookups in the cache
@@ -171,6 +172,7 @@ fpu_control_t _dl_fpu_control = _FPU_DEFAULT;
    LD_HWCAP_MASK environment variable here), there is no real point in
    setting _dl_hwcap nonzero below, but we do anyway.  */
 uint64_t _dl_hwcap_mask __attribute__ ((nocommon));
+#endif
 
 /* Prevailing state of the stack.  Generally this includes PF_X, indicating it's
  * executable but this isn't true for all platforms.  */
diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index abf10e5..be7733f 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -497,3 +497,4 @@ cb:
   if (callback)
     callback (&cur->val);
 }
+rtld_hidden_def (__tunable_set_val)
diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
index 7a1124a..79a29a2 100644
--- a/elf/dl-tunables.h
+++ b/elf/dl-tunables.h
@@ -70,6 +70,8 @@ extern void __tunables_init (char **);
 extern void __tunable_set_val (tunable_id_t, void *, tunable_callback_t);
 extern void __tunable_update_val (tunable_id_t, void *);
 
+rtld_hidden_proto (__tunable_set_val)
+
 /* Get and return a tunable value.  */
 # define TUNABLE_GET(__top, __ns, __id, __type) \
 ({									      \
diff --git a/elf/rtld.c b/elf/rtld.c
index 319ef06..3746653 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -159,7 +159,9 @@ struct rtld_global_ro _rtld_global_ro attribute_relro =
     ._dl_debug_fd = STDERR_FILENO,
     ._dl_use_load_bias = -2,
     ._dl_correct_cache_id = _DL_CACHE_DEFAULT_ID,
+#if !HAVE_TUNABLES
     ._dl_hwcap_mask = HWCAP_IMPORTANT,
+#endif
     ._dl_lazy = 1,
     ._dl_fpu_control = _FPU_DEFAULT,
     ._dl_pagesize = EXEC_PAGESIZE,
@@ -2402,6 +2404,7 @@ process_envvars (enum mode *modep)
 	    _dl_show_auxv ();
 	  break;
 
+#if !HAVE_TUNABLES
 	case 10:
 	  /* Mask for the important hardware capabilities.  */
 	  if (!__libc_enable_secure
@@ -2409,6 +2412,7 @@ process_envvars (enum mode *modep)
 	    GLRO(dl_hwcap_mask) = __strtoul_internal (&envline[11], NULL,
 						      0, 0);
 	  break;
+#endif
 
 	case 11:
 	  /* Path where the binary is found.  */
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index f26a8b2..695ac24 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -515,8 +515,10 @@ struct rtld_global_ro
   /* Mask for hardware capabilities that are available.  */
   EXTERN uint64_t _dl_hwcap;
 
+#if !HAVE_TUNABLES
   /* Mask for important hardware capabilities we honour. */
   EXTERN uint64_t _dl_hwcap_mask;
+#endif
 
 #ifdef HAVE_AUX_VECTOR
   /* Pointer to the auxv list supplied to the program at startup.  */
diff --git a/sysdeps/sparc/sparc32/dl-machine.h b/sysdeps/sparc/sparc32/dl-machine.h
index cf7272f..6923e47 100644
--- a/sysdeps/sparc/sparc32/dl-machine.h
+++ b/sysdeps/sparc/sparc32/dl-machine.h
@@ -39,7 +39,12 @@ elf_machine_matches_host (const Elf32_Ehdr *ehdr)
       /* XXX The following is wrong!  Dave Miller rejected to implement it
 	 correctly.  If this causes problems shoot *him*!  */
 #ifdef SHARED
-      return GLRO(dl_hwcap) & GLRO(dl_hwcap_mask) & HWCAP_SPARC_V9;
+# if HAVE_TUNABLES
+      uint64_t hwcap_mask = TUNABLE_GET (glibc, tune, hwcap_mask);
+# else
+      uint64_t hwcap_mask = GLRO(dl_hwcap_mask);
+# endif
+      return GLRO(dl_hwcap) & hwcap_mask & HWCAP_SPARC_V9;
 #else
       return GLRO(dl_hwcap) & HWCAP_SPARC_V9;
 #endif
diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
index b481f50..4fe58bf 100644
--- a/sysdeps/x86/cpu-features.c
+++ b/sysdeps/x86/cpu-features.c
@@ -316,7 +316,11 @@ no_cpuid:
   /* Reuse dl_platform, dl_hwcap and dl_hwcap_mask for x86.  */
   GLRO(dl_platform) = NULL;
   GLRO(dl_hwcap) = 0;
+#if !HAVE_TUNABLES
+  /* The glibc.tune.hwcap_mask tunable is initialized already, so no need to do
+     this.  */
   GLRO(dl_hwcap_mask) = HWCAP_IMPORTANT;
+#endif
 
 # ifdef __x86_64__
   if (cpu_features->kind == arch_kind_intel)
-- 
2.7.4

[PATCH 6/7] Delay initialization of CPU features struct in static binaries

[Siddhesh Poyarekar]

Allow the CPU features structure set up to be overridden by tunables
by delaying it to until after tunables are initialized.  The
initialization is already delayed in dynamically linked glibc, it is
only in static binaries that the initialization is set early to allow
it to influence IFUNC relocations that happen in libc-start.  It is a
bit too early however and there is a good place between tunables
initialization and IFUNC relocations where this can be done.

Verified that this does not regress the testsuite.

	* csu/libc-start.c [!ARCH_INIT_CPU_FEATURES]: Define
	ARCH_INIT_CPU_FEATURES.
	(LIBC_START_MAIN): Call it.
	* sysdeps/unix/sysv/linux/aarch64/libc-start.c
	(__libc_start_main): Remove.
	(ARCH_INIT_CPU_FEATURES): New macro.
	* sysdeps/x86/libc-start.c (__libc_start_main): Remove.
	(ARCH_INIT_CPU_FEATURES): New macro.
---
 csu/libc-start.c                             |  6 ++++++
 sysdeps/unix/sysv/linux/aarch64/libc-start.c | 23 +++++------------------
 sysdeps/x86/libc-start.c                     | 23 +++++------------------
 3 files changed, 16 insertions(+), 36 deletions(-)

diff --git a/csu/libc-start.c b/csu/libc-start.c
index 9a56dcb..c2dd159 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -104,6 +104,10 @@ apply_irel (void)
 # define MAIN_AUXVEC_PARAM
 #endif
 
+#ifndef ARCH_INIT_CPU_FEATURES
+# define ARCH_INIT_CPU_FEATURES()
+#endif
+
 STATIC int LIBC_START_MAIN (int (*main) (int, char **, char **
 					 MAIN_AUXVEC_DECL),
 			    int argc,
@@ -182,6 +186,8 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
 
   __tunables_init (__environ);
 
+  ARCH_INIT_CPU_FEATURES ();
+
   /* Perform IREL{,A} relocations.  */
   apply_irel ();
 
diff --git a/sysdeps/unix/sysv/linux/aarch64/libc-start.c b/sysdeps/unix/sysv/linux/aarch64/libc-start.c
index a5babd4..46041fe 100644
--- a/sysdeps/unix/sysv/linux/aarch64/libc-start.c
+++ b/sysdeps/unix/sysv/linux/aarch64/libc-start.c
@@ -16,26 +16,13 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#ifdef SHARED
-# include <csu/libc-start.c>
-# else
-/* The main work is done in the generic function.  */
-# define LIBC_START_DISABLE_INLINE
-# define LIBC_START_MAIN generic_start_main
-# include <csu/libc-start.c>
+#ifndef SHARED
+#include <ldsodefs.h>
 # include <cpu-features.c>
 
 extern struct cpu_features _dl_aarch64_cpu_features;
 
-int
-__libc_start_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
-		   int argc, char **argv,
-		   __typeof (main) init,
-		   void (*fini) (void),
-		   void (*rtld_fini) (void), void *stack_end)
-{
-  init_cpu_features (&_dl_aarch64_cpu_features);
-  return generic_start_main (main, argc, argv, init, fini, rtld_fini,
-			     stack_end);
-}
+#define ARCH_INIT_CPU_FEATURES() init_cpu_features (&_dl_aarch64_cpu_features)
+
 #endif
+# include <csu/libc-start.c>
diff --git a/sysdeps/x86/libc-start.c b/sysdeps/x86/libc-start.c
index 9a56adc..e11b490 100644
--- a/sysdeps/x86/libc-start.c
+++ b/sysdeps/x86/libc-start.c
@@ -15,27 +15,14 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#ifdef SHARED
-# include <csu/libc-start.c>
-# else
-/* The main work is done in the generic function.  */
-# define LIBC_START_DISABLE_INLINE
-# define LIBC_START_MAIN generic_start_main
-# include <csu/libc-start.c>
+#ifndef SHARED
+#include <ldsodefs.h>
 # include <cpu-features.h>
 # include <cpu-features.c>
 
 extern struct cpu_features _dl_x86_cpu_features;
 
-int
-__libc_start_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
-		   int argc, char **argv,
-		   __typeof (main) init,
-		   void (*fini) (void),
-		   void (*rtld_fini) (void), void *stack_end)
-{
-  init_cpu_features (&_dl_x86_cpu_features);
-  return generic_start_main (main, argc, argv, init, fini, rtld_fini,
-			     stack_end);
-}
+#define ARCH_INIT_CPU_FEATURES() init_cpu_features (&_dl_x86_cpu_features)
+
 #endif
+# include <csu/libc-start.c>
-- 
2.7.4

[PATCH 2/7] tunables: Add support for tunables of uint64_t type

[Siddhesh Poyarekar]

Recognize the uint64_t type in addition to the current int32_t and
size_t.  This allows addition of tunables of uint64_t types.  In
addition to adding the uint64_t type, this patch also consolidates
validation and reading of integer types in tunables.

One notable change is that of overflow computation in
tunables_strtoul.  The function was lifted from __internal_strtoul,
but it does not need the boundary condition check (i.e. result ==
ULONG_MAX) since it does not need to set errno.  As a result the check
can be simplified, which I have now done.

	* elf/dl-tunable-types.h (tunable_type_code_t): New type
	TUNABLE_TYPE_UINT_64.
	* elf/dl-tunables.c (tunables_strtoul): Return uint64_t.
	Simplify computation of overflow.
	(tunable_set_val_if_valid_range_signed,
	tunable_set_val_if_valid_range_unsigned): Remove and replace
	with this...
	(TUNABLE_SET_VAL_IF_VALID_RANGE): ... New macro.
	(tunable_initialize): Adjust.  Add uint64_t support.
	(__tunable_set_val): Add uint64_t support.
---
 elf/dl-tunable-types.h |  1 +
 elf/dl-tunables.c      | 94 +++++++++++++++++++++-----------------------------
 2 files changed, 41 insertions(+), 54 deletions(-)

diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
index 37a4e80..1d516df 100644
--- a/elf/dl-tunable-types.h
+++ b/elf/dl-tunable-types.h
@@ -24,6 +24,7 @@
 typedef enum
 {
   TUNABLE_TYPE_INT_32,
+  TUNABLE_TYPE_UINT_64,
   TUNABLE_TYPE_SIZE_T,
   TUNABLE_TYPE_STRING
 } tunable_type_code_t;
diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index ebf48bb..8d72e26 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -105,10 +105,10 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
 /* A stripped down strtoul-like implementation for very early use.  It does not
    set errno if the result is outside bounds because it gets called before
    errno may have been set up.  */
-static unsigned long int
+static uint64_t
 tunables_strtoul (const char *nptr)
 {
-  unsigned long int result = 0;
+  uint64_t result = 0;
   long int sign = 1;
   unsigned max_digit;
 
@@ -144,7 +144,7 @@ tunables_strtoul (const char *nptr)
 
   while (1)
     {
-      unsigned long int digval;
+      int digval;
       if (*nptr >= '0' && *nptr <= '0' + max_digit)
         digval = *nptr - '0';
       else if (base == 16)
@@ -159,9 +159,8 @@ tunables_strtoul (const char *nptr)
       else
         break;
 
-      if (result > ULONG_MAX / base
-	  || (result == ULONG_MAX / base && digval > ULONG_MAX % base))
-	return ULONG_MAX;
+      if (result >= (UINT64_MAX - digval) / base)
+	return UINT64_MAX;
       result *= base;
       result += digval;
       ++nptr;
@@ -170,68 +169,50 @@ tunables_strtoul (const char *nptr)
   return result * sign;
 }
 
-/* Initialize the internal type if the value validates either using the
-   explicit constraints of the tunable or with the implicit constraints of its
-   type.  */
-static void
-tunable_set_val_if_valid_range_signed (tunable_t *cur, const char *strval,
-				int64_t default_min, int64_t default_max)
-{
-  int64_t val = (int64_t) tunables_strtoul (strval);
-
-  int64_t min = cur->type.min;
-  int64_t max = cur->type.max;
-
-  if (min == max)
-    {
-      min = default_min;
-      max = default_max;
-    }
-
-  if (val >= min && val <= max)
-    {
-      cur->val.numval = val;
-      cur->strval = strval;
-    }
-}
-
-static void
-tunable_set_val_if_valid_range_unsigned (tunable_t *cur, const char *strval,
-					 uint64_t default_min, uint64_t default_max)
-{
-  uint64_t val = (uint64_t) tunables_strtoul (strval);
-
-  uint64_t min = cur->type.min;
-  uint64_t max = cur->type.max;
-
-  if (min == max)
-    {
-      min = default_min;
-      max = default_max;
-    }
-
-  if (val >= min && val <= max)
-    {
-      cur->val.numval = val;
-      cur->strval = strval;
-    }
-}
+#define TUNABLE_SET_VAL_IF_VALID_RANGE(__cur, __val, __type, __default_min, \
+				       __default_max)			      \
+({									      \
+  __type min = (__cur)->type.min;					      \
+  __type max = (__cur)->type.max;					      \
+									      \
+  if (min == max)							      \
+    {									      \
+      min = __default_min;						      \
+      max = __default_max;						      \
+    }									      \
+									      \
+  if ((__type) (__val) >= min && (__type) (val) <= max)			      \
+    {									      \
+      (__cur)->val.numval = val;					      \
+      (__cur)->strval = strval;						      \
+    }									      \
+})
 
 /* Validate range of the input value and initialize the tunable CUR if it looks
    good.  */
 static void
 tunable_initialize (tunable_t *cur, const char *strval)
 {
+  uint64_t val;
+
+  if (cur->type.type_code != TUNABLE_TYPE_STRING)
+    val = tunables_strtoul (strval);
+
   switch (cur->type.type_code)
     {
     case TUNABLE_TYPE_INT_32:
 	{
-	  tunable_set_val_if_valid_range_signed (cur, strval, INT32_MIN, INT32_MAX);
+	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, int64_t, INT32_MIN, INT32_MAX);
+	  break;
+	}
+    case TUNABLE_TYPE_UINT_64:
+	{
+	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t, 0, UINT64_MAX);
 	  break;
 	}
     case TUNABLE_TYPE_SIZE_T:
 	{
-	  tunable_set_val_if_valid_range_unsigned (cur, strval, 0, SIZE_MAX);
+	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t, 0, SIZE_MAX);
 	  break;
 	}
     case TUNABLE_TYPE_STRING:
@@ -461,6 +442,11 @@ __tunable_set_val (tunable_id_t id, void *valp, tunable_callback_t callback)
 
   switch (cur->type.type_code)
     {
+    case TUNABLE_TYPE_UINT_64:
+	{
+	  *((uint64_t *) valp) = (uint64_t) cur->val.numval;
+	  break;
+	}
     case TUNABLE_TYPE_INT_32:
 	{
 	  *((int32_t *) valp) = (int32_t) cur->val.numval;
-- 
2.7.4

[PATCH 4/7] tunables: Add LD_HWCAP_MASK to tunables

[Siddhesh Poyarekar]

Add LD_HWCAP_MASK to tunables in preparation of it being removed from
rtld.c.  This allows us to read LD_HWCAP_MASK much earlier so that it
can influence IFUNC resolution in aarch64.

This patch does not actually do anything other than read the
LD_HWCAP_MASK variable and add the tunables way to set the
LD_HWCAP_MASK, i.e. via the glibc.tune.hwcap_mask tunable.  In a
follow-up patch, the _dl_hwcap_mask will be replaced with
glibc.tune.hwcap_mask to complete the transition.

	* elf/dl-tunables.list: Add glibc.tune.hwcap_mask.
	* scripts/gen-tunables.awk: Include dl-procinfo.h.
---
 elf/dl-tunables.list     | 7 +++++++
 scripts/gen-tunables.awk | 1 +
 2 files changed, 8 insertions(+)

diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
index b9f1488..41ce9af 100644
--- a/elf/dl-tunables.list
+++ b/elf/dl-tunables.list
@@ -77,4 +77,11 @@ glibc {
       security_level: SXID_IGNORE
     }
   }
+  tune {
+    hwcap_mask {
+      type: UINT_64
+      env_alias: LD_HWCAP_MASK
+      default: HWCAP_IMPORTANT
+    }
+  }
 }
diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
index b10b00e..93e5aff 100644
--- a/scripts/gen-tunables.awk
+++ b/scripts/gen-tunables.awk
@@ -134,6 +134,7 @@ END {
   print "# error \"Do not include this file directly.\""
   print "# error \"Include tunables.h instead.\""
   print "#endif"
+  print "#include <dl-procinfo.h>\n"
 
   # Now, the enum names
   print "\ntypedef enum"
-- 
2.7.4

[PATCH 7/7] aarch64: Allow overriding HWCAP_CPUID feature check using HWCAP_MASK

[Siddhesh Poyarekar]

Now that LD_HWCAP_MASK (or glibc.tune.hwcap_mask) is read early enough
to influence cpu feature check in aarch64, use it to influence
multiarch selection.  Setting LD_HWCAP_MASK such that it clears
HWCAP_CPUID will now disable multiarch for the binary.

	* sysdeps/unix/sysv/linux/aarch64/cpu-features.c
	(init_cpu_features): Use glibc.tune.hwcap_mask.

Change-Id: Ia801ed6b8166b79a0cae184ee7417272f2f60593
---
 sysdeps/unix/sysv/linux/aarch64/cpu-features.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
index 7025062..0478fcc 100644
--- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
+++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
@@ -18,18 +18,25 @@
 
 #include <cpu-features.h>
 #include <sys/auxv.h>
+#include <elf/dl-tunables.h>
 
 static inline void
 init_cpu_features (struct cpu_features *cpu_features)
 {
-  if (GLRO(dl_hwcap) & HWCAP_CPUID)
+#if HAVE_TUNABLES
+  uint64_t hwcap_mask = TUNABLE_GET (glibc, tune, hwcap_mask, uint64_t);
+#else
+  uint64_t hwcap_mask = GLRO (dl_hwcap_mask);
+#endif
+
+  uint64_t hwcap = GLRO (dl_hwcap) & hwcap_mask;
+
+  if (hwcap & HWCAP_CPUID)
     {
       register uint64_t id = 0;
       asm volatile ("mrs %0, midr_el1" : "=r"(id));
       cpu_features->midr_el1 = id;
     }
   else
-    {
-      cpu_features->midr_el1 = 0;
-    }
+    cpu_features->midr_el1 = 0;
 }
-- 
2.7.4

[PATCH 1/7] tunables: Specify a default value for tunables

[Siddhesh Poyarekar]

Enhance dl-tunables.list to allow specifying a default value for a
tunable that it would be initialized to.

	* scripts/gen-tunables.awk: Recognize 'default' keyword in
	dl-tunables.list.
---
 scripts/gen-tunables.awk | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
index defb3e7..b10b00e 100644
--- a/scripts/gen-tunables.awk
+++ b/scripts/gen-tunables.awk
@@ -113,6 +113,14 @@ $1 == "}" {
       exit 1
     }
   }
+  else if (attr == "default") {
+    if (types[top_ns][ns][tunable] == "STRING") {
+      default_val[top_ns][ns][tunable] = sprintf(".strval = \"%s\"", val);
+    }
+    else {
+      default_val[top_ns][ns][tunable] = sprintf(".numval = %s", val)
+    }
+  }
 }
 
 END {
@@ -146,9 +154,9 @@ END {
     for (n in types[t]) {
       for (m in types[t][n]) {
         printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
-        printf (", {TUNABLE_TYPE_%s, %s, %s}, {.numval = 0}, NULL, TUNABLE_SECLEVEL_%s, %s},\n",
+        printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, NULL, TUNABLE_SECLEVEL_%s, %s},\n",
 		types[t][n][m], minvals[t][n][m], maxvals[t][n][m],
-		security_level[t][n][m], env_alias[t][n][m]);
+		default_val[t][n][m], security_level[t][n][m], env_alias[t][n][m]);
       }
     }
   }
-- 
2.7.4

Re: [PATCH v1.1 0/7] aarch64: Allow overriding HWCAP_CPUID feature check

[Adhemerval Zanella]

On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
> Hi,
> 
> This set of patches moves LD_HWCAP_MASK environment variable to tunables
> as glibc.tune.hwcap_mask and gets all of its uses using tunables instead.
> This is needed to override the HWCAP_CPUID bit for aarch64 to allow
> disabling of the multiarch MIDR check if needed.
> 
> The patchset enhances the tunables framework further for this and as a
> result, the tunable_list is now laid out in the .relro section so that it
> cannot be modified after relocations have been processed.  Also, tunables
> can now be set to have a default value, and can also take unsigned int64
> values.
> 
> I have done a sanity test of this patchset on aarch64 and seems to work
> just fine.  x86 would perhaps benefit from a similar delaying of
> cpu_features reading for static binaries so that they can be overriden by
> tunables in future (an aarch64 patch coming up; spoiler alert: the tunable
> name proposed is glibc.tune.mcpu) but I haven't done that at this time
> since it is not necessary for this patchset.
> 
> Update from last set:
> 
>  - Fixed up removal of dl_hwcap_mask in one case I missed during rebase.

It has fixed x86 build, however I am still seeing a runtime failure with:

FAIL: elf/tst-env-setuid

Which basically does:

$ env GCONV_PATH=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/iconvdata LOCPATH=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/localedata LC_ALL=C MALLOC_CHECK_=2 MALLOC_MMAP_THRESHOLD_=4096 LD_HWCAP_MASK=0xffffffff  /home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-env-setuid
: error while loading shared libraries: cannot create capability list: Cannot allocate memory

It is failing on elf/dl-hwcaps.c:

201   /* The result structure: we use a very compressed way to store the
202      various combinations of capability names.  */
203   *sz = 1 << cnt;
204   result = (struct r_strlenpair *) malloc (*sz * sizeof (*result) + total);
205   if (result == NULL)
206     _dl_signal_error (ENOMEM, NULL, NULL,
207                       N_("cannot create capability list"));

With a total value being way too large.  I am still digesting why exactly
this value is being too large.


> 
> Siddhesh
> 
> 
> Siddhesh Poyarekar (7):
>   tunables: Specify a default value for tunables
>   tunables: Add support for tunables of uint64_t type
>   tunables: Add hooks to get and update tunables
>   tunables: Add LD_HWCAP_MASK to tunables
>   tunables: Use glibc.tune.hwcap_mask tunable instead of _dl_hwcap_mask
>   Delay initialization of CPU features struct in static binaries
>   aarch64: Allow overriding HWCAP_CPUID feature check using HWCAP_MASK
> 
>  csu/libc-start.c                               |   6 ++
>  elf/dl-cache.c                                 |   9 +-
>  elf/dl-hwcaps.c                                |  15 ++-
>  elf/dl-support.c                               |   2 +
>  elf/dl-tunable-types.h                         |   1 +
>  elf/dl-tunables.c                              | 142 ++++++++++++++-----------
>  elf/dl-tunables.h                              |  43 +++++---
>  elf/dl-tunables.list                           |   7 ++
>  elf/rtld.c                                     |   4 +
>  scripts/gen-tunables.awk                       |  13 ++-
>  sysdeps/generic/ldsodefs.h                     |   2 +
>  sysdeps/sparc/sparc32/dl-machine.h             |   7 +-
>  sysdeps/unix/sysv/linux/aarch64/cpu-features.c |  15 ++-
>  sysdeps/unix/sysv/linux/aarch64/libc-start.c   |  23 +---
>  sysdeps/x86/cpu-features.c                     |   4 +
>  sysdeps/x86/libc-start.c                       |  23 +---
>  16 files changed, 192 insertions(+), 124 deletions(-)
> 

Re: [PATCH v1.1 0/7] aarch64: Allow overriding HWCAP_CPUID feature check

[Siddhesh Poyarekar]

[-- Attachment #1: Type: text/plain, Size: 1806 bytes --]

On Thursday 11 May 2017 09:08 PM, Adhemerval Zanella wrote:
> It has fixed x86 build, however I am still seeing a runtime failure with:
> 
> FAIL: elf/tst-env-setuid
> 
> Which basically does:
> 
> $ env GCONV_PATH=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/iconvdata LOCPATH=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/localedata LC_ALL=C MALLOC_CHECK_=2 MALLOC_MMAP_THRESHOLD_=4096 LD_HWCAP_MASK=0xffffffff  /home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-env-setuid
> : error while loading shared libraries: cannot create capability list: Cannot allocate memory
> 
> It is failing on elf/dl-hwcaps.c:
> 
> 201   /* The result structure: we use a very compressed way to store the
> 202      various combinations of capability names.  */
> 203   *sz = 1 << cnt;
> 204   result = (struct r_strlenpair *) malloc (*sz * sizeof (*result) + total);
> 205   if (result == NULL)
> 206     _dl_signal_error (ENOMEM, NULL, NULL,
> 207                       N_("cannot create capability list"));
> 
> With a total value being way too large.  I am still digesting why exactly
> this value is being too large.

Heh, I think I know what is going on.  There are two places where it
fails with the exact same error, and both can be reached when there are
too many bits set in DL_HWCAP_IMPORTANT. Setting LD_HWCAP_MASK to
0xffffffff in the test case is crossing both limits I assume.  The fact
that I don't see the failure and you do seems to be because of malloc
not failing on my system and failing on yours.  This may be due to
differences in overcommit behaviour.

Fix is to reduce the number of bits set in the test, so really just this
(untested) patch should do, can you please confirm?

Thanks,
Siddhesh

ChangeLog:

	* elf/Makefile (tst-env-setuid-ENV): Set LD_HWCAP_MASK to 0x1.

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 486 bytes --]

diff --git a/elf/Makefile b/elf/Makefile
index baf9678..14a4c27 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -1399,6 +1399,6 @@ $(objpfx)tst-nodelete-dlclose.out: $(objpfx)tst-nodelete-dlclose-dso.so \
 				   $(objpfx)tst-nodelete-dlclose-plugin.so
 
 tst-env-setuid-ENV = MALLOC_CHECK_=2 MALLOC_MMAP_THRESHOLD_=4096 \
-		     LD_HWCAP_MASK=0xffffffff
+		     LD_HWCAP_MASK=0x1
 tst-env-setuid-tunables-ENV = \
 	GLIBC_TUNABLES=glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096

Re: [PATCH v1.1 0/7] aarch64: Allow overriding HWCAP_CPUID feature check

[Szabolcs Nagy]

On 11/05/17 17:49, Siddhesh Poyarekar wrote:
> On Thursday 11 May 2017 09:08 PM, Adhemerval Zanella wrote:
>> It has fixed x86 build, however I am still seeing a runtime failure with:
>>
>> FAIL: elf/tst-env-setuid
>>
>> Which basically does:
>>
>> $ env GCONV_PATH=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/iconvdata LOCPATH=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/localedata LC_ALL=C MALLOC_CHECK_=2 MALLOC_MMAP_THRESHOLD_=4096 LD_HWCAP_MASK=0xffffffff  /home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-env-setuid
>> : error while loading shared libraries: cannot create capability list: Cannot allocate memory
>>
>> It is failing on elf/dl-hwcaps.c:
>>
>> 201   /* The result structure: we use a very compressed way to store the
>> 202      various combinations of capability names.  */
>> 203   *sz = 1 << cnt;
>> 204   result = (struct r_strlenpair *) malloc (*sz * sizeof (*result) + total);
>> 205   if (result == NULL)
>> 206     _dl_signal_error (ENOMEM, NULL, NULL,
>> 207                       N_("cannot create capability list"));
>>
>> With a total value being way too large.  I am still digesting why exactly
>> this value is being too large.
> 
> Heh, I think I know what is going on.  There are two places where it
> fails with the exact same error, and both can be reached when there are
> too many bits set in DL_HWCAP_IMPORTANT. Setting LD_HWCAP_MASK to
> 0xffffffff in the test case is crossing both limits I assume.  The fact
> that I don't see the failure and you do seems to be because of malloc
> not failing on my system and failing on yours.  This may be due to
> differences in overcommit behaviour.
> 

it seems LD_HWCAP_MASK affects ldso behaviour,
(some library search path is computed based on
"important" hwcap names)

so it's probably not a good idea to reuse it for
ifunc dispatch tuning of the libc.

(the exact semantics of the LD_HWCAP_MASK is not
clear to me though)

> Fix is to reduce the number of bits set in the test, so really just this
> (untested) patch should do, can you please confirm?
> 
> Thanks,
> Siddhesh
> 
> ChangeLog:
> 
> 	* elf/Makefile (tst-env-setuid-ENV): Set LD_HWCAP_MASK to 0x1.
> 

Re: [PATCH v1.1 0/7] aarch64: Allow overriding HWCAP_CPUID feature check

[Siddhesh Poyarekar]

On Friday 12 May 2017 05:40 PM, Szabolcs Nagy wrote:
> it seems LD_HWCAP_MASK affects ldso behaviour,
> (some library search path is computed based on
> "important" hwcap names)
> 
> so it's probably not a good idea to reuse it for
> ifunc dispatch tuning of the libc.
> 
> (the exact semantics of the LD_HWCAP_MASK is not
> clear to me though)

The dynamic linker is trying to allocate space for strings to describe
the hwcaps and somehow setting hwcap_mask like that is causing it to
allocated lots of memory.  It is a bug in the dynamic linker, not a
problem with setting hwcap_mask.

My 'solution' of fixing the test case papered over the fact that the
dynamic linker should not be barfing like this.  I'll work on this.
This is unrelated to the patchset under discussion however since the
problem should be visible even in 2.25.

Siddhesh

Re: [PATCH v1.1 0/7] aarch64: Allow overriding HWCAP_CPUID feature check

[Adhemerval Zanella]

On 12/05/2017 10:44, Siddhesh Poyarekar wrote:
> On Friday 12 May 2017 05:40 PM, Szabolcs Nagy wrote:
>> it seems LD_HWCAP_MASK affects ldso behaviour,
>> (some library search path is computed based on
>> "important" hwcap names)
>>
>> so it's probably not a good idea to reuse it for
>> ifunc dispatch tuning of the libc.
>>
>> (the exact semantics of the LD_HWCAP_MASK is not
>> clear to me though)
> 
> The dynamic linker is trying to allocate space for strings to describe
> the hwcaps and somehow setting hwcap_mask like that is causing it to
> allocated lots of memory.  It is a bug in the dynamic linker, not a
> problem with setting hwcap_mask.
> 
> My 'solution' of fixing the test case papered over the fact that the
> dynamic linker should not be barfing like this.  I'll work on this.
> This is unrelated to the patchset under discussion however since the
> problem should be visible even in 2.25.
> 
> Siddhesh
> 

That is my understanding as well, we should iron out the LD_HWCAP_MASK
for the possible flags values.  Answering your previous question,
on my system I am using vm.overcommit_kbytes=0 and setting LD_HWCAP_MASK
to 0x1 seems to make the test pass (since it won't try to preallocate lots
of memory).

Re: [PATCH 1/7] tunables: Specify a default value for tunables

[Adhemerval Zanella]

On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
> Enhance dl-tunables.list to allow specifying a default value for a
> tunable that it would be initialized to.
> 
> 	* scripts/gen-tunables.awk: Recognize 'default' keyword in
> 	dl-tunables.list.

LGTM, thanks.

> ---
>  scripts/gen-tunables.awk | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> index defb3e7..b10b00e 100644
> --- a/scripts/gen-tunables.awk
> +++ b/scripts/gen-tunables.awk
> @@ -113,6 +113,14 @@ $1 == "}" {
>        exit 1
>      }
>    }
> +  else if (attr == "default") {
> +    if (types[top_ns][ns][tunable] == "STRING") {
> +      default_val[top_ns][ns][tunable] = sprintf(".strval = \"%s\"", val);
> +    }
> +    else {
> +      default_val[top_ns][ns][tunable] = sprintf(".numval = %s", val)
> +    }
> +  }
>  }
>  
>  END {
> @@ -146,9 +154,9 @@ END {
>      for (n in types[t]) {
>        for (m in types[t][n]) {
>          printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
> -        printf (", {TUNABLE_TYPE_%s, %s, %s}, {.numval = 0}, NULL, TUNABLE_SECLEVEL_%s, %s},\n",
> +        printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, NULL, TUNABLE_SECLEVEL_%s, %s},\n",
>  		types[t][n][m], minvals[t][n][m], maxvals[t][n][m],
> -		security_level[t][n][m], env_alias[t][n][m]);
> +		default_val[t][n][m], security_level[t][n][m], env_alias[t][n][m]);
>        }
>      }
>    }
> 

Re: [PATCH 1/7] tunables: Specify a default value for tunables

[Adhemerval Zanella]

On 15/05/2017 18:12, Adhemerval Zanella wrote:
> 
> 
> On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
>> Enhance dl-tunables.list to allow specifying a default value for a
>> tunable that it would be initialized to.
>>
>> 	* scripts/gen-tunables.awk: Recognize 'default' keyword in
>> 	dl-tunables.list.
> 
> LGTM, thanks.

I just noted you should update the README.tunables with the new
allowed attribute.


> 
>> ---
>>  scripts/gen-tunables.awk | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
>> index defb3e7..b10b00e 100644
>> --- a/scripts/gen-tunables.awk
>> +++ b/scripts/gen-tunables.awk
>> @@ -113,6 +113,14 @@ $1 == "}" {
>>        exit 1
>>      }
>>    }
>> +  else if (attr == "default") {
>> +    if (types[top_ns][ns][tunable] == "STRING") {
>> +      default_val[top_ns][ns][tunable] = sprintf(".strval = \"%s\"", val);
>> +    }
>> +    else {
>> +      default_val[top_ns][ns][tunable] = sprintf(".numval = %s", val)
>> +    }
>> +  }
>>  }
>>  
>>  END {
>> @@ -146,9 +154,9 @@ END {
>>      for (n in types[t]) {
>>        for (m in types[t][n]) {
>>          printf ("  {TUNABLE_NAME_S(%s, %s, %s)", t, n, m)
>> -        printf (", {TUNABLE_TYPE_%s, %s, %s}, {.numval = 0}, NULL, TUNABLE_SECLEVEL_%s, %s},\n",
>> +        printf (", {TUNABLE_TYPE_%s, %s, %s}, {%s}, NULL, TUNABLE_SECLEVEL_%s, %s},\n",
>>  		types[t][n][m], minvals[t][n][m], maxvals[t][n][m],
>> -		security_level[t][n][m], env_alias[t][n][m]);
>> +		default_val[t][n][m], security_level[t][n][m], env_alias[t][n][m]);
>>        }
>>      }
>>    }
>>

Re: [PATCH 2/7] tunables: Add support for tunables of uint64_t type

[Adhemerval Zanella]

On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
> Recognize the uint64_t type in addition to the current int32_t and
> size_t.  This allows addition of tunables of uint64_t types.  In
> addition to adding the uint64_t type, this patch also consolidates
> validation and reading of integer types in tunables.
> 
> One notable change is that of overflow computation in
> tunables_strtoul.  The function was lifted from __internal_strtoul,
> but it does not need the boundary condition check (i.e. result ==
> ULONG_MAX) since it does not need to set errno.  As a result the check
> can be simplified, which I have now done.
> 
> 	* elf/dl-tunable-types.h (tunable_type_code_t): New type
> 	TUNABLE_TYPE_UINT_64.
> 	* elf/dl-tunables.c (tunables_strtoul): Return uint64_t.
> 	Simplify computation of overflow.
> 	(tunable_set_val_if_valid_range_signed,
> 	tunable_set_val_if_valid_range_unsigned): Remove and replace
> 	with this...
> 	(TUNABLE_SET_VAL_IF_VALID_RANGE): ... New macro.
> 	(tunable_initialize): Adjust.  Add uint64_t support.
> 	(__tunable_set_val): Add uint64_t support.

LGTM with some remarks.

> ---
>  elf/dl-tunable-types.h |  1 +
>  elf/dl-tunables.c      | 94 +++++++++++++++++++++-----------------------------
>  2 files changed, 41 insertions(+), 54 deletions(-)
> 
> diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
> index 37a4e80..1d516df 100644
> --- a/elf/dl-tunable-types.h
> +++ b/elf/dl-tunable-types.h
> @@ -24,6 +24,7 @@
>  typedef enum
>  {
>    TUNABLE_TYPE_INT_32,
> +  TUNABLE_TYPE_UINT_64,
>    TUNABLE_TYPE_SIZE_T,
>    TUNABLE_TYPE_STRING

As for previous patch we should update README.tunables with this new allowed
type.  Also, I think we should add that both hexadecimal and octal are also
supported.

>  } tunable_type_code_t;
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index ebf48bb..8d72e26 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -105,10 +105,10 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val,
>  /* A stripped down strtoul-like implementation for very early use.  It does not
>     set errno if the result is outside bounds because it gets called before
>     errno may have been set up.  */
> -static unsigned long int
> +static uint64_t
>  tunables_strtoul (const char *nptr)
>  {
> -  unsigned long int result = 0;
> +  uint64_t result = 0;
>    long int sign = 1;
>    unsigned max_digit;
>  
> @@ -144,7 +144,7 @@ tunables_strtoul (const char *nptr)
>  
>    while (1)
>      {
> -      unsigned long int digval;
> +      int digval;
>        if (*nptr >= '0' && *nptr <= '0' + max_digit)
>          digval = *nptr - '0';
>        else if (base == 16)
> @@ -159,9 +159,8 @@ tunables_strtoul (const char *nptr)
>        else
>          break;
>  
> -      if (result > ULONG_MAX / base
> -	  || (result == ULONG_MAX / base && digval > ULONG_MAX % base))
> -	return ULONG_MAX;
> +      if (result >= (UINT64_MAX - digval) / base)
> +	return UINT64_MAX;
>        result *= base;
>        result += digval;

I think this does not really handle overflows correctly and I would suggest
to actually use the new check_mul_overflow_size_t macro from reallocarray
patch to actually check it and result UINT64_MAX for the case.

Also, do we have any generic value range input test for tunable interface
(to check for validation, overflow, underflow, etc.)?


>        ++nptr;
> @@ -170,68 +169,50 @@ tunables_strtoul (const char *nptr)
>    return result * sign;
>  }
>  
> -/* Initialize the internal type if the value validates either using the
> -   explicit constraints of the tunable or with the implicit constraints of its
> -   type.  */
> -static void
> -tunable_set_val_if_valid_range_signed (tunable_t *cur, const char *strval,
> -				int64_t default_min, int64_t default_max)
> -{
> -  int64_t val = (int64_t) tunables_strtoul (strval);
> -
> -  int64_t min = cur->type.min;
> -  int64_t max = cur->type.max;
> -
> -  if (min == max)
> -    {
> -      min = default_min;
> -      max = default_max;
> -    }
> -
> -  if (val >= min && val <= max)
> -    {
> -      cur->val.numval = val;
> -      cur->strval = strval;
> -    }
> -}
> -
> -static void
> -tunable_set_val_if_valid_range_unsigned (tunable_t *cur, const char *strval,
> -					 uint64_t default_min, uint64_t default_max)
> -{
> -  uint64_t val = (uint64_t) tunables_strtoul (strval);
> -
> -  uint64_t min = cur->type.min;
> -  uint64_t max = cur->type.max;
> -
> -  if (min == max)
> -    {
> -      min = default_min;
> -      max = default_max;
> -    }
> -
> -  if (val >= min && val <= max)
> -    {
> -      cur->val.numval = val;
> -      cur->strval = strval;
> -    }
> -}
> +#define TUNABLE_SET_VAL_IF_VALID_RANGE(__cur, __val, __type, __default_min, \
> +				       __default_max)			      \
> +({									      \
> +  __type min = (__cur)->type.min;					      \
> +  __type max = (__cur)->type.max;					      \
> +									      \
> +  if (min == max)							      \
> +    {									      \
> +      min = __default_min;						      \
> +      max = __default_max;						      \
> +    }									      \
> +									      \
> +  if ((__type) (__val) >= min && (__type) (val) <= max)			      \
> +    {									      \
> +      (__cur)->val.numval = val;					      \
> +      (__cur)->strval = strval;						      \
> +    }									      \
> +})
>  
>  /* Validate range of the input value and initialize the tunable CUR if it looks
>     good.  */
>  static void
>  tunable_initialize (tunable_t *cur, const char *strval)
>  {
> +  uint64_t val;
> +
> +  if (cur->type.type_code != TUNABLE_TYPE_STRING)
> +    val = tunables_strtoul (strval);
> +
>    switch (cur->type.type_code)
>      {
>      case TUNABLE_TYPE_INT_32:
>  	{
> -	  tunable_set_val_if_valid_range_signed (cur, strval, INT32_MIN, INT32_MAX);
> +	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, int64_t, INT32_MIN, INT32_MAX);
> +	  break;
> +	}
> +    case TUNABLE_TYPE_UINT_64:
> +	{
> +	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t, 0, UINT64_MAX);
>  	  break;
>  	}
>      case TUNABLE_TYPE_SIZE_T:
>  	{
> -	  tunable_set_val_if_valid_range_unsigned (cur, strval, 0, SIZE_MAX);
> +	  TUNABLE_SET_VAL_IF_VALID_RANGE (cur, val, uint64_t, 0, SIZE_MAX);
>  	  break;
>  	}
>      case TUNABLE_TYPE_STRING:
> @@ -461,6 +442,11 @@ __tunable_set_val (tunable_id_t id, void *valp, tunable_callback_t callback)
>  
>    switch (cur->type.type_code)
>      {
> +    case TUNABLE_TYPE_UINT_64:
> +	{
> +	  *((uint64_t *) valp) = (uint64_t) cur->val.numval;
> +	  break;
> +	}
>      case TUNABLE_TYPE_INT_32:
>  	{
>  	  *((int32_t *) valp) = (int32_t) cur->val.numval;
> 

Re: [PATCH 3/7] tunables: Add hooks to get and update tunables

[Adhemerval Zanella]

On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
> This change adds two new tunable macros to get and update tunables
> respectively called TUNABLE_GET and TUNABLE_UPDATE_VAL.
> 
> TUNABLE_GET accepts the top namespace, tunable namespace and tunable
> id along with the target type as arguments and returns a value of the
> target type.  For example:
> 
>     TUNABLE_GET (glibc, malloc, check, int32_t)
> 
> will return the tunable value for glibc.malloc.check in an int32_t.
> 
> TUNABLE_UPDATE_VAL accepts the top namespace, tunable namespace and
> tunable id along with a pointer to the value to be stored into the
> tunable structure.  For example:
> 
>   int32_t val = 1;
>   TUNABLE_UPDATE_VAL (glibc, malloc, check, &val);
> 
> will update the glibc.malloc.check tunable.

I would prefer to simplify a bit this macro to avoid use pointer and
use the value instead and specify the type directly (so we can add
some range/type check in the future).  Something like:

# define TUNABLE_UPDATE_VAL(__top, __ns, __id, __val, __type)	   \
  ({								   \
    __tunable_update_val (TUNABLE_ENUM_NAME (__top, __ns, __id),   \
					     &(__type) { __val }); \	
   })

> 
> Given that the tunable structure is now relro, this is only really
> possible from within the dynamic linker before it is relocated.  In
> future the tunable list may get split into mutable and immutable
> tunables where mutable tunables can be modified by the library and
> userspace after relocation as well.  However whenever we actually do
> that, we will have to ensure that the mutable tunables are protected
> with locks.

This worries me a bit because I would like a compiler/linker warning
that updating a tunable in wrong place is not allowed.  Is there a
way to make compiler/linker barf with an invalid update?

> 
> 	* elf/dl-tunables.h (strval): Replace with a bool initialized.
> 	(TUNABLE_ENUM_NAME, TUNABLE_ENUM_NAME1): Adjust names to
> 	prevent collision.
> 	(__tunable_update_val): New function.
> 	(TUNABLE_GET): New macro.
> 	(TUNABLE_SET_VAL_FULL_WITH_CALLBACK): New macro.
> 	(TUNABLE_SET_VAL): Use it.
> 	(TUNABLE_SET_VAL_WITH_CALLBACK): Likewise.
> 	(TUNABLE_UPDATE_VAL): New macro.
> 	(tunables_strtoul): Replace strval with initialized.
> 	(__tunables_init): Likewise.
> 	(__tunable_set_val): Likewise.
> 	(do_tunable_update_val): New function.
> 	(tunables_initialize): Use it.
> 	(__tunable_update_val): New function.
> ---
>  elf/dl-tunables.c | 43 +++++++++++++++++++++++++++++++++++--------
>  elf/dl-tunables.h | 41 +++++++++++++++++++++++++++--------------
>  2 files changed, 62 insertions(+), 22 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index 8d72e26..abf10e5 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -184,19 +184,17 @@ tunables_strtoul (const char *nptr)
>    if ((__type) (__val) >= min && (__type) (val) <= max)			      \
>      {									      \
>        (__cur)->val.numval = val;					      \
> -      (__cur)->strval = strval;						      \
> +      (__cur)->initialized = true;					      \
>      }									      \
>  })
>  
> -/* Validate range of the input value and initialize the tunable CUR if it looks
> -   good.  */
>  static void
> -tunable_initialize (tunable_t *cur, const char *strval)
> +do_tunable_update_val (tunable_t *cur, const void *valp)
>  {
>    uint64_t val;
>  
>    if (cur->type.type_code != TUNABLE_TYPE_STRING)
> -    val = tunables_strtoul (strval);
> +    val = *((int64_t *) valp);
>  
>    switch (cur->type.type_code)
>      {
> @@ -217,7 +215,7 @@ tunable_initialize (tunable_t *cur, const char *strval)
>  	}
>      case TUNABLE_TYPE_STRING:
>  	{
> -	  cur->val.strval = cur->strval = strval;
> +	  cur->val.strval = valp;
>  	  break;
>  	}
>      default:
> @@ -225,6 +223,35 @@ tunable_initialize (tunable_t *cur, const char *strval)
>      }
>  }
>  
> +/* Validate range of the input value and initialize the tunable CUR if it looks
> +   good.  */
> +static void
> +tunable_initialize (tunable_t *cur, const char *strval)
> +{
> +  uint64_t val;
> +  const void *valp;
> +
> +  if (cur->type.type_code != TUNABLE_TYPE_STRING)
> +    {
> +      val = tunables_strtoul (strval);
> +      valp = &val;
> +    }
> +  else
> +    {
> +      cur->initialized = true;
> +      valp = strval;
> +    }
> +  do_tunable_update_val (cur, valp);
> +}
> +
> +void
> +__tunable_update_val (tunable_id_t id, void *valp)
> +{
> +  tunable_t *cur = &tunable_list[id];
> +
> +  do_tunable_update_val (cur, valp);
> +}
> +
>  #if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring
>  /* Parse the tunable string TUNESTR and adjust it to drop any tunables that may
>     be unsafe for AT_SECURE processes so that it can be used as the new
> @@ -375,7 +402,7 @@ __tunables_init (char **envp)
>  
>  	  /* Skip over tunables that have either been set already or should be
>  	     skipped.  */
> -	  if (cur->strval != NULL || cur->env_alias == NULL)
> +	  if (cur->initialized || cur->env_alias == NULL)
>  	    continue;
>  
>  	  const char *name = cur->env_alias;
> @@ -432,7 +459,7 @@ __tunable_set_val (tunable_id_t id, void *valp, tunable_callback_t callback)
>  
>    /* Don't do anything if our tunable was not set during initialization or if
>       it failed validation.  */
> -  if (cur->strval == NULL)
> +  if (!cur->initialized)
>      return;
>  
>    /* Caller does not need the value, just call the callback with our tunable
> diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
> index f33adfb..7a1124a 100644
> --- a/elf/dl-tunables.h
> +++ b/elf/dl-tunables.h
> @@ -39,8 +39,8 @@ struct _tunable
>    const char *name;			/* Internal name of the tunable.  */
>    tunable_type_t type;			/* Data type of the tunable.  */
>    tunable_val_t val;			/* The value.  */
> -  const char *strval;			/* The string containing the value,
> -					   points into envp.  */
> +  bool initialized;			/* Flag to indicate that the tunable is
> +					   initialized.  */
>    tunable_seclevel_t security_level;	/* Specify the security level for the
>  					   tunable with respect to AT_SECURE
>  					   programs.  See description of
> @@ -61,30 +61,43 @@ typedef struct _tunable tunable_t;
>  /* Full name for a tunable is top_ns.tunable_ns.id.  */
>  # define TUNABLE_NAME_S(top,ns,id) #top "." #ns "." #id
>  
> -# define TUNABLE_ENUM_NAME(top,ns,id) TUNABLE_ENUM_NAME1 (top,ns,id)
> -# define TUNABLE_ENUM_NAME1(top,ns,id) top ## _ ## ns ## _ ## id
> +# define TUNABLE_ENUM_NAME(__top,__ns,__id) TUNABLE_ENUM_NAME1 (__top,__ns,__id)
> +# define TUNABLE_ENUM_NAME1(__top,__ns,__id) __top ## _ ## __ns ## _ ## __id
>  
>  # include "dl-tunable-list.h"
>  
>  extern void __tunables_init (char **);
>  extern void __tunable_set_val (tunable_id_t, void *, tunable_callback_t);
> +extern void __tunable_update_val (tunable_id_t, void *);
> +
> +/* Get and return a tunable value.  */
> +# define TUNABLE_GET(__top, __ns, __id, __type) \
> +({									      \
> +  tunable_id_t id = TUNABLE_ENUM_NAME (__top, __ns, __id);		      \
> +  __type ret;								      \
> +  TUNABLE_SET_VAL_FULL_WITH_CALLBACK (id, &ret, NULL);			      \
> +  ret;									      \
> +})
>  
>  /* Check if the tunable has been set to a non-default value and if it is, copy
>     it over into __VAL.  */
>  # define TUNABLE_SET_VAL(__id,__val) \
> -({									      \
> -  __tunable_set_val							      \
> -   (TUNABLE_ENUM_NAME (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id), (__val),      \
> -    NULL);								      \
> -})
> +  TUNABLE_SET_VAL_FULL_WITH_CALLBACK (TUNABLE_ENUM_NAME (TOP_NAMESPACE,       \
> +							 TUNABLE_NAMESPACE,   \
> +							 __id),	(__val), NULL)
>  
>  /* Same as TUNABLE_SET_VAL, but also call the callback function __CB.  */
>  # define TUNABLE_SET_VAL_WITH_CALLBACK(__id,__val,__cb) \
> -({									      \
> -  __tunable_set_val							      \
> -   (TUNABLE_ENUM_NAME (TOP_NAMESPACE, TUNABLE_NAMESPACE, __id), (__val),      \
> -    DL_TUNABLE_CALLBACK (__cb));					      \
> -})
> +  TUNABLE_SET_VAL_FULL_WITH_CALLBACK (TUNABLE_ENUM_NAME (TOP_NAMESPACE,       \
> +							 TUNABLE_NAMESPACE,   \
> +							 __id),		      \
> +				      (__val), DL_TUNABLE_CALLBACK (__cb))
> +
> +# define TUNABLE_SET_VAL_FULL_WITH_CALLBACK(__id,__val,__cb)\
> +  __tunable_set_val (__id, (__val), (__cb))
> +
> +# define TUNABLE_UPDATE_VAL(__top, __ns, __id, __val) \
> +  __tunable_update_val (TUNABLE_ENUM_NAME (__top, __ns, __id), __val)
>  
>  /* Namespace sanity for callback functions.  Use this macro to keep the
>     namespace of the modules clean.  */
> 

Re: [PATCH 4/7] tunables: Add LD_HWCAP_MASK to tunables

[Adhemerval Zanella]

On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
> Add LD_HWCAP_MASK to tunables in preparation of it being removed from
> rtld.c.  This allows us to read LD_HWCAP_MASK much earlier so that it
> can influence IFUNC resolution in aarch64.
> 
> This patch does not actually do anything other than read the
> LD_HWCAP_MASK variable and add the tunables way to set the
> LD_HWCAP_MASK, i.e. via the glibc.tune.hwcap_mask tunable.  In a
> follow-up patch, the _dl_hwcap_mask will be replaced with
> glibc.tune.hwcap_mask to complete the transition.
> 
> 	* elf/dl-tunables.list: Add glibc.tune.hwcap_mask.
> 	* scripts/gen-tunables.awk: Include dl-procinfo.h.

I think we should also update manual/tunables.texi with this new one.

> ---
>  elf/dl-tunables.list     | 7 +++++++
>  scripts/gen-tunables.awk | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
> index b9f1488..41ce9af 100644
> --- a/elf/dl-tunables.list
> +++ b/elf/dl-tunables.list
> @@ -77,4 +77,11 @@ glibc {
>        security_level: SXID_IGNORE
>      }
>    }
> +  tune {
> +    hwcap_mask {
> +      type: UINT_64
> +      env_alias: LD_HWCAP_MASK
> +      default: HWCAP_IMPORTANT
> +    }
> +  }
>  }
> diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk
> index b10b00e..93e5aff 100644
> --- a/scripts/gen-tunables.awk
> +++ b/scripts/gen-tunables.awk
> @@ -134,6 +134,7 @@ END {
>    print "# error \"Do not include this file directly.\""
>    print "# error \"Include tunables.h instead.\""
>    print "#endif"
> +  print "#include <dl-procinfo.h>\n"
>  
>    # Now, the enum names
>    print "\ntypedef enum"
> 

Re: [PATCH 2/7] tunables: Add support for tunables of uint64_t type

[Siddhesh Poyarekar]

On Tuesday 16 May 2017 03:39 AM, Adhemerval Zanella wrote:
> As for previous patch we should update README.tunables with this new allowed
> type.  Also, I think we should add that both hexadecimal and octal are also
> supported.

I've added this.

> I think this does not really handle overflows correctly and I would suggest
> to actually use the new check_mul_overflow_size_t macro from reallocarray
> patch to actually check it and result UINT64_MAX for the case.

I don't understand, can you please elaborate?  I am specifically trying
to ensure that the computation does not overflow at all and saturating
the result at UINT64_MAX if the result is too large.  The subsequent
TUNABLE_SET_VAL_IF_VALID_RANGE check should then do a range check before
setting the tunable value and refuse to set it if it is beyond the
bounds of the tunable type.

> Also, do we have any generic value range input test for tunable interface
> (to check for validation, overflow, underflow, etc.)?

TUNABLE_SET_VAL_IF_VALID_RANGE does the range check and avoids setting
the tunable if the value is not within the range of its type or the set
range, whichever is smaller.

Siddhesh

Re: [PATCH 3/7] tunables: Add hooks to get and update tunables

[Siddhesh Poyarekar]

On Wednesday 17 May 2017 01:59 AM, Adhemerval Zanella wrote:
> I would prefer to simplify a bit this macro to avoid use pointer and
> use the value instead and specify the type directly (so we can add
> some range/type check in the future).  Something like:
> 
> # define TUNABLE_UPDATE_VAL(__top, __ns, __id, __val, __type)	   \
>   ({								   \
>     __tunable_update_val (TUNABLE_ENUM_NAME (__top, __ns, __id),   \
> 					     &(__type) { __val }); \	
>    })

It already does the range check based on the type of the tunable using
the TUNABLE_SET_VAL_IF_VALID_RANGE macro.  Isn't that sufficient?

>> Given that the tunable structure is now relro, this is only really
>> possible from within the dynamic linker before it is relocated.  In
>> future the tunable list may get split into mutable and immutable
>> tunables where mutable tunables can be modified by the library and
>> userspace after relocation as well.  However whenever we actually do
>> that, we will have to ensure that the mutable tunables are protected
>> with locks.
> 
> This worries me a bit because I would like a compiler/linker warning
> that updating a tunable in wrong place is not allowed.  Is there a
> way to make compiler/linker barf with an invalid update?

The __tunable_update_val function is currently private to ld.so, so the
likelihood of doing an update outside of the linker (and hence after
relocation) is remote.  The only possibility is for the possibility of
the tunable being updated inside ld.so after relocation, but that is the
case for GLRO(...) vars too.

Maybe something for us to note in future is to ensure that if/when we
introduce tunables that can be updated at runtime, we ensure that they
have different accessors from the regular tunables that cannot touch the
immutable tunables.

Siddhesh

Re: [PATCH 4/7] tunables: Add LD_HWCAP_MASK to tunables

[Siddhesh Poyarekar]

On Wednesday 17 May 2017 02:04 AM, Adhemerval Zanella wrote:
> On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
>> Add LD_HWCAP_MASK to tunables in preparation of it being removed from
>> rtld.c.  This allows us to read LD_HWCAP_MASK much earlier so that it
>> can influence IFUNC resolution in aarch64.
>>
>> This patch does not actually do anything other than read the
>> LD_HWCAP_MASK variable and add the tunables way to set the
>> LD_HWCAP_MASK, i.e. via the glibc.tune.hwcap_mask tunable.  In a
>> follow-up patch, the _dl_hwcap_mask will be replaced with
>> glibc.tune.hwcap_mask to complete the transition.
>>
>> 	* elf/dl-tunables.list: Add glibc.tune.hwcap_mask.
>> 	* scripts/gen-tunables.awk: Include dl-procinfo.h.
> 
> I think we should also update manual/tunables.texi with this new one.

OK, I'll send an updated series (minus the first two you acked) with
this change.

Siddhesh

Re: [PATCH 4/7] tunables: Add LD_HWCAP_MASK to tunables

[Siddhesh Poyarekar]

On Wednesday 17 May 2017 12:48 PM, Siddhesh Poyarekar wrote:
> OK, I'll send an updated series (minus the first two you acked) with
> this change.

Actually on second thoughts I'll wait for you to review the remaining
patches before reposting another iteration.

Siddhesh

Re: [PATCH 2/7] tunables: Add support for tunables of uint64_t type

[Adhemerval Zanella]

On 17/05/2017 04:07, Siddhesh Poyarekar wrote:
> On Tuesday 16 May 2017 03:39 AM, Adhemerval Zanella wrote:
>> As for previous patch we should update README.tunables with this new allowed
>> type.  Also, I think we should add that both hexadecimal and octal are also
>> supported.
> 
> I've added this.
> 
>> I think this does not really handle overflows correctly and I would suggest
>> to actually use the new check_mul_overflow_size_t macro from reallocarray
>> patch to actually check it and result UINT64_MAX for the case.
> 
> I don't understand, can you please elaborate?  I am specifically trying
> to ensure that the computation does not overflow at all and saturating
> the result at UINT64_MAX if the result is too large.  The subsequent
> TUNABLE_SET_VAL_IF_VALID_RANGE check should then do a range check before
> setting the tunable value and refuse to set it if it is beyond the
> bounds of the tunable type.
> 

My mistake here, I noticed after fire up the email that it is indeed
checking correctly for overflow.

>> Also, do we have any generic value range input test for tunable interface
>> (to check for validation, overflow, underflow, etc.)?
> 
> TUNABLE_SET_VAL_IF_VALID_RANGE does the range check and avoids setting
> the tunable if the value is not within the range of its type or the set
> range, whichever is smaller.
> 
> Siddhesh

Sorry, I meant an external testcase to stress out such cases.

Re: [PATCH 3/7] tunables: Add hooks to get and update tunables

[Adhemerval Zanella]

On 17/05/2017 04:17, Siddhesh Poyarekar wrote:
> On Wednesday 17 May 2017 01:59 AM, Adhemerval Zanella wrote:
>> I would prefer to simplify a bit this macro to avoid use pointer and
>> use the value instead and specify the type directly (so we can add
>> some range/type check in the future).  Something like:
>>
>> # define TUNABLE_UPDATE_VAL(__top, __ns, __id, __val, __type)	   \
>>   ({								   \
>>     __tunable_update_val (TUNABLE_ENUM_NAME (__top, __ns, __id),   \
>> 					     &(__type) { __val }); \	
>>    })
> 
> It already does the range check based on the type of the tunable using
> the TUNABLE_SET_VAL_IF_VALID_RANGE macro.  Isn't that sufficient?

Indeed and I think it might be extensible enough for user-defined types
(for instance a tunable that might have a very strict value range).  In
any case, I still think to use the value instead of a pointer would be
simpler, unless you plan to TUNABLE_UPDATE_VAL to return the previous
value (which seems to the case currently).

> 
>>> Given that the tunable structure is now relro, this is only really
>>> possible from within the dynamic linker before it is relocated.  In
>>> future the tunable list may get split into mutable and immutable
>>> tunables where mutable tunables can be modified by the library and
>>> userspace after relocation as well.  However whenever we actually do
>>> that, we will have to ensure that the mutable tunables are protected
>>> with locks.
>>
>> This worries me a bit because I would like a compiler/linker warning
>> that updating a tunable in wrong place is not allowed.  Is there a
>> way to make compiler/linker barf with an invalid update?
> 
> The __tunable_update_val function is currently private to ld.so, so the
> likelihood of doing an update outside of the linker (and hence after
> relocation) is remote.  The only possibility is for the possibility of
> the tunable being updated inside ld.so after relocation, but that is the
> case for GLRO(...) vars too.
> 
> Maybe something for us to note in future is to ensure that if/when we
> introduce tunables that can be updated at runtime, we ensure that they
> have different accessors from the regular tunables that cannot touch the
> immutable tunables.

Fair enough.

Re: [PATCH 4/7] tunables: Add LD_HWCAP_MASK to tunables

[Adhemerval Zanella]

On 17/05/2017 06:44, Siddhesh Poyarekar wrote:
> On Wednesday 17 May 2017 12:48 PM, Siddhesh Poyarekar wrote:
>> OK, I'll send an updated series (minus the first two you acked) with
>> this change.
> 
> Actually on second thoughts I'll wait for you to review the remaining
> patches before reposting another iteration.
> 
> Siddhesh
> 

Right, I plan to finishing up the remaining ones today or by tomorrow.

Re: [PATCH 5/7] tunables: Use glibc.tune.hwcap_mask tunable instead of _dl_hwcap_mask

[Adhemerval Zanella]

On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
> Drop _dl_hwcap_mask when building with tunables.  This completes the
> transition of hwcap_mask reading from _dl_hwcap_mask to tunables.
> 
> 	* elf/dl-cache.c: Include dl-tunables.h.
> 	(_dl_load_cache_lookup)[HAVE_TUNABLES]: Read
> 	glibc.tune.hwcap_mask.
> 	* elf/dl-hwcaps.c: Include dl-tunables.h.
> 	(_dl_important_hwcaps)[HAVE_TUNABLES]: Read and update
> 	glibc.tune.hwcap_mask.
> 	* sysdeps/sparc/sparc32/dl-machine.h: Likewise.
> 	* elf/dl-support.c (_dl_hwcap2)[HAVE_TUNABLES]: Drop
> 	_dl_hwcap_mask.
> 	* elf/dl-tunables.c (__tunable_set_val): Make a hidden alias.
> 	* elf/dl-tunables.h (__tunable_set_val): Likewise.
> 	* elf/rtld.c (rtld_global_ro)[HAVE_TUNABLES]: Drop
> 	_dl_hwcap_mask.
> 	(process_envvars)[HAVE_TUNABLES]: Likewise.
> 	* sysdeps/generic/ldsodefs.h (rtld_global_ro)[HAVE_TUNABLES]:
> 	Likewise.
> 	* sysdeps/x86/cpu-features.c (init_cpu_features): Don't
> 	initialize dl_hwcap_mask when tunables are enabled.

I would recommend to incorporate the fix for LD_HWCAP_MASK on tst-end-setuid
you sent before [1] to avoid this fail on some configuration (mine for
instance).  Also I think we should open a bugzilla to iron out the
elf/dl-hwcaps.c allocation scheme which is triggering this issue.

[1] https://sourceware.org/ml/libc-alpha/2017-05/msg00340.html


> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index f26a8b2..695ac24 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -515,8 +515,10 @@ struct rtld_global_ro
>    /* Mask for hardware capabilities that are available.  */
>    EXTERN uint64_t _dl_hwcap;
>  
> +#if !HAVE_TUNABLES
>    /* Mask for important hardware capabilities we honour. */
>    EXTERN uint64_t _dl_hwcap_mask;
> +#endif
>  
>  #ifdef HAVE_AUX_VECTOR
>    /* Pointer to the auxv list supplied to the program at startup.  */
> diff --git a/sysdeps/sparc/sparc32/dl-machine.h b/sysdeps/sparc/sparc32/dl-machine.h
> index cf7272f..6923e47 100644
> --- a/sysdeps/sparc/sparc32/dl-machine.h
> +++ b/sysdeps/sparc/sparc32/dl-machine.h
> @@ -39,7 +39,12 @@ elf_machine_matches_host (const Elf32_Ehdr *ehdr)
>        /* XXX The following is wrong!  Dave Miller rejected to implement it
>  	 correctly.  If this causes problems shoot *him*!  */
>  #ifdef SHARED
> -      return GLRO(dl_hwcap) & GLRO(dl_hwcap_mask) & HWCAP_SPARC_V9;
> +# if HAVE_TUNABLES
> +      uint64_t hwcap_mask = TUNABLE_GET (glibc, tune, hwcap_mask);

You are missing '#include <elf/dl-tunables.h>' and the final type argument for
TUNABLE_GET (it fails to build for sparcv9 with tunable enabled).

> +# else
> +      uint64_t hwcap_mask = GLRO(dl_hwcap_mask);
> +# endif
> +      return GLRO(dl_hwcap) & hwcap_mask & HWCAP_SPARC_V9;
>  #else
>        return GLRO(dl_hwcap) & HWCAP_SPARC_V9;
>  #endif
> diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
> index b481f50..4fe58bf 100644
> --- a/sysdeps/x86/cpu-features.c
> +++ b/sysdeps/x86/cpu-features.c
> @@ -316,7 +316,11 @@ no_cpuid:
>    /* Reuse dl_platform, dl_hwcap and dl_hwcap_mask for x86.  */
>    GLRO(dl_platform) = NULL;
>    GLRO(dl_hwcap) = 0;
> +#if !HAVE_TUNABLES
> +  /* The glibc.tune.hwcap_mask tunable is initialized already, so no need to do
> +     this.  */
>    GLRO(dl_hwcap_mask) = HWCAP_IMPORTANT;
> +#endif
>  
>  # ifdef __x86_64__
>    if (cpu_features->kind == arch_kind_intel)
> 

Re: [PATCH 5/7] tunables: Use glibc.tune.hwcap_mask tunable instead of _dl_hwcap_mask

[Adhemerval Zanella]

On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
> Drop _dl_hwcap_mask when building with tunables.  This completes the
> transition of hwcap_mask reading from _dl_hwcap_mask to tunables.
> 
> 	* elf/dl-cache.c: Include dl-tunables.h.
> 	(_dl_load_cache_lookup)[HAVE_TUNABLES]: Read
> 	glibc.tune.hwcap_mask.
> 	* elf/dl-hwcaps.c: Include dl-tunables.h.
> 	(_dl_important_hwcaps)[HAVE_TUNABLES]: Read and update
> 	glibc.tune.hwcap_mask.
> 	* sysdeps/sparc/sparc32/dl-machine.h: Likewise.
> 	* elf/dl-support.c (_dl_hwcap2)[HAVE_TUNABLES]: Drop
> 	_dl_hwcap_mask.
> 	* elf/dl-tunables.c (__tunable_set_val): Make a hidden alias.
> 	* elf/dl-tunables.h (__tunable_set_val): Likewise.
> 	* elf/rtld.c (rtld_global_ro)[HAVE_TUNABLES]: Drop
> 	_dl_hwcap_mask.
> 	(process_envvars)[HAVE_TUNABLES]: Likewise.
> 	* sysdeps/generic/ldsodefs.h (rtld_global_ro)[HAVE_TUNABLES]:
> 	Likewise.
> 	* sysdeps/x86/cpu-features.c (init_cpu_features): Don't
> 	initialize dl_hwcap_mask when tunables are enabled.

I would recommend to incorporate the fix for LD_HWCAP_MASK on tst-end-setuid
you sent before [1] to avoid this fail on some configuration (mine for
instance).  Also I think we should open a bugzilla to iron out the
elf/dl-hwcaps.c allocation scheme which is triggering this issue.

[1] https://sourceware.org/ml/libc-alpha/2017-05/msg00340.html


> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index f26a8b2..695ac24 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -515,8 +515,10 @@ struct rtld_global_ro
>    /* Mask for hardware capabilities that are available.  */
>    EXTERN uint64_t _dl_hwcap;
>  
> +#if !HAVE_TUNABLES
>    /* Mask for important hardware capabilities we honour. */
>    EXTERN uint64_t _dl_hwcap_mask;
> +#endif
>  
>  #ifdef HAVE_AUX_VECTOR
>    /* Pointer to the auxv list supplied to the program at startup.  */
> diff --git a/sysdeps/sparc/sparc32/dl-machine.h b/sysdeps/sparc/sparc32/dl-machine.h
> index cf7272f..6923e47 100644
> --- a/sysdeps/sparc/sparc32/dl-machine.h
> +++ b/sysdeps/sparc/sparc32/dl-machine.h
> @@ -39,7 +39,12 @@ elf_machine_matches_host (const Elf32_Ehdr *ehdr)
>        /* XXX The following is wrong!  Dave Miller rejected to implement it
>  	 correctly.  If this causes problems shoot *him*!  */
>  #ifdef SHARED
> -      return GLRO(dl_hwcap) & GLRO(dl_hwcap_mask) & HWCAP_SPARC_V9;
> +# if HAVE_TUNABLES
> +      uint64_t hwcap_mask = TUNABLE_GET (glibc, tune, hwcap_mask);

You are missing '#include <elf/dl-tunables.h>' and the final type argument for
TUNABLE_GET (it fails to build for sparcv9 with tunable enabled).

> +# else
> +      uint64_t hwcap_mask = GLRO(dl_hwcap_mask);
> +# endif
> +      return GLRO(dl_hwcap) & hwcap_mask & HWCAP_SPARC_V9;
>  #else
>        return GLRO(dl_hwcap) & HWCAP_SPARC_V9;
>  #endif
> diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
> index b481f50..4fe58bf 100644
> --- a/sysdeps/x86/cpu-features.c
> +++ b/sysdeps/x86/cpu-features.c
> @@ -316,7 +316,11 @@ no_cpuid:
>    /* Reuse dl_platform, dl_hwcap and dl_hwcap_mask for x86.  */
>    GLRO(dl_platform) = NULL;
>    GLRO(dl_hwcap) = 0;
> +#if !HAVE_TUNABLES
> +  /* The glibc.tune.hwcap_mask tunable is initialized already, so no need to do
> +     this.  */
>    GLRO(dl_hwcap_mask) = HWCAP_IMPORTANT;
> +#endif
>  
>  # ifdef __x86_64__
>    if (cpu_features->kind == arch_kind_intel)
> 

Re: [PATCH 6/7] Delay initialization of CPU features struct in static binaries

[Adhemerval Zanella]

On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
> Allow the CPU features structure set up to be overridden by tunables
> by delaying it to until after tunables are initialized.  The
> initialization is already delayed in dynamically linked glibc, it is
> only in static binaries that the initialization is set early to allow
> it to influence IFUNC relocations that happen in libc-start.  It is a
> bit too early however and there is a good place between tunables
> initialization and IFUNC relocations where this can be done.
> 
> Verified that this does not regress the testsuite.
> 
> 	* csu/libc-start.c [!ARCH_INIT_CPU_FEATURES]: Define
> 	ARCH_INIT_CPU_FEATURES.
> 	(LIBC_START_MAIN): Call it.
> 	* sysdeps/unix/sysv/linux/aarch64/libc-start.c
> 	(__libc_start_main): Remove.
> 	(ARCH_INIT_CPU_FEATURES): New macro.
> 	* sysdeps/x86/libc-start.c (__libc_start_main): Remove.
> 	(ARCH_INIT_CPU_FEATURES): New macro.

LGTM with some nits about indentation.

> ---
>  csu/libc-start.c                             |  6 ++++++
>  sysdeps/unix/sysv/linux/aarch64/libc-start.c | 23 +++++------------------
>  sysdeps/x86/libc-start.c                     | 23 +++++------------------
>  3 files changed, 16 insertions(+), 36 deletions(-)
> 
> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index 9a56dcb..c2dd159 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -104,6 +104,10 @@ apply_irel (void)
>  # define MAIN_AUXVEC_PARAM
>  #endif
>  
> +#ifndef ARCH_INIT_CPU_FEATURES
> +# define ARCH_INIT_CPU_FEATURES()
> +#endif
> +
>  STATIC int LIBC_START_MAIN (int (*main) (int, char **, char **
>  					 MAIN_AUXVEC_DECL),
>  			    int argc,
> @@ -182,6 +186,8 @@ LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
>  
>    __tunables_init (__environ);
>  
> +  ARCH_INIT_CPU_FEATURES ();
> +
>    /* Perform IREL{,A} relocations.  */
>    apply_irel ();
>  
> diff --git a/sysdeps/unix/sysv/linux/aarch64/libc-start.c b/sysdeps/unix/sysv/linux/aarch64/libc-start.c
> index a5babd4..46041fe 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/libc-start.c
> +++ b/sysdeps/unix/sysv/linux/aarch64/libc-start.c
> @@ -16,26 +16,13 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#ifdef SHARED
> -# include <csu/libc-start.c>
> -# else
> -/* The main work is done in the generic function.  */
> -# define LIBC_START_DISABLE_INLINE
> -# define LIBC_START_MAIN generic_start_main
> -# include <csu/libc-start.c>
> +#ifndef SHARED
> +#include <ldsodefs.h>
>  # include <cpu-features.c>
>  
>  extern struct cpu_features _dl_aarch64_cpu_features;
>  
> -int
> -__libc_start_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> -		   int argc, char **argv,
> -		   __typeof (main) init,
> -		   void (*fini) (void),
> -		   void (*rtld_fini) (void), void *stack_end)
> -{
> -  init_cpu_features (&_dl_aarch64_cpu_features);
> -  return generic_start_main (main, argc, argv, init, fini, rtld_fini,
> -			     stack_end);
> -}
> +#define ARCH_INIT_CPU_FEATURES() init_cpu_features (&_dl_aarch64_cpu_features)
> +
>  #endif
> +# include <csu/libc-start.c>

Resulting indentation seems odd, I think it should be:

#ifndef SHARED
# include <ldsodefs.h>
# include <cpu-features.c>

extern struct cpu_features _dl_aarch64_cpu_features;

# define ARCH_INIT_CPU_FEATURES() init_cpu_features (&_dl_aarch64_cpu_features)

#endif
#include <csu/libc-start.c>

Same for x86.

> diff --git a/sysdeps/x86/libc-start.c b/sysdeps/x86/libc-start.c
> index 9a56adc..e11b490 100644
> --- a/sysdeps/x86/libc-start.c
> +++ b/sysdeps/x86/libc-start.c
> @@ -15,27 +15,14 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#ifdef SHARED
> -# include <csu/libc-start.c>
> -# else
> -/* The main work is done in the generic function.  */
> -# define LIBC_START_DISABLE_INLINE
> -# define LIBC_START_MAIN generic_start_main
> -# include <csu/libc-start.c>
> +#ifndef SHARED
> +#include <ldsodefs.h>
>  # include <cpu-features.h>
>  # include <cpu-features.c>
>  
>  extern struct cpu_features _dl_x86_cpu_features;
>  
> -int
> -__libc_start_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
> -		   int argc, char **argv,
> -		   __typeof (main) init,
> -		   void (*fini) (void),
> -		   void (*rtld_fini) (void), void *stack_end)
> -{
> -  init_cpu_features (&_dl_x86_cpu_features);
> -  return generic_start_main (main, argc, argv, init, fini, rtld_fini,
> -			     stack_end);
> -}
> +#define ARCH_INIT_CPU_FEATURES() init_cpu_features (&_dl_x86_cpu_features)
> +
>  #endif
> +# include <csu/libc-start.c>
> 

Re: [PATCH 7/7] aarch64: Allow overriding HWCAP_CPUID feature check using HWCAP_MASK

[Adhemerval Zanella]

On 11/05/2017 11:51, Siddhesh Poyarekar wrote:
> Now that LD_HWCAP_MASK (or glibc.tune.hwcap_mask) is read early enough
> to influence cpu feature check in aarch64, use it to influence
> multiarch selection.  Setting LD_HWCAP_MASK such that it clears
> HWCAP_CPUID will now disable multiarch for the binary.
> 
> 	* sysdeps/unix/sysv/linux/aarch64/cpu-features.c
> 	(init_cpu_features): Use glibc.tune.hwcap_mask.

LGTM.

> 
> Change-Id: Ia801ed6b8166b79a0cae184ee7417272f2f60593
> ---
>  sysdeps/unix/sysv/linux/aarch64/cpu-features.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
> index 7025062..0478fcc 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
> +++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
> @@ -18,18 +18,25 @@
>  
>  #include <cpu-features.h>
>  #include <sys/auxv.h>
> +#include <elf/dl-tunables.h>
>  
>  static inline void
>  init_cpu_features (struct cpu_features *cpu_features)
>  {
> -  if (GLRO(dl_hwcap) & HWCAP_CPUID)
> +#if HAVE_TUNABLES
> +  uint64_t hwcap_mask = TUNABLE_GET (glibc, tune, hwcap_mask, uint64_t);
> +#else
> +  uint64_t hwcap_mask = GLRO (dl_hwcap_mask);
> +#endif

I noted the hwcap_mask get logic is duplicated over various places.  Should we
define a helper macro for it?

> +
> +  uint64_t hwcap = GLRO (dl_hwcap) & hwcap_mask;
> +
> +  if (hwcap & HWCAP_CPUID)
>      {
>        register uint64_t id = 0;
>        asm volatile ("mrs %0, midr_el1" : "=r"(id));
>        cpu_features->midr_el1 = id;
>      }
>    else
> -    {
> -      cpu_features->midr_el1 = 0;
> -    }
> +    cpu_features->midr_el1 = 0;
>  }
> 

Re: [PATCH 5/7] tunables: Use glibc.tune.hwcap_mask tunable instead of _dl_hwcap_mask

[Siddhesh Poyarekar]

On Wednesday 17 May 2017 08:58 PM, Adhemerval Zanella wrote:
> I would recommend to incorporate the fix for LD_HWCAP_MASK on tst-end-setuid
> you sent before [1] to avoid this fail on some configuration (mine for
> instance).  Also I think we should open a bugzilla to iron out the
> elf/dl-hwcaps.c allocation scheme which is triggering this issue.
> 
> [1] https://sourceware.org/ml/libc-alpha/2017-05/msg00340.html

Pushed fix to the test case:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;a=commitdiff;h=ce79740bdbccea312df6cfcf70689efb57792fc9

and reported bug:

https://sourceware.org/bugzilla/show_bug.cgi?id=21502

Siddhesh

> 
> 
>> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
>> index f26a8b2..695ac24 100644
>> --- a/sysdeps/generic/ldsodefs.h
>> +++ b/sysdeps/generic/ldsodefs.h
>> @@ -515,8 +515,10 @@ struct rtld_global_ro
>>    /* Mask for hardware capabilities that are available.  */
>>    EXTERN uint64_t _dl_hwcap;
>>  
>> +#if !HAVE_TUNABLES
>>    /* Mask for important hardware capabilities we honour. */
>>    EXTERN uint64_t _dl_hwcap_mask;
>> +#endif
>>  
>>  #ifdef HAVE_AUX_VECTOR
>>    /* Pointer to the auxv list supplied to the program at startup.  */
>> diff --git a/sysdeps/sparc/sparc32/dl-machine.h b/sysdeps/sparc/sparc32/dl-machine.h
>> index cf7272f..6923e47 100644
>> --- a/sysdeps/sparc/sparc32/dl-machine.h
>> +++ b/sysdeps/sparc/sparc32/dl-machine.h
>> @@ -39,7 +39,12 @@ elf_machine_matches_host (const Elf32_Ehdr *ehdr)
>>        /* XXX The following is wrong!  Dave Miller rejected to implement it
>>  	 correctly.  If this causes problems shoot *him*!  */
>>  #ifdef SHARED
>> -      return GLRO(dl_hwcap) & GLRO(dl_hwcap_mask) & HWCAP_SPARC_V9;
>> +# if HAVE_TUNABLES
>> +      uint64_t hwcap_mask = TUNABLE_GET (glibc, tune, hwcap_mask);
> 
> You are missing '#include <elf/dl-tunables.h>' and the final type argument for
> TUNABLE_GET (it fails to build for sparcv9 with tunable enabled).
> 
>> +# else
>> +      uint64_t hwcap_mask = GLRO(dl_hwcap_mask);
>> +# endif
>> +      return GLRO(dl_hwcap) & hwcap_mask & HWCAP_SPARC_V9;
>>  #else
>>        return GLRO(dl_hwcap) & HWCAP_SPARC_V9;
>>  #endif
>> diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
>> index b481f50..4fe58bf 100644
>> --- a/sysdeps/x86/cpu-features.c
>> +++ b/sysdeps/x86/cpu-features.c
>> @@ -316,7 +316,11 @@ no_cpuid:
>>    /* Reuse dl_platform, dl_hwcap and dl_hwcap_mask for x86.  */
>>    GLRO(dl_platform) = NULL;
>>    GLRO(dl_hwcap) = 0;
>> +#if !HAVE_TUNABLES
>> +  /* The glibc.tune.hwcap_mask tunable is initialized already, so no need to do
>> +     this.  */
>>    GLRO(dl_hwcap_mask) = HWCAP_IMPORTANT;
>> +#endif
>>  
>>  # ifdef __x86_64__
>>    if (cpu_features->kind == arch_kind_intel)
>>

Re: [PATCH 7/7] aarch64: Allow overriding HWCAP_CPUID feature check using HWCAP_MASK

[Siddhesh Poyarekar]

On Thursday 18 May 2017 12:52 AM, Adhemerval Zanella wrote:
> I noted the hwcap_mask get logic is duplicated over various places.  Should we
> define a helper macro for it?

Sorry I missed this suggestion, I have posted a patch for it now that
applies on top of my patchset.

https://sourceware.org/ml/libc-alpha/2017-05/msg00578.html

Siddhesh