| Message ID | 20170806222606.GA13700@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Aug 6, 2017 at 6:26 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Linux/x86, there are 3 ways to make a system call: > > 1. call *%gs:SYSINFO_OFFSET. This requires TLS initialization. > 2. call *_dl_sysinfo. This requires relocation of _dl_sysinfo. > 3. int $0x80. This works everywhere. > > When an object file is compiled with PIC, #1 is prefered since it is > faster than #3 and doesn't require relocation of _dl_sysinfo. For > dynamic executables, ld.so initializes TLS. However, for static > executables, before TLS is initialized by __libc_setup_tls, #3 should > be used for syscalls. This patch adds _startup_sbrk and _startup_fatal > to be used in static executables before __libc_setup_tls is called. By > default, they are defined to __sbrk and __libc_fatal, respectively. On > x86, a special _startup_sbrk is provided and _startup_fatal is turned > into ABORT_INSTRUCTION. > > Any comments? I have questions: 1) When you say "Linux/x86", do you mean specifically the old 32-bit ABI, or does this apply to x32 and/or x64 as well? 2) This is described as fixing a bug; please add a test case. (The bug report describes a crash during 'make install', but that's not good enough, it should be caught by 'make check' pre-installation.) 3) Couldn't we avoid adding complexity in shared code by doing startup in a slightly different order? Specifically, why can't we ensure that TLS is initialized, at least enough of it that "call *%gs:SYSINFO_OFFSET" works, before we attempt to make any syscalls? zw
On Mon, Aug 7, 2017 at 4:47 AM, Zack Weinberg <zackw@panix.com> wrote: > On Sun, Aug 6, 2017 at 6:26 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Linux/x86, there are 3 ways to make a system call: >> >> 1. call *%gs:SYSINFO_OFFSET. This requires TLS initialization. >> 2. call *_dl_sysinfo. This requires relocation of _dl_sysinfo. >> 3. int $0x80. This works everywhere. >> >> When an object file is compiled with PIC, #1 is prefered since it is >> faster than #3 and doesn't require relocation of _dl_sysinfo. For >> dynamic executables, ld.so initializes TLS. However, for static >> executables, before TLS is initialized by __libc_setup_tls, #3 should >> be used for syscalls. This patch adds _startup_sbrk and _startup_fatal >> to be used in static executables before __libc_setup_tls is called. By >> default, they are defined to __sbrk and __libc_fatal, respectively. On >> x86, a special _startup_sbrk is provided and _startup_fatal is turned >> into ABORT_INSTRUCTION. >> >> Any comments? > > I have questions: > > 1) When you say "Linux/x86", do you mean specifically the old 32-bit > ABI, or does this apply to x32 and/or x64 as well? It only applies to Linux/x86 since Linux/x86-64 always use "syscall" which doesn't need TLS nor relocation. > 2) This is described as fixing a bug; please add a test case. (The bug > report describes a crash during 'make install', but that's not good > enough, it should be caught by 'make check' pre-installation.) All static tests fail when GCC is default to PIE. > 3) Couldn't we avoid adding complexity in shared code by doing startup > in a slightly different order? Specifically, why can't we ensure that > TLS is initialized, at least enough of it that "call > *%gs:SYSINFO_OFFSET" works, before we attempt to make any syscalls? To initialize TLS, we need a single syscall, sbrk. My patch provides _startup_sbrk which can be used befor TLS is initialized.
On Mon, Aug 7, 2017 at 8:38 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Mon, Aug 7, 2017 at 4:47 AM, Zack Weinberg <zackw@panix.com> wrote: > > To initialize TLS, we need a single syscall, sbrk. I presume this is to allocate memory for the TLS block. But why can't the TLS block - remember, this is for the main thread, in a statically linked binary - be statically allocated data? zw
On Mon, Aug 7, 2017 at 6:11 AM, Zack Weinberg <zackw@panix.com> wrote: > On Mon, Aug 7, 2017 at 8:38 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Mon, Aug 7, 2017 at 4:47 AM, Zack Weinberg <zackw@panix.com> wrote: >> >> To initialize TLS, we need a single syscall, sbrk. > > I presume this is to allocate memory for the TLS block. But why can't > the TLS block - remember, this is for the main thread, in a statically > linked binary - be statically allocated data? > I can look into it. But tunables_strdup also calls sbrk before TLS is initialized. We still need it even if TLS initialization doesn't.
On further reflection: On Mon, Aug 7, 2017 at 8:38 AM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Mon, Aug 7, 2017 at 4:47 AM, Zack Weinberg <zackw@panix.com> wrote: >> 1) When you say "Linux/x86", do you mean specifically the old 32-bit >> ABI, or does this apply to x32 and/or x64 as well? > > It only applies to Linux/x86 since Linux/x86-64 always use "syscall" > which doesn't need TLS nor relocation. I think of "x86" as the umbrella term nowadays. I think I am not the only person who would be less confused if, in the future, you would consistently call it "x86-32" or "i386" when you mean specifically the old 32-bit ABI and operating mode (i.e. processor is not in long mode). >> 2) This is described as fixing a bug; please add a test case. (The bug >> report describes a crash during 'make install', but that's not good >> enough, it should be caught by 'make check' pre-installation.) > > All static tests fail when GCC is default to PIE. OK, in that case please mention that in the commit message so that people doing archaeology know why there doesn't need to be a specific test case. zw
On 06/08/2017 19:26, H.J. Lu wrote: [..] > diff --git a/sysdeps/generic/startup.h b/sysdeps/generic/startup.h > new file mode 100644 > index 0000000000..aa63b31181 > --- /dev/null > +++ b/sysdeps/generic/startup.h > @@ -0,0 +1,30 @@ > +/* Generic definitions of functions used by static libc main startup. > + 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/>. */ > + > +static inline void * > +_startup_sbrk (intptr_t __delta) > +{ > + return __sbrk (__delta); > +} > + > +__attribute__ ((__noreturn__)) > +static inline void > +_startup_fatal (const char *__message) > +{ > + __libc_fatal (__message); > +} I think there is no need of underlying prefixes for inline functions. > diff --git a/sysdeps/unix/sysv/linux/i386/Makefile b/sysdeps/unix/sysv/linux/i386/Makefile > index 4080b8c966..cefa1511f6 100644 > --- a/sysdeps/unix/sysv/linux/i386/Makefile > +++ b/sysdeps/unix/sysv/linux/i386/Makefile > @@ -31,6 +31,10 @@ sysdep_routines += divdi3 > shared-only-routines += divdi3 > CPPFLAGS-divdi3.c = -Din_divdi3_c > endif > +ifneq (,$(pic-default)) > +sysdep_routines += startup_sbrk > +static-only-routines += startup_sbrk > +endif > endif > > ifeq ($(subdir),nptl) > diff --git a/sysdeps/unix/sysv/linux/i386/startup.h b/sysdeps/unix/sysv/linux/i386/startup.h > new file mode 100644 > index 0000000000..ccfba45153 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/i386/startup.h > @@ -0,0 +1,38 @@ > +/* Linux/i386 definitions of functions used by static libc main startup. > + 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/>. */ > + > +#if defined PIC && !defined SHARED We already check and set build-pie-default on configure/make.in, wouldn't be useful if we also define a BUILD_PIE as well? > +# include <abort-instr.h> > + > +/* Can't use "call *%gs:SYSINFO_OFFSET" during statup in static PIE. */ > +# define I386_USE_SYSENTER 0 > + > +extern void * _startup_sbrk (intptr_t) attribute_hidden; > + > +__attribute__ ((__noreturn__)) > +static inline void > +_startup_fatal (const char *__message __attribute__ ((unused))) > +{ > + /* This is only called very early during startup in static PIE. > + FIXME: How can it be improved? */ > + ABORT_INSTRUCTION; > + __builtin_unreachable (); > +} Maybe also provide a __writev using 'int $0x80' so it can use _dl_debug_printf? > +#else > +# include_next <startup.h> > +#endif > diff --git a/sysdeps/unix/sysv/linux/i386/startup_sbrk.c b/sysdeps/unix/sysv/linux/i386/startup_sbrk.c > new file mode 100644 > index 0000000000..8239938ddf > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/i386/startup_sbrk.c I am not very found of replicating generic code, but for this case it seems that i386 version seems to have too many specific cases that I am not sure if it worth trying to consolidate it. > @@ -0,0 +1,67 @@ > +/* Linux/i386 definitions of _startup_sbrk. > + 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/>. */ > + > +#include <unistd.h> > +#include <startup.h> > +#include <errno.h> > +#include <sysdep.h> > + > +/* Defined in brk.c. */ > +extern void *__curbrk attribute_hidden; > + > +static int > +startup_brk (void *addr) > +{ > + INTERNAL_SYSCALL_DECL (err); > + void *newbrk = (void *) INTERNAL_SYSCALL_CALL (brk, err, addr); > + __curbrk = newbrk; > + if (newbrk < addr) > + _startup_fatal (NULL); > + return 0; > +} > + > +/* Extend the process's data space by INCREMENT. If INCREMENT is negative, > + shrink data space by - INCREMENT. Return start of new space allocated, > + or call _startup_fatal for errors. */ > + > +void * > +_startup_sbrk (intptr_t increment) > +{ > + void *oldbrk; > + > + /* Update __curbrk from the kernel's brk value. That way two separate > + instances of __brk and __sbrk can share the heap, returning > + interleaved pieces of it. */ > + if (__curbrk == NULL) > + if (startup_brk (0) < 0) /* Initialize the break. */ > + _startup_fatal (NULL); > + > + if (increment == 0) > + return __curbrk; > + > + oldbrk = __curbrk; > + if (increment > 0 > + ? ((uintptr_t) oldbrk + (uintptr_t) increment < (uintptr_t) oldbrk) > + : ((uintptr_t) oldbrk < (uintptr_t) -increment)) > + _startup_fatal (NULL); > + > + if (startup_brk (oldbrk + increment) < 0) > + _startup_fatal (NULL); > + > + return oldbrk; > +} >
On Mon, Aug 7, 2017 at 6:26 AM, Zack Weinberg <zackw@panix.com> wrote: > On Mon, Aug 7, 2017 at 9:21 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Mon, Aug 7, 2017 at 6:11 AM, Zack Weinberg <zackw@panix.com> wrote: >>> On Mon, Aug 7, 2017 at 8:38 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>>> On Mon, Aug 7, 2017 at 4:47 AM, Zack Weinberg <zackw@panix.com> wrote: >>>> >>>> To initialize TLS, we need a single syscall, sbrk. >>> >>> I presume this is to allocate memory for the TLS block. But why can't >>> the TLS block - remember, this is for the main thread, in a statically >>> linked binary - be statically allocated data? >> >> I can look into it. The code is #if TLS_TCB_AT_TP /* Align the TCB offset to the maximum alignment, as _dl_allocate_tls_storage (in elf/dl-tls.c) does using __libc_memalign and dl_tls_static_align. */ tcb_offset = roundup (memsz + GL(dl_tls_static_size), max_align); tlsblock = __sbrk (tcb_offset + TLS_INIT_TCB_SIZE + max_align); #elif TLS_DTV_AT_TP tcb_offset = roundup (TLS_INIT_TCB_SIZE, align ?: 1); tlsblock = __sbrk (tcb_offset + memsz + max_align + TLS_PRE_TCB_SIZE + GL(dl_tls_static_size)); tlsblock += TLS_PRE_TCB_SIZE; Are you suggesting that we use a static data with a fixed size and check the size at run-time? >> But tunables_strdup also calls sbrk before TLS is initialized. We >> still need it even if TLS initialization doesn't. > > Again, does this _need_ to happen that early, or could it be moved later? > We have __tunables_init (__environ); ARCH_INIT_CPU_FEATURES (); /* Perform IREL{,A} relocations. */ ARCH_SETUP_IREL (); /* The stack guard goes into the TCB, so initialize it early. */ __libc_setup_tls (); __libc_setup_tls should be called after ARCH_INIT_CPU_FEATURES and ARCH_SETUP_IREL, which must be called after __tunables_init since __tunables_init can change CPU features. Given this, we need a working sbrk before TLS is initialized.
On Mon, Aug 7, 2017 at 6:34 AM, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > > > On 06/08/2017 19:26, H.J. Lu wrote: > [..] >> diff --git a/sysdeps/generic/startup.h b/sysdeps/generic/startup.h >> new file mode 100644 >> index 0000000000..aa63b31181 >> --- /dev/null >> +++ b/sysdeps/generic/startup.h >> @@ -0,0 +1,30 @@ >> +/* Generic definitions of functions used by static libc main startup. >> + 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/>. */ >> + >> +static inline void * >> +_startup_sbrk (intptr_t __delta) >> +{ >> + return __sbrk (__delta); >> +} >> + >> +__attribute__ ((__noreturn__)) >> +static inline void >> +_startup_fatal (const char *__message) >> +{ >> + __libc_fatal (__message); >> +} > > I think there is no need of underlying prefixes for inline functions. But _startup_sbrk may not be inlined for i386. >> diff --git a/sysdeps/unix/sysv/linux/i386/Makefile b/sysdeps/unix/sysv/linux/i386/Makefile >> index 4080b8c966..cefa1511f6 100644 >> --- a/sysdeps/unix/sysv/linux/i386/Makefile >> +++ b/sysdeps/unix/sysv/linux/i386/Makefile >> @@ -31,6 +31,10 @@ sysdep_routines += divdi3 >> shared-only-routines += divdi3 >> CPPFLAGS-divdi3.c = -Din_divdi3_c >> endif >> +ifneq (,$(pic-default)) >> +sysdep_routines += startup_sbrk >> +static-only-routines += startup_sbrk >> +endif >> endif >> >> ifeq ($(subdir),nptl) >> diff --git a/sysdeps/unix/sysv/linux/i386/startup.h b/sysdeps/unix/sysv/linux/i386/startup.h >> new file mode 100644 >> index 0000000000..ccfba45153 >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/i386/startup.h >> @@ -0,0 +1,38 @@ >> +/* Linux/i386 definitions of functions used by static libc main startup. >> + 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/>. */ >> + >> +#if defined PIC && !defined SHARED > > We already check and set build-pie-default on configure/make.in, wouldn't > be useful if we also define a BUILD_PIE as well? We can have #if define PIC && !defined SHARED # define BUILD_PIE #endif in include/libc-symbols.h or config.h.in >> +# include <abort-instr.h> >> + >> +/* Can't use "call *%gs:SYSINFO_OFFSET" during statup in static PIE. */ >> +# define I386_USE_SYSENTER 0 >> + >> +extern void * _startup_sbrk (intptr_t) attribute_hidden; >> + >> +__attribute__ ((__noreturn__)) >> +static inline void >> +_startup_fatal (const char *__message __attribute__ ((unused))) >> +{ >> + /* This is only called very early during startup in static PIE. >> + FIXME: How can it be improved? */ >> + ABORT_INSTRUCTION; >> + __builtin_unreachable (); >> +} > > Maybe also provide a __writev using 'int $0x80' so it can use _dl_debug_printf? _dl_debug_printf calls _dl_debug_vdprintf which makes quite a few syscalls. _startup_fatal is called very early during start up when something goes wrong. When it is called, something very very bad must have happened. I don't think ABORT_INSTRUCTION is a terrible choice. > >> +#else >> +# include_next <startup.h> >> +#endif >> diff --git a/sysdeps/unix/sysv/linux/i386/startup_sbrk.c b/sysdeps/unix/sysv/linux/i386/startup_sbrk.c >> new file mode 100644 >> index 0000000000..8239938ddf >> --- /dev/null >> +++ b/sysdeps/unix/sysv/linux/i386/startup_sbrk.c > > I am not very found of replicating generic code, but for this case it seems > that i386 version seems to have too many specific cases that I am not sure > if it worth trying to consolidate it. > >> @@ -0,0 +1,67 @@ >> +/* Linux/i386 definitions of _startup_sbrk. >> + 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/>. */ >> + >> +#include <unistd.h> >> +#include <startup.h> >> +#include <errno.h> >> +#include <sysdep.h> >> + >> +/* Defined in brk.c. */ >> +extern void *__curbrk attribute_hidden; >> + >> +static int >> +startup_brk (void *addr) >> +{ >> + INTERNAL_SYSCALL_DECL (err); >> + void *newbrk = (void *) INTERNAL_SYSCALL_CALL (brk, err, addr); >> + __curbrk = newbrk; >> + if (newbrk < addr) >> + _startup_fatal (NULL); >> + return 0; >> +} >> + >> +/* Extend the process's data space by INCREMENT. If INCREMENT is negative, >> + shrink data space by - INCREMENT. Return start of new space allocated, >> + or call _startup_fatal for errors. */ >> + >> +void * >> +_startup_sbrk (intptr_t increment) >> +{ >> + void *oldbrk; >> + >> + /* Update __curbrk from the kernel's brk value. That way two separate >> + instances of __brk and __sbrk can share the heap, returning >> + interleaved pieces of it. */ >> + if (__curbrk == NULL) >> + if (startup_brk (0) < 0) /* Initialize the break. */ >> + _startup_fatal (NULL); >> + >> + if (increment == 0) >> + return __curbrk; >> + >> + oldbrk = __curbrk; >> + if (increment > 0 >> + ? ((uintptr_t) oldbrk + (uintptr_t) increment < (uintptr_t) oldbrk) >> + : ((uintptr_t) oldbrk < (uintptr_t) -increment)) >> + _startup_fatal (NULL); >> + >> + if (startup_brk (oldbrk + increment) < 0) >> + _startup_fatal (NULL); >> + >> + return oldbrk; >> +} >>
On 07/08/2017 13:48, H.J. Lu wrote: > On Mon, Aug 7, 2017 at 6:34 AM, Adhemerval Zanella > <adhemerval.zanella@linaro.org> wrote: >> >> >> On 06/08/2017 19:26, H.J. Lu wrote: >> [..] >>> diff --git a/sysdeps/generic/startup.h b/sysdeps/generic/startup.h >>> new file mode 100644 >>> index 0000000000..aa63b31181 >>> --- /dev/null >>> +++ b/sysdeps/generic/startup.h >>> @@ -0,0 +1,30 @@ >>> +/* Generic definitions of functions used by static libc main startup. >>> + 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/>. */ >>> + >>> +static inline void * >>> +_startup_sbrk (intptr_t __delta) >>> +{ >>> + return __sbrk (__delta); >>> +} >>> + >>> +__attribute__ ((__noreturn__)) >>> +static inline void >>> +_startup_fatal (const char *__message) >>> +{ >>> + __libc_fatal (__message); >>> +} >> >> I think there is no need of underlying prefixes for inline functions. > > But _startup_sbrk may not be inlined for i386. I meant for arguments name. > >>> diff --git a/sysdeps/unix/sysv/linux/i386/Makefile b/sysdeps/unix/sysv/linux/i386/Makefile >>> index 4080b8c966..cefa1511f6 100644 >>> --- a/sysdeps/unix/sysv/linux/i386/Makefile >>> +++ b/sysdeps/unix/sysv/linux/i386/Makefile >>> @@ -31,6 +31,10 @@ sysdep_routines += divdi3 >>> shared-only-routines += divdi3 >>> CPPFLAGS-divdi3.c = -Din_divdi3_c >>> endif >>> +ifneq (,$(pic-default)) >>> +sysdep_routines += startup_sbrk >>> +static-only-routines += startup_sbrk >>> +endif >>> endif >>> >>> ifeq ($(subdir),nptl) >>> diff --git a/sysdeps/unix/sysv/linux/i386/startup.h b/sysdeps/unix/sysv/linux/i386/startup.h >>> new file mode 100644 >>> index 0000000000..ccfba45153 >>> --- /dev/null >>> +++ b/sysdeps/unix/sysv/linux/i386/startup.h >>> @@ -0,0 +1,38 @@ >>> +/* Linux/i386 definitions of functions used by static libc main startup. >>> + 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/>. */ >>> + >>> +#if defined PIC && !defined SHARED >> >> We already check and set build-pie-default on configure/make.in, wouldn't >> be useful if we also define a BUILD_PIE as well? > > We can have > > #if define PIC && !defined SHARED > # define BUILD_PIE > #endif > > in include/libc-symbols.h or config.h.in I would prefer it to make it more readable. > >>> +# include <abort-instr.h> >>> + >>> +/* Can't use "call *%gs:SYSINFO_OFFSET" during statup in static PIE. */ >>> +# define I386_USE_SYSENTER 0 >>> + >>> +extern void * _startup_sbrk (intptr_t) attribute_hidden; >>> + >>> +__attribute__ ((__noreturn__)) >>> +static inline void >>> +_startup_fatal (const char *__message __attribute__ ((unused))) >>> +{ >>> + /* This is only called very early during startup in static PIE. >>> + FIXME: How can it be improved? */ >>> + ABORT_INSTRUCTION; >>> + __builtin_unreachable (); >>> +} >> >> Maybe also provide a __writev using 'int $0x80' so it can use _dl_debug_printf? > > _dl_debug_printf calls _dl_debug_vdprintf which makes quite > a few syscalls. _startup_fatal is called very early during start > up when something goes wrong. When it is called, something > very very bad must have happened. I don't think ABORT_INSTRUCTION > is a terrible choice. > I do agree the ABORT_INSTRUCTION is not a bad choice, it is the lack of information of what might the cause it that bothers me. Even with strace it might be the case where syscalls does not fail, but there is an issue with pointer value check. I do not have a simple good solution though and I also think this kind of issues would indicate an underlying issue with either the kernel or toolchain, so I think we can live with it for now.
On Aug 07 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
> Given this, we need a working sbrk before TLS is initialized.
How about making sbrk always use int $0x80?
Andreas.
On Mon, Aug 7, 2017 at 10:37 AM, Andreas Schwab <schwab@linux-m68k.org> wrote: > On Aug 07 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote: > >> Given this, we need a working sbrk before TLS is initialized. > > How about making sbrk always use int $0x80? > > Andreas. We can do that in sbrk.o and will make change less complex. I will give it a try.
On Mon, Aug 7, 2017 at 12:26 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Mon, Aug 7, 2017 at 6:26 AM, Zack Weinberg <zackw@panix.com> wrote: >> On Mon, Aug 7, 2017 at 9:21 AM, H.J. Lu <hjl.tools@gmail.com> wrote: >>> On Mon, Aug 7, 2017 at 6:11 AM, Zack Weinberg <zackw@panix.com> wrote: >>>> >>>> I presume this is to allocate memory for the TLS block. But why can't >>>> the TLS block - remember, this is for the main thread, in a statically >>>> linked binary - be statically allocated data? >>> >>> I can look into it. > > The code is > > #if TLS_TCB_AT_TP > /* Align the TCB offset to the maximum alignment, as > _dl_allocate_tls_storage (in elf/dl-tls.c) does using __libc_memalign > and dl_tls_static_align. */ > tcb_offset = roundup (memsz + GL(dl_tls_static_size), max_align); > tlsblock = __sbrk (tcb_offset + TLS_INIT_TCB_SIZE + max_align); > #elif TLS_DTV_AT_TP > tcb_offset = roundup (TLS_INIT_TCB_SIZE, align ?: 1); > tlsblock = __sbrk (tcb_offset + memsz + max_align > + TLS_PRE_TCB_SIZE + GL(dl_tls_static_size)); > tlsblock += TLS_PRE_TCB_SIZE; > > Are you suggesting that we use a static data with a fixed size and > check the size at run-time? This is a static binary - there should be no guessing or checking required! The linker ought to be able to allocate the entire initial TCB as static data. I'm surprised it doesn't already do that, in fact. (It might be necessary to *copy* that initial TCB onto the heap if someone calls pthread_key_create or dlopen, but at that point malloc works.) >> Again, does this _need_ to happen that early, or could it be moved later? > > __libc_setup_tls should be called after ARCH_INIT_CPU_FEATURES and > ARCH_SETUP_IREL, <andreas> Why? </andreas> zw
diff --git a/csu/libc-tls.c b/csu/libc-tls.c index 3c897bf28b..6f0e698220 100644 --- a/csu/libc-tls.c +++ b/csu/libc-tls.c @@ -16,11 +16,12 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#include <unistd.h> +#include <stdio.h> +#include <startup.h> #include <errno.h> #include <ldsodefs.h> #include <tls.h> -#include <unistd.h> -#include <stdio.h> #include <sys/param.h> @@ -142,11 +143,11 @@ __libc_setup_tls (void) _dl_allocate_tls_storage (in elf/dl-tls.c) does using __libc_memalign and dl_tls_static_align. */ tcb_offset = roundup (memsz + GL(dl_tls_static_size), max_align); - tlsblock = __sbrk (tcb_offset + TLS_INIT_TCB_SIZE + max_align); + tlsblock = _startup_sbrk (tcb_offset + TLS_INIT_TCB_SIZE + max_align); #elif TLS_DTV_AT_TP tcb_offset = roundup (TLS_INIT_TCB_SIZE, align ?: 1); - tlsblock = __sbrk (tcb_offset + memsz + max_align - + TLS_PRE_TCB_SIZE + GL(dl_tls_static_size)); + tlsblock = _startup_sbrk (tcb_offset + memsz + max_align + + TLS_PRE_TCB_SIZE + GL(dl_tls_static_size)); tlsblock += TLS_PRE_TCB_SIZE; #else /* In case a model with a different layout for the TCB and DTV @@ -193,7 +194,7 @@ __libc_setup_tls (void) # error "Either TLS_TCB_AT_TP or TLS_DTV_AT_TP must be defined" #endif if (__builtin_expect (lossage != NULL, 0)) - __libc_fatal (lossage); + _startup_fatal (lossage); /* Update the executable's link map with enough information to make the TLS routines happy. */ diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c index 231fb8ca93..23c89b2c03 100644 --- a/elf/dl-tunables.c +++ b/elf/dl-tunables.c @@ -18,9 +18,11 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#include <unistd.h> +#include <stdio.h> +#include <startup.h> #include <stdint.h> #include <stdbool.h> -#include <unistd.h> #include <stdlib.h> #include <sysdep.h> #include <fcntl.h> @@ -42,7 +44,9 @@ tunables_strdup (const char *in) size_t i = 0; while (in[i++] != '\0'); - char *out = __sbrk (i); + + /* Can't use __sbrk before __libc_setup_tls is called. */ + char *out = _startup_sbrk (i); /* FIXME: In reality if the allocation fails, __sbrk will crash attempting to set the thread-local errno since the TCB has not yet been set up. This diff --git a/sysdeps/generic/startup.h b/sysdeps/generic/startup.h new file mode 100644 index 0000000000..aa63b31181 --- /dev/null +++ b/sysdeps/generic/startup.h @@ -0,0 +1,30 @@ +/* Generic definitions of functions used by static libc main startup. + 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/>. */ + +static inline void * +_startup_sbrk (intptr_t __delta) +{ + return __sbrk (__delta); +} + +__attribute__ ((__noreturn__)) +static inline void +_startup_fatal (const char *__message) +{ + __libc_fatal (__message); +} diff --git a/sysdeps/unix/sysv/linux/i386/Makefile b/sysdeps/unix/sysv/linux/i386/Makefile index 4080b8c966..cefa1511f6 100644 --- a/sysdeps/unix/sysv/linux/i386/Makefile +++ b/sysdeps/unix/sysv/linux/i386/Makefile @@ -31,6 +31,10 @@ sysdep_routines += divdi3 shared-only-routines += divdi3 CPPFLAGS-divdi3.c = -Din_divdi3_c endif +ifneq (,$(pic-default)) +sysdep_routines += startup_sbrk +static-only-routines += startup_sbrk +endif endif ifeq ($(subdir),nptl) diff --git a/sysdeps/unix/sysv/linux/i386/startup.h b/sysdeps/unix/sysv/linux/i386/startup.h new file mode 100644 index 0000000000..ccfba45153 --- /dev/null +++ b/sysdeps/unix/sysv/linux/i386/startup.h @@ -0,0 +1,38 @@ +/* Linux/i386 definitions of functions used by static libc main startup. + 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/>. */ + +#if defined PIC && !defined SHARED +# include <abort-instr.h> + +/* Can't use "call *%gs:SYSINFO_OFFSET" during statup in static PIE. */ +# define I386_USE_SYSENTER 0 + +extern void * _startup_sbrk (intptr_t) attribute_hidden; + +__attribute__ ((__noreturn__)) +static inline void +_startup_fatal (const char *__message __attribute__ ((unused))) +{ + /* This is only called very early during startup in static PIE. + FIXME: How can it be improved? */ + ABORT_INSTRUCTION; + __builtin_unreachable (); +} +#else +# include_next <startup.h> +#endif diff --git a/sysdeps/unix/sysv/linux/i386/startup_sbrk.c b/sysdeps/unix/sysv/linux/i386/startup_sbrk.c new file mode 100644 index 0000000000..8239938ddf --- /dev/null +++ b/sysdeps/unix/sysv/linux/i386/startup_sbrk.c @@ -0,0 +1,67 @@ +/* Linux/i386 definitions of _startup_sbrk. + 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/>. */ + +#include <unistd.h> +#include <startup.h> +#include <errno.h> +#include <sysdep.h> + +/* Defined in brk.c. */ +extern void *__curbrk attribute_hidden; + +static int +startup_brk (void *addr) +{ + INTERNAL_SYSCALL_DECL (err); + void *newbrk = (void *) INTERNAL_SYSCALL_CALL (brk, err, addr); + __curbrk = newbrk; + if (newbrk < addr) + _startup_fatal (NULL); + return 0; +} + +/* Extend the process's data space by INCREMENT. If INCREMENT is negative, + shrink data space by - INCREMENT. Return start of new space allocated, + or call _startup_fatal for errors. */ + +void * +_startup_sbrk (intptr_t increment) +{ + void *oldbrk; + + /* Update __curbrk from the kernel's brk value. That way two separate + instances of __brk and __sbrk can share the heap, returning + interleaved pieces of it. */ + if (__curbrk == NULL) + if (startup_brk (0) < 0) /* Initialize the break. */ + _startup_fatal (NULL); + + if (increment == 0) + return __curbrk; + + oldbrk = __curbrk; + if (increment > 0 + ? ((uintptr_t) oldbrk + (uintptr_t) increment < (uintptr_t) oldbrk) + : ((uintptr_t) oldbrk < (uintptr_t) -increment)) + _startup_fatal (NULL); + + if (startup_brk (oldbrk + increment) < 0) + _startup_fatal (NULL); + + return oldbrk; +}