| Message ID | 1487015120-29166-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 02/13/2017 08:45 PM, Adhemerval Zanella wrote: > +/* 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), sizeof (struct pthread)) \ > + : 0)) How does this preserve the alignment of struct pthread? It's also not clear to me how the “version control” aspect of __tcb_private_ss is supposed to work. If the intent is to prevent loading of split-stack binaries with an older glibc, then a data symbol would be a safer choice. Thanks, Florian
On 14/02/2017 08:03, Florian Weimer wrote: > On 02/13/2017 08:45 PM, Adhemerval Zanella wrote: >> +/* 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), sizeof (struct pthread)) \ >> + : 0)) > > How does this preserve the alignment of struct pthread? Indeed, it should be ALIGN_UP (sizeof (tcbprehead_t), __alignof__ (struct pthread) > > It's also not clear to me how the “version control” aspect of __tcb_private_ss is supposed to work. If the intent is to prevent loading of split-stack binaries with an older glibc, then a data symbol would be a safer choice. > My idea is to mimic 67385a01d22 (powerpc: Add hwcap/hwcap2/platform data to TCB) and is indeed to prevent new binaries to run on older glibc where the pre-tcb header is not allocated. I will check if a data symbol works better.
On 02/14/2017 05:03 AM, Florian Weimer wrote: > On 02/13/2017 08:45 PM, Adhemerval Zanella wrote: >> +/* 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), sizeof (struct pthread)) \ + >> : 0)) > > How does this preserve the alignment of struct pthread? > > It's also not clear to me how the “version control” aspect of > __tcb_private_ss is supposed to work. If the intent is to prevent > loading of split-stack binaries with an older glibc, then a data > symbol would be a safer choice. Why is a data symbol a safer choice? I'm curious, because I recommended the design pattern that Adhemerval is copying from the POWER example. -- Cheers, Carlos.
On 03/14/2017 02:17 AM, Carlos O'Donell wrote: > On 02/14/2017 05:03 AM, Florian Weimer wrote: >> On 02/13/2017 08:45 PM, Adhemerval Zanella wrote: >>> +/* 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), sizeof (struct pthread)) \ + >>> : 0)) >> >> How does this preserve the alignment of struct pthread? >> >> It's also not clear to me how the “version control” aspect of >> __tcb_private_ss is supposed to work. If the intent is to prevent >> loading of split-stack binaries with an older glibc, then a data >> symbol would be a safer choice. > > Why is a data symbol a safer choice? No lazy binding and hence a more well-defined failure mode. Thanks, Florian
On 14/03/2017 03:53, Florian Weimer wrote: > On 03/14/2017 02:17 AM, Carlos O'Donell wrote: >> On 02/14/2017 05:03 AM, Florian Weimer wrote: >>> On 02/13/2017 08:45 PM, Adhemerval Zanella wrote: >>>> +/* 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), sizeof (struct pthread)) \ + >>>> : 0)) >>> >>> How does this preserve the alignment of struct pthread? >>> >>> It's also not clear to me how the “version control” aspect of >>> __tcb_private_ss is supposed to work. If the intent is to prevent >>> loading of split-stack binaries with an older glibc, then a data >>> symbol would be a safer choice. >> >> Why is a data symbol a safer choice? > > No lazy binding and hence a more well-defined failure mode. > > Thanks, > Florian > Carlos, I also see Florian suggestion a better approach. Using a weak function symbol, it will fail at runtime after symbol resolution with an undefined jump to a null symbol (for aarch64 it will be a segfault anyway), while with data symbol loader will explicit dump the missing symbol.
diff --git a/sysdeps/aarch64/Makefile b/sysdeps/aarch64/Makefile index 562c137..0155988 100644 --- a/sysdeps/aarch64/Makefile +++ b/sysdeps/aarch64/Makefile @@ -5,7 +5,7 @@ CFLAGS-backtrace.c += -funwind-tables endif 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..b0f1a3b 100644 --- a/sysdeps/aarch64/Versions +++ b/sysdeps/aarch64/Versions @@ -3,3 +3,11 @@ libc { _mcount; } } + +ld { + GLIBC_2.26 { + # Symbol used to version control the private GLIBC TCB split-stack + # field. + __tcb_private_ss; + } +} diff --git a/sysdeps/aarch64/nptl/tcb-offsets.sym b/sysdeps/aarch64/nptl/tcb-offsets.sym index 238647d..6004379 100644 --- a/sysdeps/aarch64/nptl/tcb-offsets.sym +++ b/sysdeps/aarch64/nptl/tcb-offsets.sym @@ -3,4 +3,4 @@ PTHREAD_MULTIPLE_THREADS_OFFSET offsetof (struct pthread, header.multiple_threads) PTHREAD_TID_OFFSET offsetof (struct pthread, tid) -PTHREAD_SIZEOF sizeof (struct pthread) +PTHREAD_PRE_TCB_SIZE TLS_PRE_TCB_SIZE diff --git a/sysdeps/aarch64/nptl/tls.h b/sysdeps/aarch64/nptl/tls.h index 175df39..f526fa6 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-internal.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), sizeof (struct pthread)) \ + : 0)) /* Alignment requirements for the TCB. */ # define TLS_TCB_ALIGN __alignof__ (struct pthread) @@ -84,7 +97,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 +106,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..4df3cd4 --- /dev/null +++ b/sysdeps/aarch64/tcb-version.c @@ -0,0 +1,24 @@ +/* TCB field abi advertise symbols. + Copyright (C) 2017 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/>. */ + +/* Symbol used to version control the private GLIBC TCB split-stack + field. */ +void +__tcb_private_ss (void) +{ +} diff --git a/sysdeps/unix/sysv/linux/aarch64/ld.abilist b/sysdeps/unix/sysv/linux/aarch64/ld.abilist index ec7f617..1c0a952 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.26 GLIBC_2.26 A +GLIBC_2.26 __tcb_private_ss F diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h b/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h index 4be2259..e4ac2ba 100644 --- a/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h @@ -114,7 +114,7 @@ extern int __local_multiple_threads attribute_hidden; # else # define SINGLE_THREAD_P(R) \ mrs x##R, tpidr_el0; \ - sub x##R, x##R, PTHREAD_SIZEOF; \ + sub x##R, x##R, PTHREAD_PRE_TCB_SIZE; \ ldr w##R, [x##R, PTHREAD_MULTIPLE_THREADS_OFFSET] # endif # endif
Changes from previous version [1] - Instead of allocate the split-stack field on tcbhead_t, allocate is before thread pointer and adjust struct pthread get methods. -- 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 bofore 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 de existence 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 __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. (__tcb_private_ss): Likewise. * sysdeps/aarch64/nptl/tcb-offsets.sym (pthread_sizeof): rename to pthread_pre_tcb_size. * sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h [!is_in (libprthread) && !is_in (libc) (single_thread_p): use pthread_pre_tcb_size instead of pthread_sizeof. [1] https://sourceware.org/ml/libc-alpha/2016-07/msg00647.html --- ChangeLog | 19 +++++++++++++++++++ sysdeps/aarch64/Makefile | 2 +- sysdeps/aarch64/Versions | 8 ++++++++ sysdeps/aarch64/nptl/tcb-offsets.sym | 2 +- sysdeps/aarch64/nptl/tls.h | 25 ++++++++++++++++++++----- sysdeps/aarch64/tcb-version.c | 24 ++++++++++++++++++++++++ sysdeps/unix/sysv/linux/aarch64/ld.abilist | 2 ++ sysdeps/unix/sysv/linux/aarch64/sysdep-cancel.h | 2 +- 8 files changed, 76 insertions(+), 8 deletions(-) create mode 100644 sysdeps/aarch64/tcb-version.c -- 2.7.4