From patchwork Mon Aug 28 14:24:04 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 22380 Received: (qmail 49304 invoked by alias); 28 Aug 2017 14:25:14 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 10423 invoked by uid 89); 28 Aug 2017 14:24:46 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, LIKELY_SPAM_BODY, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=Got, H*r:10.0.0, usable X-HELO: mail-qk0-f175.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=6QRp3JL5SY3T00cp9hp9c0jp4UgQ3sE16YeuoyHGrr0=; b=p0MrLCZkGUs7ExtwasgnZDmUlrIFJNXjcZ6VLLCnSfYgXypX6jRRRZGM2q/4WsznWq fl1bTEsOWJWE3o5+6npDLhJ9T6/YAbxKwz/7g7nYzU0xR6iexvCTqwY2Q6HKCzuGBaTW qoQM2DGI8MMHJDb4uTNq5+1f3gzxYfW2X6RBb+yiFFGGoiHfgIuMujTUtEMs71UMtzPF ZX2UTjMtTo81fpYX9omqpe9f12RqLeWtC/mvUlsy9cGsbX2XDuEXvVcAX+poaOtxyl7s ZBEQ77GoVc9uuc0vSQZSM8QhiPoZfixS2EYbk99UxMQuNQs2+NATKtqNR7k3jdQ09d/r LGNQ== X-Gm-Message-State: AHYfb5iqI2CDpVS4+Nimtk+fqbmFjlBeKAlU+UNH6i5/QTUrys/QH7MO 9cr+yT/TKfPXzra9 X-Received: by 10.55.77.66 with SMTP id a63mr1081122qkb.322.1503930249678; Mon, 28 Aug 2017 07:24:09 -0700 (PDT) Subject: Re: [PATCH][BZ 21672] fix pthread_create crash in ia64 To: Sergei Trofimovich Cc: libc-alpha@sourceware.org, vapier@gentoo.org References: <20170625220715.19422-1-slyfox@gentoo.org> <20170705224717.61c25d62@sf> <20170802215336.6c4aa4b1@sf> <9e0e0e0c-5268-fcc6-0ded-7f155e600598@linaro.org> <20170802222645.6060bc6b@sf> <20170827202149.01dc2164@sf> From: Adhemerval Zanella Message-ID: <5abf182d-16ab-96e3-068b-5240efccc1c8@linaro.org> Date: Mon, 28 Aug 2017 11:24:04 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170827202149.01dc2164@sf> On 27/08/2017 16:21, Sergei Trofimovich wrote: > On Wed, 2 Aug 2017 22:26:45 +0100 > Sergei Trofimovich wrote: > >> On Wed, 2 Aug 2017 17:59:57 -0300 >> Adhemerval Zanella wrote: >> >>> On 02/08/2017 17:53, Sergei Trofimovich wrote: >>>> On Wed, 5 Jul 2017 22:47:17 +0100 >>>> Sergei Trofimovich wrote: >>>> >>>>> On Sun, 25 Jun 2017 23:07:15 +0100 >>>>> Sergei Trofimovich wrote: >>>>> >>>>>> Minimal reproducer: >>>>>> >>>>>> #include >>>>>> >>>>>> 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 >>>>>> --- >>>>>> 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))