Re: [PATCH] PR libgcc/60790: Avoid IFUNC resolver access to uninitialized data

[Florian Weimer <fw at deneb dot enyo dot de>]

* Jeff Law:

> On 03/29/2018 08:00 AM, Florian Weimer wrote:
>> This patch performs lazy initialization of the relevant CPUID feature
>> register value.  It will needlessly invoke the CPUID determination code
>> on architectures which lack CPUID support or support for the feature
>> register, but I think it's not worth to avoid the complexity for that.
>> 
>> I verified manually that the CMPXCHG16B implementation is still selected
>> for a 128-bit load after the change.  I don't know how to write an
>> automated test for that.
>> 
>> Thanks,
>> Florian
>> 
>> pr60790.patch
>> 
>> 
>> Index: libatomic/ChangeLog
>> ===================================================================
>> --- libatomic/ChangeLog	(revision 258952)
>> +++ libatomic/ChangeLog	(working copy)
>> @@ -1,3 +1,18 @@
>> +2018-03-29  Florian Weimer  <fweimer@tor.usersys.redhat.com>
>> +
>> +	PR libgcc/60790
>> +	x86: Do not assume ELF constructors run before IFUNC resolvers.
>> +	* config/x86/host-config.h (libat_feat1_ecx, libat_feat1_edx):
>> +	Remove declarations.
>> +	(__libat_feat1, __libat_feat1_init): Declare.
>> +	(FEAT1_REGISTER): Define.
>> +	(load_feat1): New function.
>> +	(IFUNC_COND_1): Adjust.
>> +	* config/x86/init.c (libat_feat1_ecx, libat_feat1_edx)
>> +	(init_cpuid): Remove definitions.
>> +	(__libat_feat1): New variable.
>> +	(__libat_feat1_init): New function.
> OK.

Here is the backport to gcc-8-branch.

libatomic/ChangeLog

	PR libgcc/60790
	x86: Do not assume ELF constructors run before IFUNC resolvers.
	* config/x86/host-config.h (libat_feat1_ecx, libat_feat1_edx):
	Remove declarations.
	(__libat_feat1, __libat_feat1_init): Declare.
	(FEAT1_REGISTER): Define.
	(load_feat1): New function.
	(IFUNC_COND_1): Adjust.
	* config/x86/init.c (libat_feat1_ecx, libat_feat1_edx)
	(init_cpuid): Remove definitions.
	(__libat_feat1): New variable.
	(__libat_feat1_init): New function.

Index: libatomic/config/x86/host-config.h
===================================================================
--- libatomic/config/x86/host-config.h	(revision 264990)
+++ libatomic/config/x86/host-config.h	(working copy)
@@ -25,13 +25,39 @@
 #if HAVE_IFUNC
 #include <cpuid.h>
 
-extern unsigned int libat_feat1_ecx HIDDEN;
-extern unsigned int libat_feat1_edx HIDDEN;
+#ifdef __x86_64__
+# define FEAT1_REGISTER ecx
+#else
+# define FEAT1_REGISTER edx
+#endif
 
+/* Value of the CPUID feature register FEAT1_REGISTER for the cmpxchg
+   bit for IFUNC_COND1 below.  */
+extern unsigned int __libat_feat1 HIDDEN;
+
+/* Initialize libat_feat1 and return its value.  */
+unsigned int __libat_feat1_init (void) HIDDEN;
+
+/* Return the value of the relevant feature register for the relevant
+   cmpxchg bit, or 0 if there is no CPUID support.  */
+static inline unsigned int
+__attribute__ ((const))
+load_feat1 (void)
+{
+  /* See the store in __libat_feat1_init.  */
+  unsigned int feat1 = __atomic_load_n (&__libat_feat1, __ATOMIC_RELAXED);
+  if (feat1 == 0)
+    /* Assume that initialization has not happened yet.  This may get
+       called repeatedly if the CPU does not have any feature bits at
+       all.  */
+    feat1 = __libat_feat1_init ();
+  return feat1;
+}
+
 #ifdef __x86_64__
-# define IFUNC_COND_1	(libat_feat1_ecx & bit_CMPXCHG16B)
+# define IFUNC_COND_1	(load_feat1 () & bit_CMPXCHG16B)
 #else
-# define IFUNC_COND_1	(libat_feat1_edx & bit_CMPXCHG8B)
+# define IFUNC_COND_1	(load_feat1 () & bit_CMPXCHG8B)
 #endif
 
 #ifdef __x86_64__
Index: libatomic/config/x86/init.c
===================================================================
--- libatomic/config/x86/init.c	(revision 264990)
+++ libatomic/config/x86/init.c	(working copy)
@@ -26,13 +26,17 @@
 
 #if HAVE_IFUNC
 
-unsigned int libat_feat1_ecx, libat_feat1_edx;
+unsigned int __libat_feat1;
 
-static void __attribute__((constructor))
-init_cpuid (void)
+unsigned int
+__libat_feat1_init (void)
 {
-  unsigned int eax, ebx;
-  __get_cpuid (1, &eax, &ebx, &libat_feat1_ecx, &libat_feat1_edx);
+  unsigned int eax, ebx, ecx, edx;
+  FEAT1_REGISTER = 0;
+  __get_cpuid (1, &eax, &ebx, &ecx, &edx);
+  /* See the load in load_feat1.  */
+  __atomic_store_n (&__libat_feat1, FEAT1_REGISTER, __ATOMIC_RELAXED);
+  return FEAT1_REGISTER;
 }
 
 #endif /* HAVE_IFUNC */
Index: .
===================================================================
--- .	(revision 264990)
+++ .	(working copy)

Property changes on: .
___________________________________________________________________
Modified: svn:mergeinfo
## -0,0 +0,1 ##
   Merged /trunk:r260603