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] |
On Mon, 28 Aug 2017 11:24:04 -0300 Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > On 27/08/2017 16:21, Sergei Trofimovich wrote: > > On Wed, 2 Aug 2017 22:26:45 +0100 > > Sergei Trofimovich <slyfox@gentoo.org> wrote: > > > >> On Wed, 2 Aug 2017 17:59:57 -0300 > >> Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > >> > >>> On 02/08/2017 17:53, Sergei Trofimovich wrote: > >>>> On Wed, 5 Jul 2017 22:47:17 +0100 > >>>> Sergei Trofimovich <slyfox@gentoo.org> wrote: > >>>> > >>>>> On Sun, 25 Jun 2017 23:07:15 +0100 > >>>>> Sergei Trofimovich <slyfox@gentoo.org> wrote: > >>>>> > >>>>>> Minimal reproducer: > >>>>>> > >>>>>> #include <pthread.h> > >>>>>> > >>>>>> static void * f (void * p) { return NULL; } > >>>>>> > >>>>>> int main (int argc, const char ** argv) { > >>>>>> pthread_t t; > >>>>>> pthread_create (&t, NULL, &f, NULL); > >>>>>> > >>>>>> pthread_join (t, NULL); > >>>>>> return 0; > >>>>>> } > >>>>>> > >>>>>> $ gcc -O0 -ggdb3 -o r bug.c -pthread && ./r > >>>>>> > >>>>>> Program terminated with signal SIGSEGV, Segmentation fault. > >>>>>> #0 0x2000000000077da0 in start_thread (arg=0x0) at pthread_create.c:432 > >>>>>> 432 __madvise (pd->stackblock, freesize - PTHREAD_STACK_MIN, MADV_DONTNEED); > >>>>>> > >>>>>> Here crash happens right after attempt to free unused part of > >>>>>> thread's stack. > >>>>>> > >>>>>> On most architectures stack grows only down or grows only up. > >>>>>> And there glibc decides which of unused ends of stack blocks can be freed. > >>>>>> > >>>>>> ia64 maintans two stacks. Both of them grow from the opposite directions: > >>>>>> - normal "sp" stack (stack for local variables) grows down > >>>>>> - register stack "bsp" grows up from the opposite end of stack block > >>>>>> > >>>>>> In this failure case we have prematurely freed "rsp" stack. > >>>>>> > >>>>>> The change leaves a few pages from both sides of stack block. > >>>>>> > >>>>>> Bug: https://sourceware.org/PR21672 > >>>>>> Bug: https://bugs.gentoo.org/622694 > >>>>>> Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> > >>>>>> --- > >>>>>> nptl/pthread_create.c | 18 ++++++++++++++++-- > >>>>>> 1 file changed, 16 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c > >>>>>> index 7a970ffc5b..6e3f6db5b1 100644 > >>>>>> --- a/nptl/pthread_create.c > >>>>>> +++ b/nptl/pthread_create.c > >>>>>> @@ -555,10 +555,24 @@ START_THREAD_DEFN > >>>>>> size_t pagesize_m1 = __getpagesize () - 1; > >>>>>> #ifdef _STACK_GROWS_DOWN > >>>>>> char *sp = CURRENT_STACK_FRAME; > >>>>>> - size_t freesize = (sp - (char *) pd->stackblock) & ~pagesize_m1; > >>>>>> + char *freeblock = (char *) pd->stackblock; > >>>>>> + size_t freesize = (sp - freeblock) & ~pagesize_m1; > >>>>>> assert (freesize < pd->stackblock_size); > >>>>>> +# ifdef __ia64__ > >>>>>> if (freesize > PTHREAD_STACK_MIN) > >>>>>> - __madvise (pd->stackblock, freesize - PTHREAD_STACK_MIN, MADV_DONTNEED); > >>>>>> + { > >>>>>> + /* On ia64 stack grows both ways! > >>>>>> + - normal "sp" stack (stack for local variables) grows down > >>>>>> + - register stack "bsp" grows up from the opposite end of stack block > >>>>>> + > >>>>>> + Thus we leave PTHREAD_STACK_MIN bytes from stack block top > >>>>>> + and leave same PTHREAD_STACK_MIN at stack block bottom. */ > >>>>>> + freeblock += PTHREAD_STACK_MIN; > >>>>>> + freesize -= PTHREAD_STACK_MIN; > >>>>>> + } > >>>>>> +# endif > >>>>>> + if (freesize > PTHREAD_STACK_MIN) > >>>>>> + __madvise (freeblock, freesize - PTHREAD_STACK_MIN, MADV_DONTNEED); > >>>>>> #else > >>>>>> /* Page aligned start of memory to free (higher than or equal > >>>>>> to current sp plus the minimum stack size). */ > >>>>>> -- > >>>>>> 2.13.1 > >>>>>> > >>>>> > >>>>> Ping :) > >>>> > >>>> Ping^2 > >>>> > >>> > >>> What about https://sourceware.org/ml/libc-alpha/2017-07/msg00302.html ? > >> > >> Oh, I didn't get that email back in my inbox. Fetching from UI. > >> Will testboth in SKI and real machine and report back in that thread. > >> > >> [preliminary] Fix looks good to me. > >> > >> Thank you! > > > > Got to testing your patch today on glibc-master. > > > > The traces below are traces of 'nptl/tst-align' glibc test. > > > > Without your patch pthread_create() returns an error due to > > guard page being way off: > > > > 4645 mmap2(NULL, 8388608, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_STACK, -1, 0) = 0x2000000000334000 > > 4645 mprotect(0x2000000000734000, 8372224, PROT_READ|PROT_WRITE) = -1 ENOMEM (Cannot allocate memory) > > 4645 munmap(0x2000000000334000, 8388608) = 0 > > > > With your patch thread is being created fine! > > But crashes at pthread_join() shutdown (the same way I've reported originally). > > > > // pthread_create > > 5315 mmap2(NULL, 8388608, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_STACK, -1, 0) = 0x2000000000334000 > > 5315 mprotect(0x2000000000334000, 4177920, PROT_READ|PROT_WRITE) = 0 > > 5315 mprotect(0x2000000000734000, 4194304, PROT_READ|PROT_WRITE) = 0 > > ... > > // pthread_join > > 5316 madvise(0x2000000000334000, 8175616, MADV_DONTNEED) = 0 > > 5316 --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_ACCERR, si_addr=0x8} --- > > 5315 <... futex resumed>) = ? > > 5316 +++ killed by SIGSEGV +++ > > I think we need to apply the same logic for disjointed stacks on the > madvise call after thread finishes. The patch below should do it (only > tested with ia64 build on x86_64 which is also not affected). If it > fixes ia64 I can prepare a patch. > > --- > > diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c > index 6d1bcaa..9b4cf84 100644 > --- a/nptl/allocatestack.c > +++ b/nptl/allocatestack.c > @@ -356,7 +356,7 @@ setup_stack_prot (char *mem, size_t size, char *guard, size_t guardsize, > const int prot) > { > char *guardend = guard + guardsize; > -#if _STACK_GROWS_DOWN > +#if _STACK_GROWS_DOWN && !defined(NEED_SEPARATE_REGISTER_STACK) > /* As defined at guard_position, for architectures with downward stack > the guard page is always at start of the allocated area. */ > if (__mprotect (guardend, size - guardsize, prot) != 0) > @@ -372,6 +372,34 @@ setup_stack_prot (char *mem, size_t size, char *guard, size_t guardsize, > return 0; > } > > +/* Mark the memory of the stack as usable to the kerneli (through > + madivse (MADV_DONTNEED). It frees everything except for the space used > + for the TCB itself. */ > +static inline void > +__always_inline > +advise_stack_range (void *mem, size_t size, uintptr_t pd, size_t guardsize) > +{ > + uintptr_t sp = (uintptr_t) CURRENT_STACK_FRAME; > + size_t pagesize_m1 = __getpagesize () - 1; > +#if _STACK_GROWS_DOWN && !defined(NEED_SEPARATE_REGISTER_STACK) > + size_t freesize = (sp - (uintptr_t) mem) & ~pagesize_m1; > + assert (freesize < size); > + if (freesize > PTHREAD_STACK_MIN) > + __madvise (mem, freesize - PTHREAD_STACK_MIN, MADV_DONTNEED); > +#else > + /* Page aligned start of memory to free (higher than or equal > + to current sp plus the minimum stack size). */ > + uintptr_t freeblock = (sp + PTHREAD_STACK_MIN + pagesize_m1) & ~pagesize_m1; > + uintptr_t free_end = (pd - guardsize) & ~pagesize_m1; > + if (free_end > freeblock) > + { > + size_t freesize = free_end - freeblock; > + assert (freesize < size); > + __madvise ((void*) freeblock, freesize, MADV_DONTNEED); > + } > +#endif > +} > + > /* Returns a usable stack for a new thread either by allocating a > new stack or reusing a cached stack of sufficient size. > ATTR must be non-NULL and point to a valid pthread_attr. > diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c > index 2f8ada3..3148f78 100644 > --- a/nptl/pthread_create.c > +++ b/nptl/pthread_create.c > @@ -553,29 +553,8 @@ START_THREAD_DEFN > > /* Mark the memory of the stack as usable to the kernel. We free > everything except for the space used for the TCB itself. */ > - size_t pagesize_m1 = __getpagesize () - 1; > -#ifdef _STACK_GROWS_DOWN > - char *sp = CURRENT_STACK_FRAME; > - size_t freesize = (sp - (char *) pd->stackblock) & ~pagesize_m1; > - assert (freesize < pd->stackblock_size); > - if (freesize > PTHREAD_STACK_MIN) > - __madvise (pd->stackblock, freesize - PTHREAD_STACK_MIN, MADV_DONTNEED); > -#else > - /* Page aligned start of memory to free (higher than or equal > - to current sp plus the minimum stack size). */ > - void *freeblock = (void*)((size_t)(CURRENT_STACK_FRAME > - + PTHREAD_STACK_MIN > - + pagesize_m1) > - & ~pagesize_m1); > - char *free_end = (char *) (((uintptr_t) pd - pd->guardsize) & ~pagesize_m1); > - /* Is there any space to free? */ > - if (free_end > (char *)freeblock) > - { > - size_t freesize = (size_t)(free_end - (char *)freeblock); > - assert (freesize < pd->stackblock_size); > - __madvise (freeblock, freesize, MADV_DONTNEED); > - } > -#endif > + advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd, > + pd->guardsize); > > /* If the thread is detached free the TCB. */ > if (IS_DETACHED (pd)) That works! Tested as 'find nptl -name '*.out' -delete && make check subdirs=nptl' Before the patch: Summary of test results: 223 FAIL 107 PASS 6 UNSUPPORTED 1 XFAIL After the patch: Summary of test results: 1 FAIL 329 PASS 6 UNSUPPORTED 1 XFAIL -- Sergei
Attachment:
pgpDTm1XGq0Wp.pgp
Description: Цифровая подпись OpenPGP
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |