[PATCH][BZ 21672] fix pthread_create crash in ia64
Sergei Trofimovich
slyfox@gentoo.org
Mon Aug 28 16:39:00 GMT 2017
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: æøфрþòðѠÿþôÿøÑÂь OpenPGP
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20170828/948e9612/attachment.sig>
More information about the Libc-alpha
mailing list