This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] aarch64: Use explicit offsets in _dl_tlsdesc_dynamic


On 12/01/2016 04:06 PM, Szabolcs Nagy wrote:
On 01/12/16 14:49, Florian Weimer wrote:
Commit 389d1f1b232b3d6b9d73ee2c50e543ace6675621 (“Partial ILP32
support for aarch64”) broke dynamic TLS support because a load
offset changed:

 0000000000000030 <_dl_tlsdesc_dynamic>:
   30:  a9bc7bfd        stp     x29, x30, [sp,#-64]!
   34:  910003fd        mov     x29, sp
   38:  a9020be1        stp     x1, x2, [sp,#32]
   3c:  a90313e3        stp     x3, x4, [sp,#48]
   40:  d53bd044        mrs     x4, tpidr_el0
   44:  c8dffc1f        ldar    xzr, [x0]
   48:  f9400401        ldr     x1, [x0,#8]
   4c:  f9400080        ldr     x0, [x4]
   50:  f9400823        ldr     x3, [x1,#16]
   54:  f9400002        ldr     x2, [x0]
   58:  eb02007f        cmp     x3, x2
   5c:  540001a8        b.hi    90 <_dl_tlsdesc_dynamic+0x60>
   60:  f9400022        ldr     x2, [x1]
   64:  8b021000        add     x0, x0, x2, lsl #4
   68:  f9400000        ldr     x0, [x0]
   6c:  b100041f        cmn     x0, #0x1
   70:  54000100        b.eq    90 <_dl_tlsdesc_dynamic+0x60>
-  74:  f9400421        ldr     x1, [x1,#8]
+  74:  f9400821        ldr     x1, [x1,#16]
   78:  8b010000        add     x0, x0, x1
…


nice catch.

It totally broke our Fedora builders because we still carry a patch which disables static TLS allocation.

This commit introduces explicit struct offsets, generated
from the C headers, fixing the regression.

2016-12-01  Florian Weimer  <fweimer@redhat.com>

	* sysdeps/aarch64/tlsdesc.sym (TCBHEAD_DTV, DTV_COUNTER)
	(TLS_DTV_UNALLOCATED): Add.
	* sysdeps/aarch64/dl-tlsdesc.S (_dl_tlsdesc_dynamic): Use explicit
	offsets.

diff --git a/sysdeps/aarch64/dl-tlsdesc.S b/sysdeps/aarch64/dl-tlsdesc.S
index 42fa943..37a0211 100644
--- a/sysdeps/aarch64/dl-tlsdesc.S
+++ b/sysdeps/aarch64/dl-tlsdesc.S
@@ -21,7 +21,7 @@
 #include <sysdep.h>
 #include <tls.h>
 #include "tlsdesc.h"
-
+	

whitespace change

Yeah, sorry about that.  Will fix.

I have a test case which triggers a crash on aarch64, but I'm not yet sure if it actually covers this bug. It fails even with the fix above. valground still shows an OOB write in TLS data:

==16070== Invalid write of size 8
==16070==    at 0x4897C9C: init_one_static_tls (allocatestack.c:1196)
==16070==    by 0x4897C9C: __pthread_init_static_tls (allocatestack.c:1213)
==16070==    by 0x18A1CB: _dl_try_allocate_static_tls (dl-reloc.c:106)
==16070==    by 0x19383F: _dl_tlsdesc_resolve_rela_fixup (tlsdesc.c:104)
==16070==    by 0x193B03: _dl_tlsdesc_resolve_rela (dl-tlsdesc.S:288)
==16070==    by 0x193B03: _dl_tlsdesc_resolve_rela (dl-tlsdesc.S:288)
==16070==    by 0x193B03: _dl_tlsdesc_resolve_rela (dl-tlsdesc.S:288)
==16070==    by 0x193B03: _dl_tlsdesc_resolve_rela (dl-tlsdesc.S:288)
==16070==    by 0x193B03: _dl_tlsdesc_resolve_rela (dl-tlsdesc.S:288)
==16070==    by 0x193B03: _dl_tlsdesc_resolve_rela (dl-tlsdesc.S:288)
==16070==    by 0x193B03: _dl_tlsdesc_resolve_rela (dl-tlsdesc.S:288)
==16070==    by 0x193B03: _dl_tlsdesc_resolve_rela (dl-tlsdesc.S:288)
==16070==    by 0x193B03: _dl_tlsdesc_resolve_rela (dl-tlsdesc.S:288)
==16070==  Address 0x5240158 is 8 bytes after a block of size 272 alloc'd
==16070==    at 0x4835D4C: calloc (vg_replace_malloc.c:711)
==16070==    by 0x18F9CF: allocate_dtv (dl-tls.c:322)
==16070==    by 0x190117: _dl_allocate_tls (dl-tls.c:570)
==16070==    by 0x4898D4B: allocate_stack (allocatestack.c:578)
==16070==    by 0x4898D4B: pthread_create@@GLIBC_2.17 (pthread_create.c:539)
==16070==    by 0x401CCB: xpthread_create (test-skeleton.c:691)
==16070==    by 0x401CCB: do_test (tst-tls-manydynamic.c:97)
==16070==    by 0x4018CF: main (test-skeleton.c:539)

I need to check if this happens before the ILP32 enablement patch, too.

Florian
aarch64: Use explicit offsets in _dl_tlsdesc_dynamic

Commit 389d1f1b232b3d6b9d73ee2c50e543ace6675621 (â??Partial ILP32
support for aarch64â??) broke dynamic TLS support because a load
offset changed:

 0000000000000030 <_dl_tlsdesc_dynamic>:
   30:  a9bc7bfd        stp     x29, x30, [sp,#-64]!
   34:  910003fd        mov     x29, sp
   38:  a9020be1        stp     x1, x2, [sp,#32]
   3c:  a90313e3        stp     x3, x4, [sp,#48]
   40:  d53bd044        mrs     x4, tpidr_el0
   44:  c8dffc1f        ldar    xzr, [x0]
   48:  f9400401        ldr     x1, [x0,#8]
   4c:  f9400080        ldr     x0, [x4]
   50:  f9400823        ldr     x3, [x1,#16]
   54:  f9400002        ldr     x2, [x0]
   58:  eb02007f        cmp     x3, x2
   5c:  540001a8        b.hi    90 <_dl_tlsdesc_dynamic+0x60>
   60:  f9400022        ldr     x2, [x1]
   64:  8b021000        add     x0, x0, x2, lsl #4
   68:  f9400000        ldr     x0, [x0]
   6c:  b100041f        cmn     x0, #0x1
   70:  54000100        b.eq    90 <_dl_tlsdesc_dynamic+0x60>
-  74:  f9400421        ldr     x1, [x1,#8]
+  74:  f9400821        ldr     x1, [x1,#16]
   78:  8b010000        add     x0, x0, x1
â?¦

This commit introduces explicit struct offsets, generated
from the C headers, fixing the regression.  It includes a new
test with many dynamic TLS variables, forcing full dynamic TLS
allocation, which provides test coverage for the bug.

2016-12-02  Florian Weimer  <fweimer@redhat.com>

	* elf/Makefile [build-shared] (tests): Add tst-latepthread.
	(one-hundred, tst-tls-many-dynamic-modules): Define.
	(modules-names): Add $(tst-tls-many-dynamic-modules).
	(tst-tls-manydynamic%mod.os): Build with special preprocessor
	macros.
	(tst-tls-manydynamic): Link against libdl, libpthread.
	(tst-tls-manydynamic.out): The test needs the test modules at run
	time.
	* elf/tst-tls-manydynamic.c: New file.
	* elf/tst-tls-manydynamic.h: Likewise.
	* elf/tst-tls-manydynamicmod.c: Likewise.

2016-12-01  Florian Weimer  <fweimer@redhat.com>

	* sysdeps/aarch64/tlsdesc.sym (TCBHEAD_DTV, DTV_COUNTER)
	(TLS_DTV_UNALLOCATED): Add.
	* sysdeps/aarch64/dl-tlsdesc.S (_dl_tlsdesc_dynamic): Use explicit
	offsets.

diff --git a/elf/Makefile b/elf/Makefile
index 33b003b..0d6f691 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -152,7 +152,7 @@ tests += loadtest restest1 preloadtest loadfail multiload origtest resolvfail \
 	 tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
 	 tst-ptrguard1 tst-tlsalign tst-tlsalign-extern tst-nodelete-opened \
 	 tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
-	 tst-latepthread
+	 tst-latepthread tst-tls-manydynamic
 #	 reldep9
 ifeq ($(build-hardcoded-path-in-tests),yes)
 tests += tst-dlopen-aout
@@ -173,6 +173,10 @@ tlsmod17a-suffixes = 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
 tlsmod18a-suffixes = 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
 tlsmod17a-modules = $(addprefix tst-tlsmod17a, $(tlsmod17a-suffixes))
 tlsmod18a-modules = $(addprefix tst-tlsmod18a, $(tlsmod17a-suffixes))
+one-hundred = $(foreach x,0 1 2 3 4 5 6 7 8 9, \
+  0$x 1$x 2$x 3$x 4$x 5$x 6$x 7$x 8$x 9$x)
+tst-tls-many-dynamic-modules := \
+  $(foreach n,$(one-hundred),tst-tls-manydynamic$(n)mod)
 extra-test-objs += $(tlsmod17a-modules:=.os) $(tlsmod18a-modules:=.os) \
 		   tst-tlsalign-vars.o
 test-extras += tst-tlsmod17a tst-tlsmod18a tst-tlsalign-vars
@@ -227,7 +231,7 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-tlsalign-lib tst-nodelete-opened-lib tst-nodelete2mod \
 		tst-audit11mod1 tst-audit11mod2 tst-auditmod11 \
 		tst-audit12mod1 tst-audit12mod2 tst-audit12mod3 tst-auditmod12 \
-		tst-latepthreadmod
+		tst-latepthreadmod $(tst-tls-many-dynamic-modules)
 ifeq (yes,$(have-mtls-dialect-gnu2))
 tests += tst-gnu2-tls1
 modules-names += tst-gnu2-tls1mod
@@ -1275,6 +1279,15 @@ $(objpfx)tst-latepthreadmod.so: $(shared-thread-library)
 $(objpfx)tst-latepthread: $(libdl)
 $(objpfx)tst-latepthread.out: $(objpfx)tst-latepthreadmod.so
 
+# The test modules are parameterized by preprocessor macros.
+$(patsubst %,$(objpfx)%.os,$(tst-tls-many-dynamic-modules)): \
+  $(objpfx)tst-tls-manydynamic%mod.os : tst-tls-manydynamicmod.c
+	$(compile-command.c) \
+	  -DNAME=tls_global_$* -DSETTER=set_value_$* -DGETTER=get_value_$*
+$(objpfx)tst-tls-manydynamic: $(libdl) $(shared-thread-library)
+$(objpfx)tst-tls-manydynamic.out: \
+  $(patsubst %,$(objpfx)%.so,$(tst-tls-many-dynamic-modules))
+
 tst-prelink-ENV = LD_TRACE_PRELINKING=1
 
 $(objpfx)tst-prelink-conflict.out: $(objpfx)tst-prelink.out
diff --git a/elf/tst-tls-manydynamic.c b/elf/tst-tls-manydynamic.c
new file mode 100644
index 0000000..2f0f5f7
--- /dev/null
+++ b/elf/tst-tls-manydynamic.c
@@ -0,0 +1,132 @@
+/* Test with many dynamic TLS variables.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This test intends to exercise dynamic TLS variable allocation.  It
+   achieves this by combining dlopen (to avoid static TLS allocation
+   after static TLS resizing), many DSOs with a large variable (to
+   exceed the static TLS reserve), and an already-running thread (to
+   force full dynamic TLS initialization).  */
+
+#include "tst-tls-manydynamic.h"
+
+#include <dlfcn.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+static int do_test (void);
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
+
+void *handles[COUNT];
+set_value_func set_value_funcs[COUNT];
+get_value_func get_value_funcs[COUNT];
+
+static void
+init_functions (void)
+{
+  for (int i = 0; i < COUNT; ++i)
+    {
+      /* Open the module.  */
+      {
+        char soname[100];
+        snprintf (soname, sizeof (soname), "tst-tls-manydynamic%02dmod.so", i);
+        handles[i] = dlopen (soname, RTLD_LAZY);
+        if (handles[i] == NULL)
+          {
+            printf ("error: dlopen failed: %s\n", dlerror ());
+            exit (1);
+          }
+      }
+
+      /* Obtain the setter function.  */
+      {
+        char fname[100];
+        snprintf (fname, sizeof (fname), "set_value_%02d", i);
+        void *func = dlsym (handles[i], fname);
+        if (func == NULL)
+          {
+            printf ("error: dlsym: %s\n", dlerror ());
+            exit (1);
+          }
+        set_value_funcs[i] = func;
+      }
+
+      /* Obtain the getter function.  */
+      {
+        char fname[100];
+        snprintf (fname, sizeof (fname), "get_value_%02d", i);
+        void *func = dlsym (handles[i], fname);
+        if (func == NULL)
+          {
+            printf ("error: dlsym: %s\n", dlerror ());
+            exit (1);
+          }
+        get_value_funcs[i] = func;
+      }
+    }
+}
+
+/* Running thread which forces real TLS initialization.  */
+static void *
+blocked_thread_func (void *closure)
+{
+  pause ();
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_t blocked_thread = xpthread_create (NULL, blocked_thread_func, NULL);
+  init_functions ();
+
+  struct value values[COUNT];
+  /* Initialze the TLS variables.  */
+  for (int i = 0; i < COUNT; ++i)
+    {
+      for (int j = 0; j < PER_VALUE_COUNT; ++j)
+        values[i].num[j] = rand ();
+      set_value_funcs[i] (&values[i]);
+    }
+
+  /* Read back their values to check that they do not overlap.  */
+  for (int i = 0; i < COUNT; ++i)
+    {
+      struct value actual;
+      get_value_funcs[i] (&actual);
+
+      for (int j = 0; j < PER_VALUE_COUNT; ++j)
+        if (actual.num[j] != values[i].num[j])
+        {
+          printf ("error: mismatch at variable %d/%d: %d != %d\n",
+                  i, j, actual.num[j], values[i].num[j]);
+          exit (1);
+        }
+    }
+
+  pthread_cancel (blocked_thread);
+  xpthread_join (blocked_thread);
+
+  /* Close the modules.  */
+  for (int i = 0; i < COUNT; ++i)
+    dlclose (handles[i]);
+
+  return 0;
+}
diff --git a/elf/tst-tls-manydynamic.h b/elf/tst-tls-manydynamic.h
new file mode 100644
index 0000000..4756ece
--- /dev/null
+++ b/elf/tst-tls-manydynamic.h
@@ -0,0 +1,44 @@
+/* Interfaces for test with many dynamic TLS variables.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef TST_TLS_MANYDYNAMIC_H
+#define TST_TLS_MANYDYNAMIC_H
+
+enum
+  {
+    /* This many TLS variables (and modules) are defined.  */
+    COUNT = 100,
+
+    /* Number of elements in the TLS variable.  */
+    PER_VALUE_COUNT = 1,
+  };
+
+/* The TLS variables are of this type.  We use a larger type to ensure
+   that we can reach the static TLS limit with COUNT variables.  */
+struct value
+{
+  int num[PER_VALUE_COUNT];
+};
+
+/* Set the TLS variable defined in the module.  */
+typedef void (*set_value_func) (const struct value *);
+
+/* Read the TLS variable defined in the module.  */
+typedef void (*get_value_func) (struct value *);
+
+#endif /* TST_TLS_MANYDYNAMICMOD_H */
diff --git a/elf/tst-tls-manydynamicmod.c b/elf/tst-tls-manydynamicmod.c
new file mode 100644
index 0000000..578f11b
--- /dev/null
+++ b/elf/tst-tls-manydynamicmod.c
@@ -0,0 +1,36 @@
+/* Module for test with many dynamic TLS variables.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* This file is parameterized by macros NAME, SETTER, GETTER, which
+   are set form the Makefile.  */
+
+#include "tst-tls-manydynamic.h"
+
+__thread struct value NAME;
+
+void
+SETTER (const struct value *value)
+{
+  NAME = *value;
+}
+
+void
+GETTER (struct value *value)
+{
+  *value = NAME;
+}
diff --git a/sysdeps/aarch64/dl-tlsdesc.S b/sysdeps/aarch64/dl-tlsdesc.S
index 42fa943..9e557dd 100644
--- a/sysdeps/aarch64/dl-tlsdesc.S
+++ b/sysdeps/aarch64/dl-tlsdesc.S
@@ -154,7 +154,7 @@ _dl_tlsdesc_undefweak:
 	   _dl_tlsdesc_dynamic (struct tlsdesc *tdp)
 	   {
 	     struct tlsdesc_dynamic_arg *td = tdp->arg;
-	     dtv_t *dtv = *(dtv_t **)((char *)__thread_pointer + DTV_OFFSET);
+	     dtv_t *dtv = *(dtv_t **)((char *)__thread_pointer + TCBHEAD_DTV);
 	     if (__builtin_expect (td->gen_count <= dtv[0].counter
 		&& (dtv[td->tlsinfo.ti_module].pointer.val
 		    != TLS_DTV_UNALLOCATED),
@@ -193,18 +193,18 @@ _dl_tlsdesc_dynamic:
 	   td->entry in _dl_tlsdesc_resolve_rela_fixup ensuring that the load
 	   from [x0,#PTR_SIZE] here happens after the initialization of td->arg.  */
 	ldar	PTR_REG (zr), [x0]
-	ldr	PTR_REG (1), [x0,#PTR_SIZE]
-	ldr	PTR_REG (0), [x4]
-	ldr	PTR_REG (3), [x1,#(PTR_SIZE * 2)]
-	ldr	PTR_REG (2), [x0]
+	ldr	PTR_REG (1), [x0,#TLSDESC_ARG]
+	ldr	PTR_REG (0), [x4,#TCBHEAD_DTV]
+	ldr	PTR_REG (3), [x1,#TLSDESC_GEN_COUNT]
+	ldr	PTR_REG (2), [x0,#DTV_COUNTER]
 	cmp	PTR_REG (3), PTR_REG (2)
 	b.hi	2f
-	ldr	PTR_REG (2), [x1]
+	ldr	PTR_REG (2), [x1,#TLSDESC_MODID]
 	add	PTR_REG (0), PTR_REG (0), PTR_REG (2), lsl #(PTR_LOG_SIZE + 1)
-	ldr	PTR_REG (0), [x0]
-	cmn	x0, #0x1
+	ldr	PTR_REG (0), [x0] /* Load val member of DTV entry.  */
+	cmp	x0, #TLS_DTV_UNALLOCATED
 	b.eq	2f
-	ldr	PTR_REG (1), [x1,#(PTR_SIZE * 2)]
+	ldr	PTR_REG (1), [x1,#TLSDESC_MODOFF]
 	add	PTR_REG (0), PTR_REG (0), PTR_REG (1)
 	sub	PTR_REG (0), PTR_REG (0), PTR_REG (4)
 1:
diff --git a/sysdeps/aarch64/tlsdesc.sym b/sysdeps/aarch64/tlsdesc.sym
index 63766af..a06a719 100644
--- a/sysdeps/aarch64/tlsdesc.sym
+++ b/sysdeps/aarch64/tlsdesc.sym
@@ -13,3 +13,6 @@ TLSDESC_ARG			offsetof(struct tlsdesc, arg)
 TLSDESC_GEN_COUNT	offsetof(struct tlsdesc_dynamic_arg, gen_count)
 TLSDESC_MODID		offsetof(struct tlsdesc_dynamic_arg, tlsinfo.ti_module)
 TLSDESC_MODOFF		offsetof(struct tlsdesc_dynamic_arg, tlsinfo.ti_offset)
+TCBHEAD_DTV		offsetof(tcbhead_t, dtv)
+DTV_COUNTER		offsetof(dtv_t, counter)
+TLS_DTV_UNALLOCATED	TLS_DTV_UNALLOCATED

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]