Re: [PATCH][BZ 21672] fix pthread_create crash in ia64

[Sergei Trofimovich <slyich at gmail dot com>]

On Fri, 7 Jul 2017 11:36:17 -0300
Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> On 25/06/2017 19:07, Sergei Trofimovich 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  
> 
> I am trying to validate this change using the same emulator you referenced
> in the bug reports (ski) using first master then 2.25 to check if it is
> a recent change (since Mike Frysinger reported that on actual hardware that
> nptl tests are actually working [1]).  However, even your simple 
> testcase seems to fail either or without the proposed fix. So I am not sure 
> if it is something wrong with the kernel I build (a 4.12 from linux-stable), 
> the base environment I set (I basically use the sysroot create from
> build-many-glibc.py plus a busybox to have some workable tools).
> 
> In any case, I would like to know why exactly this started to happen
> since on 2.25 mostly nptl cases shows no issue.  I am more inclined to
> take this is something related to my changes for BZ#18988 [2] and I think
> we need to take these separate stack in consideration on 'setup_stack_prot'
> which is not what we currently doing. 
> 
> If I understood correctly, for both IA64 and HPPA (the only arch with 
> _STACK_GROWN_UP) the mmap area for thread stack will have two disjoin areas
> and on ia64 it will be the two stack areas divided by the guard page. If
> it is the case I think we need to use the same logic of _STACK_GROWS_UP
> on 'setup_stack_prot'. Could you check if the patch below helps?

Oh, I didn't mention which glibc works and which does not.
On my system glibc-2.23 works and glibc-2.24 does not. I didn't try 2.25 yet.

AFAIU it does not yet contain stack changes. I planned to try 2.25/git
after I get 2.24 back working.

I think ia64 worked only by chance as [handwave] register stack
does not necessarily gets spilled to memory if CPU has enough cache.

I'll try glibc-2.25 with your patch only and report back.

> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index ec7d42e..d07b410 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)
> 
> [1] https://sourceware.org/glibc/wiki/Release/2.25
> [2] https://sourceware.org/git/?p=glibc.git;a=commit;h=0edbf1230131dfeb03d843d2859e2104456fad80
> 
> > 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).  */
> >   
> 


-- 

  Sergei

Attachment: pgpPOumV46nqM.pgp
Description: Цифровая подпись OpenPGP