Fix _dl_profile_fixup data-dependency issue (Bug 23690)

There is a data-dependency between the fields of struct l_reloc_result
and the field used as the initialization guard. Users of the guard
expect writes to the structure to be observable when they also observe
the guard initialized. The solution for this problem is to use an acquire
and release load and store to ensure previous writes to the structure are
observable if the guard is initialized.

The previous implementation used DL_FIXUP_VALUE_ADDR (l_reloc_result->addr)
as the initialization guard, making it impossible for some architectures
to load and store it atomically, i.e. hppa and ia64, due to its larger size.

This commit adds an unsigned int to l_reloc_result to be used as the new
initialization guard of the struct, making it possible to load and store
it atomically in all architectures. The fix ensures that the values
observed in l_reloc_result are consistent and do not lead to crashes.
The algorithm is documented in the code in elf/dl-runtime.c
(_dl_profile_fixup). Not all data races have been eliminated.

Tested with build-many-glibcs and on powerpc, powerpc64, and powerpc64le.

	[BZ #23690]
	* elf/dl-runtime.c (_dl_profile_fixup): Guarantee memory
	modification order when accessing reloc_result->addr.
	* include/link.h (reloc_result): Add field init.
	* nptl/Makefile (tests): Add tst-audit-threads.
	(modules-names): Add tst-audit-threads-mod1 and
	tst-audit-threads-mod2.
	Add rules to build tst-audit-threads.
	* nptl/tst-audit-threads-mod1.c: New file.
	* nptl/tst-audit-threads-mod2.c: Likewise.
	* nptl/tst-audit-threads.c: Likewise.
	* nptl/tst-audit-threads.h: Likewise.

Signed-off-by: Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com>
Reviewed-by: Carlos O'Donell <carlos@redhat.com>

1 files changed, 43 insertions, 5 deletions

diff --git a/elf/dl-runtime.c b/elf/dl-runtime.c
index 63bbc89776..3d2f4a7a76 100644
--- a/elf/dl-runtime.c
+++ b/elf/dl-runtime.c
@@ -183,10 +183,36 @@ _dl_profile_fixup (
   /* This is the address in the array where we store the result of previous
      relocations.  */
   struct reloc_result *reloc_result = &l->l_reloc_result[reloc_index];
-  DL_FIXUP_VALUE_TYPE *resultp = &reloc_result->addr;
 
-  DL_FIXUP_VALUE_TYPE value = *resultp;
-  if (DL_FIXUP_VALUE_CODE_ADDR (value) == 0)
+ /* CONCURRENCY NOTES:
+
+  Multiple threads may be calling the same PLT sequence and with
+  LD_AUDIT enabled they will be calling into _dl_profile_fixup to
+  update the reloc_result with the result of the lazy resolution.
+  The reloc_result guard variable is reloc_init, and we use
+  acquire/release loads and store to it to ensure that the results of
+  the structure are consistent with the loaded value of the guard.
+  This does not fix all of the data races that occur when two or more
+  threads read reloc_result->reloc_init with a value of zero and read
+  and write to that reloc_result concurrently.  The expectation is
+  generally that while this is a data race it works because the
+  threads write the same values.  Until the data races are fixed
+  there is a potential for problems to arise from these data races.
+  The reloc result updates should happen in parallel but there should
+  be an atomic RMW which does the final update to the real result
+  entry (see bug 23790).
+
+  The following code uses reloc_result->init set to 0 to indicate if it is
+  the first time this object is being relocated, otherwise 1 which
+  indicates the object has already been relocated.
+
+  Reading/Writing from/to reloc_result->reloc_init must not happen
+  before previous writes to reloc_result complete as they could
+  end-up with an incomplete struct.  */
+  DL_FIXUP_VALUE_TYPE value;
+  unsigned int init = atomic_load_acquire (&reloc_result->init);
+
+  if (init == 0)
     {
       /* This is the first time we have to relocate this object.  */
       const ElfW(Sym) *const symtab
@@ -346,19 +372,31 @@ _dl_profile_fixup (
 
       /* Store the result for later runs.  */
       if (__glibc_likely (! GLRO(dl_bind_not)))
-	*resultp = value;
+	{
+	  reloc_result->addr = value;
+	  /* Guarantee all previous writes complete before
+	     init is updated.  See CONCURRENCY NOTES earlier  */
+	  atomic_store_release (&reloc_result->init, 1);
+	}
+      init = 1;
     }
+  else
+    value = reloc_result->addr;
 
   /* By default we do not call the pltexit function.  */
   long int framesize = -1;
 
+
 #ifdef SHARED
   /* Auditing checkpoint: report the PLT entering and allow the
      auditors to change the value.  */
-  if (DL_FIXUP_VALUE_CODE_ADDR (value) != 0 && GLRO(dl_naudit) > 0
+  if (GLRO(dl_naudit) > 0
       /* Don't do anything if no auditor wants to intercept this call.  */
       && (reloc_result->enterexit & LA_SYMB_NOPLTENTER) == 0)
     {
+      /* Sanity check:  DL_FIXUP_VALUE_CODE_ADDR (value) should have been
+	 initialized earlier in this function or in another thread.  */
+      assert (DL_FIXUP_VALUE_CODE_ADDR (value) != 0);
       ElfW(Sym) *defsym = ((ElfW(Sym) *) D_PTR (reloc_result->bound,
 						l_info[DT_SYMTAB])
 			   + reloc_result->boundndx);