From patchwork Fri Mar 17 14:38:11 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: 19635 Received: (qmail 85146 invoked by alias); 17 Mar 2017 14:38:32 -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 84293 invoked by uid 89); 17 Mar 2017 14:38:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-27.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-qt0-f180.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:references:from:message-id:date :user-agent:mime-version:in-reply-to; bh=n1SZl+FjH+JmJ9UUlq4ULGnylzLboDg5PsBuhCVj84M=; b=eyjLgz9dkYOspjYtQFyTaWJ3jeOZmtD41w13CQwbpxexcZUNtjrc+dop0hYu7Zk8pu p+xGdGnUqqMBAJ+lxnHKsyyMYiFpN9rbPL2peWSTOFFSk8gNqcnnfeVv4uoGfvJwh3iD 6xHcnWEALnZ8r85w3jsbCWy8fdUUceuieDnuaE7BFJc3NI4fuI6EfcR8GzYTH9pBfMHM UGetQJHR38MRMGTbMafwss8oTegdtBv/wd9/kUzauUPc9Hv6q5CY87zoUyAtF6NjvlkJ VWGLcyaLjKaROZDdlPMzaqL6reG3lf4hhWgamOgGH+Pf9o6lyak1YgLJH54xv8IgTA/v 1bSg== X-Gm-Message-State: AFeK/H0yW9xBQYbT1o/rbi32dIQ1sfvpWamVCZEIr5aH6lQsPMyCsnxOC3BjysUkAinD9gAe X-Received: by 10.200.55.241 with SMTP id e46mr14768237qtc.200.1489761500235; Fri, 17 Mar 2017 07:38:20 -0700 (PDT) Subject: Re: [PATCH] posix_spawn: use a larger min stack for -fstack-check [BZ #21253] To: Andrew Pinski , Florian Weimer , GNU C Library References: <20170316073012.22763-1-vapier@gentoo.org> <20170316215236.GA24205@vapier> <20170316224124.GB24205@vapier> From: Adhemerval Zanella Message-ID: <8506619b-cc52-bf74-db4c-65f6cfd99c8e@linaro.org> Date: Fri, 17 Mar 2017 11:38:11 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170316224124.GB24205@vapier> On 16/03/2017 19:41, Mike Frysinger wrote: > On 16 Mar 2017 14:56, Andrew Pinski wrote: >> On Thu, Mar 16, 2017 at 2:52 PM, Mike Frysinger wrote: >>> On 16 Mar 2017 09:17, Florian Weimer wrote: >>>> On 03/16/2017 08:30 AM, Mike Frysinger wrote: >>>>> When glibc is built with -fstack-check, trying to use posix_spawn can >>>>> lead to segfaults due to gcc internally probing stack memory too far. >>>>> The new spawn API will allocate a minimum of 1 page, but the stack >>>>> checking logic might probe a couple of pages. When it tries to walk >>>>> them, everything falls apart. >>>>> >>>>> The gcc internal docs [1] state the default interval checking is one >>>>> page. Which means we need two pages (the current one, and the next >>>>> probed). No target currently defines it larger. >>>> >>>> GCC miscomputes the offsets in some cases, so I would not rely on this. >>>> >>>> Would it be possible compile the functions involved without >>>> -fstack-check instead? >>> >>> i mentioned in the bug that it's not feasible: compiling this one file >>> doesn't help as it calls other glibc funcs which will have checking >>> enabled. so we'd have to manually track the full call stack here and >>> disable it on all the files which is a fairly fragile/burdensome process. >>> >>>>> /* Add a slack area for child's stack. */ >>>>> size_t argv_size = (argc * sizeof (void *)) + 512; >>>>> - size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize)); >>>>> + /* We need at least a few pages in case the compiler's stack checking is >>>>> + enabled. In some configs, it is known to use at least 24KiB. */ >>>>> + size_t stack_size = ALIGN_UP (argv_size, 32 * 1024); >>>> >>>> 64 KiB pages are common, so this reduces the stack size in many cases. >>> >>> common where ? are you talking about large pages ? >>> >>> going by PAGE_SHIFT in the kernel, looks to me like very few >>> targets allow using 64 KiB at all, let alone being common. >> >> RHEL on ARM64 defaults to 64k. >> For Cavium Octeon SDK, the mips kernel defaults to 64k page size. > > i think it's fair to say that those userbases do not constitute anywhere > close to a majority, or even a signficiantly large presence. > > from what i can tell, gcc does not expose any CPP defines we can use to > see if stack checking is enabled. so we don't have a way of turning the > extra allocations on conditionally. > > wouldn't be easy to add a configure check either since -fstack-check can > be turned on via default specs. > -mike > My main concern is to add some hackery on an implementation that should be neutral to any compiler option and to keep try to adjust the initial stack size based on possible compiler version/architecture. In any way, I think the following patch should the -fstack-check if compiler is using it: diff --git a/config.h.in b/config.h.in index fb2cc51..6bfa8f7 100644 --- a/config.h.in +++ b/config.h.in @@ -263,4 +263,7 @@ in i386 6 argument syscall issue). */ #define CAN_USE_REGISTER_ASM_EBP 0 +/* Build glibc with gcc -fstack-check. */ +#define BUILD_FSTACK_CHECK 0 + #endif diff --git a/configure.ac b/configure.ac index 4981bf9..26629e1 100644 --- a/configure.ac +++ b/configure.ac @@ -1626,6 +1626,22 @@ for opt in -ffp-contract=off -mno-fused-madd; do done]) AC_SUBST(libc_cv_cc_nofma) + +dnl Determine if -fstack-check is being used +AC_CACHE_CHECK(for -fstack-check, libc_cv_fstack_check, [dnl +cat > conftest.c << \EOF +void foo (void) {}; +EOF +dnl +libc_cv_fstack_check=no +if ${CC-cc} $CFLAGS -Q -v conftest.c -c 2>&1 | sed -e '/cc1/!d;s/(")|(^.* - )//g' | grep '\-fstack\-check' >/dev/null; then + libc_cv_fstack_check=yes +fi +rm -f conftest*]) +if test x"$libc_cv_fstack_check" = xyes; then + AC_DEFINE(BUILD_FSTACK_CHECK) +fi + if test -n "$submachine"; then AC_CACHE_CHECK([for compiler option for CPU variant], libc_cv_cc_submachine, [dnl diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c index 24f75db..bfc033f 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/sysdeps/unix/sysv/linux/spawni.c @@ -317,8 +317,20 @@ __spawnix (pid_t * pid, const char *file, | ((GL (dl_stack_flags) & PF_X) ? PROT_EXEC : 0)); /* Add a slack area for child's stack. */ +#if BUILD_FSTACK_CHECK +/* The gcc -fstack-check internal docs state the default interval checking is + one page, which means we need two pages (the current one, and the next + probed). No target currently defines it larger. Further, it mentions that + the default minimum stack size needed to recover from an overflow is 4/8KiB + for sjlj or 8/12KiB for others. But some Linux targets (like mips and + ppc) go up to 16KiB (and some non-Linux targets go up to 24KiB). So + we allocate at least 4 pages for such cases*/ +# define MIN_PAGES 4 +#else +# define MIn_PAGES 1 +#endif size_t argv_size = (argc * sizeof (void *)) + 512; - size_t stack_size = ALIGN_UP (argv_size, GLRO(dl_pagesize)); + size_t stack_size = ALIGN_UP (argv_size, MIN_PAGES * GLRO(dl_pagesize)); void *stack = __mmap (NULL, stack_size, prot, MAP_PRIVATE | MAP_ANONYMOUS | MAP_STACK, -1, 0); if (__glibc_unlikely (stack == MAP_FAILED))