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][BZ 21672] fix pthread_create crash in ia64


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]