Patchwork aarch64: Add split-stack TCB field

login
register
mail settings
Submitter Adhemerval Zanella Netto
Date Feb. 14, 2018, 1:04 p.m.
Message ID <ecda1a6b-2c53-f655-c425-7755ac3590dd@linaro.org>
Download mbox | patch
Permalink /patch/25934/
State New
Headers show

Comments

Adhemerval Zanella Netto - Feb. 14, 2018, 1:04 p.m.
On 13/02/2018 14:56, Szabolcs Nagy wrote:
> On 07/02/18 17:58, Adhemerval Zanella wrote:
>> This patch adds split-stack support pointer guard on glibc for aarch64.
>> Different from other architectures (powerpc, s390) where the memory is
>> placed on TCB, aarch64 one is placed before thread pointer initial
>> position.  It has an advantage over extending TCB because for aarch64
>> TLS variable placement take in consideration tcbhead_t size and by
>> changing its value would require to also update the static linker
>> (and it would also add incompatibility with glibc and older linkers).
>>
>> For aarch64 tcb direct access is fastest for thread local variable on
>> all mode and related TLS access.  It requires just a direct load with
>> displacement of -8 (since thread pointer points to tcbhead_t).
>>
>> It also adds a loader symbol (__tcb_private_ss) to signal the existence
> 
> __libc_tcb_private_ss

Fixed locally.

> 
>> of the split stack guard area.
>>
>> Checked on aarch64-linux-gnu.
>>
>>     * sysdeps/aarch64/Makefile [$(subdir) = elf] (sysdeps-dl-routines):
>>     Add tcb-version.
>>     * sysdeps/aarch64/Versions [ld] (GLIBC_2.26): Add
>>     __libc_tcb_private_ss.
>>     * sysdeps/aarch64/nptl/tls.h (tcbprehead_t): New struct.
>>     (TLS_PRE_TCB_SIZE): Take tcbprehead_t in consideration.
>>     (TLS_DEFINE_INIT_TP): Likewise.
>>     (THREAD_SELF): Likewise.
>>     (DB_THREAD_SELF): Likewise.
>>     * sysdeps/aarch64/tcb-version.c: New file.
>>     * sysdeps/unix/sysv/linux/aarch64/ld.abilist (GLIBC_2.26): Add.
>>     (__libc_tcb_private_ss): Likewise.
>>     * sysdeps/aarch64/nptl/tcb-offsets.sym (PTHREAD_SIZEOF): Rename to
>>     PTHREAD_PRE_TCB_SIZE.
> 
> i don't see where PTHREAD_PRE_TCB_SIZE is used
> (or PTHREAD_SIZEOF)

Indeed, it is an artefact from previous version. I removed it in the patch below. 

>> diff --git a/sysdeps/aarch64/nptl/tls.h b/sysdeps/aarch64/nptl/tls.h
>> index ac39c24..2d88310 100644
>> --- a/sysdeps/aarch64/nptl/tls.h
>> +++ b/sysdeps/aarch64/nptl/tls.h
>> @@ -26,6 +26,7 @@
>>   # include <stddef.h>
>>   # include <stdint.h>
>>   # include <dl-dtv.h>
>> +# include <libc-pointer-arith.h>
>>     #else /* __ASSEMBLER__ */
>>   # include <tcb-offsets.h>
>> @@ -49,6 +50,12 @@ typedef struct
>>     void *private;
>>   } tcbhead_t;
>>   +typedef struct
>> +{
>> +  /* GCC split stack support.  */
>> +  void *__private_ss;
>> +} tcbprehead_t;
>> +
>>   /* This is the size of the initial TCB.  */
>>   # define TLS_INIT_TCB_SIZE    sizeof (tcbhead_t)
>>   @@ -58,8 +65,14 @@ typedef struct
>>   /* This is the size of the TCB.  */
>>   # define TLS_TCB_SIZE        sizeof (tcbhead_t)
>>   -/* This is the size we need before TCB.  */
>> -# define TLS_PRE_TCB_SIZE    sizeof (struct pthread)
>> +/* This is the size we need before TCB.  Check if there is room for
>> +   tcbprehead_t in struct pthread's final padding and if not add it on
>> +   required pre-tcb size.  */
>> +# define TLS_PRE_TCB_SIZE \
>> +  (sizeof (struct pthread)                        \
>> +   + (PTHREAD_STRUCT_END_PADDING < sizeof (tcbprehead_t)        \
>> +      ? ALIGN_UP (sizeof (tcbprehead_t), __alignof__ (struct pthread))    \
>> +      : 0))
>>   
> 
> it looks ok, but a bit ugly, i don't see a clean way
> to express the requirements in c.
> 
> if i understand correctly the tls layout is
> 
> struct pthread   tcbprehead_t   tcbhead_t   static tls
> <--------------><-------------><----------><----------->
>                                ^
>                                TP
> 
> where tcbprehead_t may overlap with paddings at the end
> of struct pthread.
> 
> and we have to make sure offsets from TP to the members
> of tcbprehead_t don't change.
> 
> i think there should be a comment about this layout and
> the alignment/offset requirements somewhere in tls.h
> (so next time it's easier to allocate such a tls slot)

I have added a comment about the layout and possible newer member
additions on tcbprehead_t.

--

	* sysdeps/aarch64/Makefile [$(subdir) = elf] (sysdeps-dl-routines):
	Add tcb-version.
	* sysdeps/aarch64/Versions [ld] (GLIBC_2.28): Add
	__libc_tcb_private_ss.
	* sysdeps/aarch64/nptl/tls.h (tcbprehead_t): New struct.
	(TLS_PRE_TCB_SIZE): Take tcbprehead_t in consideration.
	(TLS_DEFINE_INIT_TP): Likewise.
	(THREAD_SELF): Likewise.
	(DB_THREAD_SELF): Likewise.
	* sysdeps/aarch64/tcb-version.c: New file.
	* sysdeps/unix/sysv/linux/aarch64/ld.abilist (GLIBC_2.28): Add.
	(__libc_tcb_private_ss): Likewise.

---

Patch

diff --git a/sysdeps/aarch64/Makefile b/sysdeps/aarch64/Makefile
index 5f3e203..141442a 100644
--- a/sysdeps/aarch64/Makefile
+++ b/sysdeps/aarch64/Makefile
@@ -1,7 +1,7 @@ 
 long-double-fcts = yes
 
 ifeq ($(subdir),elf)
-sysdep-dl-routines += tlsdesc dl-tlsdesc
+sysdep-dl-routines += tlsdesc dl-tlsdesc tcb-version
 gen-as-const-headers += dl-link.sym
 endif
 
diff --git a/sysdeps/aarch64/Versions b/sysdeps/aarch64/Versions
index e1aa44f..dcc395c 100644
--- a/sysdeps/aarch64/Versions
+++ b/sysdeps/aarch64/Versions
@@ -3,3 +3,11 @@  libc {
     _mcount;
   }
 }
+
+ld {
+  GLIBC_2.28 {
+    # Symbol used to version control the private GLIBC TCB split-stack
+    # field.
+    __libc_tcb_private_ss;
+  }
+}
diff --git a/sysdeps/aarch64/nptl/tls.h b/sysdeps/aarch64/nptl/tls.h
index ac39c24..99606d0 100644
--- a/sysdeps/aarch64/nptl/tls.h
+++ b/sysdeps/aarch64/nptl/tls.h
@@ -26,6 +26,7 @@ 
 # include <stddef.h>
 # include <stdint.h>
 # include <dl-dtv.h>
+# include <libc-pointer-arith.h>
 
 #else /* __ASSEMBLER__ */
 # include <tcb-offsets.h>
@@ -43,12 +44,32 @@ 
 /* Get the thread descriptor definition.  */
 # include <nptl/descr.h>
 
+/* The resulting TLS layout is:
+
+   |----------------|--------------|-----------|------------|
+   | struct pthread | tcbprehead_t | tcbhead_t | static tls |
+   |----------------|--------------|-----------|------------|
+				   \_ Thread pointer
+
+   And tcbprehead_t might overlap the padding at the end of the
+   struct pthread, otherwise extra space is allocated (check TLS_PRE_TCB_SIZE
+   definition).
+
+   Additional fields on tcbhead_t must keep member offset constant (so they
+   should be added at the beggining of the struct).  */
+
 typedef struct
 {
   dtv_t *dtv;
   void *private;
 } tcbhead_t;
 
+typedef struct
+{
+  /* GCC split stack support.  */
+  void *__private_ss;
+} tcbprehead_t;
+
 /* This is the size of the initial TCB.  */
 # define TLS_INIT_TCB_SIZE	sizeof (tcbhead_t)
 
@@ -58,8 +79,14 @@  typedef struct
 /* This is the size of the TCB.  */
 # define TLS_TCB_SIZE		sizeof (tcbhead_t)
 
-/* This is the size we need before TCB.  */
-# define TLS_PRE_TCB_SIZE	sizeof (struct pthread)
+/* This is the size we need before TCB.  Check if there is room for
+   tcbprehead_t in struct pthread's final padding and if not add it on
+   required pre-tcb size.  */
+# define TLS_PRE_TCB_SIZE \
+  (sizeof (struct pthread)						\
+   + (PTHREAD_STRUCT_END_PADDING < sizeof (tcbprehead_t)		\
+      ? ALIGN_UP (sizeof (tcbprehead_t), __alignof__ (struct pthread))	\
+      : 0))
 
 /* Alignment requirements for the TCB.  */
 # define TLS_TCB_ALIGN		__alignof__ (struct pthread)
@@ -84,7 +111,8 @@  typedef struct
   ({ __asm __volatile ("msr tpidr_el0, %0" : : "r" (tcbp)); NULL; })
 
 /* Value passed to 'clone' for initialization of the thread register.  */
-# define TLS_DEFINE_INIT_TP(tp, pd) void *tp = (pd) + 1
+# define TLS_DEFINE_INIT_TP(tp, pd) \
+  void *tp = (void*)((uintptr_t) (pd) + TLS_PRE_TCB_SIZE)
 
 /* Return the address of the dtv for the current thread.  */
 # define THREAD_DTV() \
@@ -92,11 +120,12 @@  typedef struct
 
 /* Return the thread descriptor for the current thread.  */
 # define THREAD_SELF \
- ((struct pthread *)__builtin_thread_pointer () - 1)
+  ((struct pthread *)((uintptr_t) __builtin_thread_pointer () \
+		      - TLS_PRE_TCB_SIZE))
 
 /* Magic for libthread_db to know how to do THREAD_SELF.  */
 # define DB_THREAD_SELF \
-  CONST_THREAD_AREA (64, sizeof (struct pthread))
+  CONST_THREAD_AREA (64, TLS_PRE_TCB_SIZE)
 
 /* Access to data in the thread descriptor is easy.  */
 # define THREAD_GETMEM(descr, member) \
diff --git a/sysdeps/aarch64/tcb-version.c b/sysdeps/aarch64/tcb-version.c
new file mode 100644
index 0000000..eaa75dd
--- /dev/null
+++ b/sysdeps/aarch64/tcb-version.c
@@ -0,0 +1,23 @@ 
+/* TCB field abi advertise symbols.
+   Copyright (C) 2017-2018 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/>.  */
+
+#include <libc-internal.h>
+
+/* Symbol used to version control the private GLIBC TCB split-stack
+   field.  */
+long int attribute_relro __libc_tcb_private_ss;
diff --git a/sysdeps/unix/sysv/linux/aarch64/ld.abilist b/sysdeps/unix/sysv/linux/aarch64/ld.abilist
index ec7f617..aaddb24 100644
--- a/sysdeps/unix/sysv/linux/aarch64/ld.abilist
+++ b/sysdeps/unix/sysv/linux/aarch64/ld.abilist
@@ -8,3 +8,5 @@  GLIBC_2.17 calloc F
 GLIBC_2.17 free F
 GLIBC_2.17 malloc F
 GLIBC_2.17 realloc F
+GLIBC_2.28 GLIBC_2.28 A
+GLIBC_2.28 __libc_tcb_private_ss D 0x8