Re: V3 [PATCH] x86: Initialize CPU info via IFUNC relocation [BZ 26203]

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

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

On Tue, Sep 29, 2020 at 12:53 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu via Libc-alpha:
>
> > Here is the updated patch.  Does it address your concerns?
>
> “git am” fails to apply it for me:
>
> Applying: x86: Initialize CPU info via IFUNC relocation [BZ 26203]
> error: corrupt patch at line 41
> Patch failed at 0001 x86: Initialize CPU info via IFUNC relocation [BZ 26203]
>
> The patch program does not like it, either:
>
> patching file sysdeps/i386/dl-machine.h
> patching file sysdeps/x86/cacheinfo.c
> patch: **** malformed patch at line 73: @@ -770,6 +769,8 @@ init_cacheinfo (void)
>
> I don't think it's the Red Hat mail system that's at fault this time. 8-/
>

Here is the updated patch with the correct commit log.

Thanks.

-- 
H.J.

[-- Attachment #2: 0001-x86-Initialize-CPU-info-via-IFUNC-relocation-BZ-2620.patch --]
[-- Type: text/x-patch, Size: 8732 bytes --]

From 8ee884ce6c1a52c3b88159ecdef3f8401743ad3c Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sat, 4 Jul 2020 06:35:49 -0700
Subject: [PATCH] x86: Initialize CPU info via IFUNC relocation [BZ 26203]

X86 CPU features in ld.so are initialized by init_cpu_features, which is
invoked by DL_PLATFORM_INIT from _dl_sysdep_start.  But when ld.so is
loaded by static executable, DL_PLATFORM_INIT is never called.  Also
x86 cache info in libc.o and libc.a is initialized by a constructor
which may be called too late.  Since some fields in _rtld_global_ro
in ld.so are initialized by dynamic relocation, we can also initialize
x86 CPU features in _rtld_global_ro in ld.so and cache info in libc.so
by initializing dummy function pointers in ld.so and libc.so via IFUNC
relocation.

Key points:

1. IFUNC is always supported, independent of --enable-multi-arch or
--disable-multi-arch.  Linker generates IFUNC relocations from input
IFUNC objects and ld.so performs IFUNC relocations.
2. There are no IFUNC dependencies in ld.so before dynamic relocation
have been performed,
3. The x86 CPU features in ld.so is initialized by DL_PLATFORM_INIT
in dynamic executable and by IFUNC relocation in dlopen in static
executable.
4. The x86 cache info in libc.o is initialized by IFUNC relocation.
5. In libc.a, both x86 CPU features and cache info are initialized from
ARCH_INIT_CPU_FEATURES, not by IFUNC relocation, before __libc_early_init
is called.

Note: _dl_x86_init_cpu_features can be called more than once from
DL_PLATFORM_INIT and during relocation in ld.so.
---
 sysdeps/i386/dl-machine.h          |  7 +++----
 sysdeps/x86/cacheinfo.c            | 21 +++++++++++++++++++--
 sysdeps/x86/cpu-features.c         | 12 +++++++++++-
 sysdeps/x86/dl-get-cpu-features.c  | 27 ++++++++++++++++++++++++++-
 sysdeps/x86/include/cpu-features.h |  1 +
 sysdeps/x86/libc-start.c           |  2 +-
 sysdeps/x86_64/dl-machine.h        |  7 +++----
 7 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
index 0f08079e48..bdc21d1a3c 100644
--- a/sysdeps/i386/dl-machine.h
+++ b/sysdeps/i386/dl-machine.h
@@ -25,7 +25,6 @@
 #include <sysdep.h>
 #include <tls.h>
 #include <dl-tlsdesc.h>
-#include <cpu-features.c>
 
 /* Return nonzero iff ELF header is compatible with the running host.  */
 static inline int __attribute__ ((unused))
@@ -248,9 +247,9 @@ static inline void __attribute__ ((unused))
 dl_platform_init (void)
 {
 #if IS_IN (rtld)
-  /* init_cpu_features has been called early from __libc_start_main in
-     static executable.  */
-  init_cpu_features (&GLRO(dl_x86_cpu_features));
+  /* _dl_x86_init_cpu_features is a wrapper for init_cpu_features which
+     has been called early from __libc_start_main in static executable.  */
+  _dl_x86_init_cpu_features ();
 #else
   if (GLRO(dl_platform) != NULL && *GLRO(dl_platform) == '\0')
     /* Avoid an empty string which would disturb us.  */
diff --git a/sysdeps/x86/cacheinfo.c b/sysdeps/x86/cacheinfo.c
index dadec5d58f..65ab29123d 100644
--- a/sysdeps/x86/cacheinfo.c
+++ b/sysdeps/x86/cacheinfo.c
@@ -16,7 +16,9 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#if IS_IN (libc)
+/* NB: In libc.a, this file is included in libc-static.c.  In libc.so,
+   this file is standalone.  */
+#if IS_IN (libc) && (defined SHARED || defined _PRIVATE_CPU_FEATURES_H)
 
 #include <assert.h>
 #include <stdbool.h>
@@ -756,7 +758,6 @@ intel_bug_no_cache_info:
 
 
 static void
-__attribute__((constructor))
 init_cacheinfo (void)
 {
   /* Find out what brand of processor.  */
@@ -770,6 +771,12 @@ init_cacheinfo (void)
   unsigned int threads = 0;
   const struct cpu_features *cpu_features = __get_cpu_features ();
 
+  /* NB: In libc.so, cpu_features is defined in ld.so and is initialized
+     by DL_PLATFORM_INIT or IFUNC relocation before init_cacheinfo is
+     called by IFUNC relocation.  In libc.a, init_cacheinfo is called
+     from init_cpu_features by ARCH_INIT_CPU_FEATURES.  */
+  assert (cpu_features->basic.kind != arch_kind_unknown);
+
   if (cpu_features->basic.kind == arch_kind_intel)
     {
       data = handle_intel (_SC_LEVEL1_DCACHE_SIZE, cpu_features);
@@ -900,4 +907,14 @@ init_cacheinfo (void)
 # endif
 }
 
+# ifdef SHARED
+/* NB: In libc.so, call init_cacheinfo by initializing a dummy function
+   pointer via IFUNC relocation after CPU features in ld.so have been
+   initialized by DL_PLATFORM_INIT or IFUNC relocation.  */
+extern void __x86_cacheinfo (void) attribute_hidden;
+const void (*__x86_cacheinfo_p) (void) attribute_hidden
+  = __x86_cacheinfo;
+
+__ifunc (__x86_cacheinfo, __x86_cacheinfo, NULL, void, init_cacheinfo);
+# endif
 #endif
diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
index 6551df19c0..874ce9c886 100644
--- a/sysdeps/x86/cpu-features.c
+++ b/sysdeps/x86/cpu-features.c
@@ -16,7 +16,12 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <cpuid.h>
+#ifdef SHARED
+/* NB: Workaround the GCC <cpuid.h> bug:
+	https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96238
+ */
+# include <cpuid.h>
+#endif
 #include <cpu-features.h>
 #include <dl-hwcap.h>
 #include <libc-pointer-arith.h>
@@ -746,4 +751,9 @@ no_cpuid:
 # endif
     }
 #endif
+
+#ifndef SHARED
+  /* NB: In libc.a, call init_cacheinfo.  */
+  init_cacheinfo ();
+#endif
 }
diff --git a/sysdeps/x86/dl-get-cpu-features.c b/sysdeps/x86/dl-get-cpu-features.c
index 5f9e46b0c6..349472d99f 100644
--- a/sysdeps/x86/dl-get-cpu-features.c
+++ b/sysdeps/x86/dl-get-cpu-features.c
@@ -1,4 +1,4 @@
-/* This file is part of the GNU C Library.
+/* Initialize CPU feature data via IFUNC relocation.
    Copyright (C) 2015-2020 Free Software Foundation, Inc.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -18,6 +18,31 @@
 
 #include <ldsodefs.h>
 
+#ifdef SHARED
+# include <cpu-features.c>
+
+/* NB: Normally, DL_PLATFORM_INIT calls init_cpu_features to initialize
+   CPU features in dynamic executable.  But when loading ld.so inside of
+   static executable, DL_PLATFORM_INIT isn't called and IFUNC relocation
+   is used to call init_cpu_features.  In static executable, it is called
+   once by IFUNC relocation.  In dynamic executable, it is called twice
+   by DL_PLATFORM_INIT and by IFUNC relocation.  */
+extern void __x86_cpu_features (void) attribute_hidden;
+const void (*__x86_cpu_features_p) (void) attribute_hidden
+  = __x86_cpu_features;
+
+void
+_dl_x86_init_cpu_features (void)
+{
+  struct cpu_features *cpu_features = __get_cpu_features ();
+  if (cpu_features->basic.kind == arch_kind_unknown)
+    init_cpu_features (cpu_features);
+}
+
+__ifunc (__x86_cpu_features, __x86_cpu_features, NULL, void,
+	 _dl_x86_init_cpu_features);
+#endif
+
 #undef __x86_get_cpu_features
 
 const struct cpu_features *
diff --git a/sysdeps/x86/include/cpu-features.h b/sysdeps/x86/include/cpu-features.h
index dcf29b6fe8..f62be0b9b3 100644
--- a/sysdeps/x86/include/cpu-features.h
+++ b/sysdeps/x86/include/cpu-features.h
@@ -159,6 +159,7 @@ struct cpu_features
 /* Unused for x86.  */
 #  define INIT_ARCH()
 #  define __x86_get_cpu_features(max) (&GLRO(dl_x86_cpu_features))
+extern void _dl_x86_init_cpu_features (void) attribute_hidden;
 # endif
 
 # ifdef __x86_64__
diff --git a/sysdeps/x86/libc-start.c b/sysdeps/x86/libc-start.c
index 875bb93e55..4f72fcf397 100644
--- a/sysdeps/x86/libc-start.c
+++ b/sysdeps/x86/libc-start.c
@@ -20,7 +20,7 @@
    PIE.  */
 # include <startup.h>
 # include <ldsodefs.h>
-# include <cpu-features.h>
+# include <cacheinfo.c>
 # include <cpu-features.c>
 
 extern struct cpu_features _dl_x86_cpu_features;
diff --git a/sysdeps/x86_64/dl-machine.h b/sysdeps/x86_64/dl-machine.h
index ca73d8fef9..bb93c7c6ab 100644
--- a/sysdeps/x86_64/dl-machine.h
+++ b/sysdeps/x86_64/dl-machine.h
@@ -26,7 +26,6 @@
 #include <sysdep.h>
 #include <tls.h>
 #include <dl-tlsdesc.h>
-#include <cpu-features.c>
 
 /* Return nonzero iff ELF header is compatible with the running host.  */
 static inline int __attribute__ ((unused))
@@ -223,9 +222,9 @@ static inline void __attribute__ ((unused))
 dl_platform_init (void)
 {
 #if IS_IN (rtld)
-  /* init_cpu_features has been called early from __libc_start_main in
-     static executable.  */
-  init_cpu_features (&GLRO(dl_x86_cpu_features));
+  /* _dl_x86_init_cpu_features is a wrapper for init_cpu_features which
+     has been called early from __libc_start_main in static executable.  */
+  _dl_x86_init_cpu_features ();
 #else
   if (GLRO(dl_platform) != NULL && *GLRO(dl_platform) == '\0')
     /* Avoid an empty string which would disturb us.  */
-- 
2.26.2