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

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

[Sergei Trofimovich]

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

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

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 2981 bytes --]

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 :)

-- 

  Sergei

[-- Attachment #2: Цифровая подпись OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Adhemerval Zanella]

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?

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).  */
> 

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

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 3237 bytes --]

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

-- 

  Sergei

[-- Attachment #2: Цифровая подпись OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Adhemerval Zanella]

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 ?

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

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 3839 bytes --]

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!

-- 

  Sergei

[-- Attachment #2: Цифровая подпись OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 5744 bytes --]

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 #2: Цифровая подпись OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 5285 bytes --]

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 +++

-- 

  Sergei

[-- Attachment #2: Цифровая подпись OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Adhemerval Zanella]

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))

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

[Sergei Trofimovich]

[-- Attachment #1: Type: text/plain, Size: 9953 bytes --]

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 #2: Цифровая подпись OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]