From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oo1-xc43.google.com (mail-oo1-xc43.google.com [IPv6:2607:f8b0:4864:20::c43]) by sourceware.org (Postfix) with ESMTPS id 666893858D35 for ; Wed, 7 Oct 2020 17:43:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 666893858D35 Received: by mail-oo1-xc43.google.com with SMTP id f19so562363oot.4 for ; Wed, 07 Oct 2020 10:43:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=rr40AY17+JN/ccHdnYDYpYoE+EL3oWtfHYYLIZ6ISKI=; b=JBQa5bJ/W1elGgjOdjMHTOKimtzjcO0Q9h0g/sDu7xAX78YeovSdzdJuTq9yUeW2kl T6IohF5ezA3Waqobz2PNr8zlW8bpi90maOww+Yh/f1jrWnTc7MbP38s4xN6ss4HxA9XE j4lh6KVkpHiWenbq//WoDRhGobzKmjm0lFAKkwGSAqlYOny0AGMB8pKlg3ObufKGGZzK yjN0BfgmaPVJ30mLjqONnWB5GLxw9vhrmbOjD7TuhclyekGkRHBvl5Lv7G3YLHeQS0c2 8in+kyl5p4MYeJLrAcObGDVGZRXzY40oLJsBD25vTsuWTCKSf6Bncl22kc/aOLV84+yO SsuQ== X-Gm-Message-State: AOAM532Wh8MbBU2nyCWxSgAGQdPJMljIVffSW5cfJLlMT9fxC8wPIDjp 1QAgLtk6d1Vnv/aDuEI8CYwxK55fpZRiWPCCAB4= X-Google-Smtp-Source: ABdhPJzWMVzGqR5SxyUc86nVDzuxbj/uZppqxB/TRkQD4b0cE2CbMgcvq5u2gDDqnO15qQxtBZjZImNvNE0ffWyvl2Q= X-Received: by 2002:a4a:d40c:: with SMTP id n12mr2801524oos.35.1602092616732; Wed, 07 Oct 2020 10:43:36 -0700 (PDT) MIME-Version: 1.0 References: <20201005134534.GT6642@arm.com> <20201006092532.GU6642@arm.com> <20201006152553.GY6642@arm.com> <20201006165520.GA6642@arm.com> <20201007104720.GH6642@arm.com> <20201007154559.GI6642@arm.com> In-Reply-To: <20201007154559.GI6642@arm.com> From: "H.J. Lu" Date: Wed, 7 Oct 2020 10:43:00 -0700 Message-ID: Subject: Re: [RFC PATCH 0/4] x86: Improve Minimum Alternate Stack Size To: Dave Martin Cc: "Chang S. Bae" , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Andy Lutomirski , "the arch/x86 maintainers" , Len Brown , Dave Hansen , Michael Ellerman , Tony Luck , "Ravi V. Shankar" , GNU C Library , linux-arch , Linux API , LKML Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3029.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Oct 2020 17:43:39 -0000 On Wed, Oct 7, 2020 at 8:46 AM Dave Martin wrote: > > On Wed, Oct 07, 2020 at 06:30:03AM -0700, H.J. Lu wrote: > > On Wed, Oct 7, 2020 at 3:47 AM Dave Martin wrote: > > > > > > On Tue, Oct 06, 2020 at 10:44:14AM -0700, H.J. Lu wrote: > > [...] > > > > > I updated my glibc patch to add both _SC_MINSIGSTKSZ and _SC_SIGSTKSZ. > > > > _SC_MINSIGSTKSZ is the minimum signal stack size from AT_MINSIGSTKSZ, > > > > which is the signal frame size used by kernel, and _SC_SIGSTKSZ is the value > > > > of sysconf (_SC_MINSIGSTKSZ) + 6KB for user application. > > > > > > Can I suggest sysconf (_SC_MINSIGSTKSZ) * 4 instead? > > > > Done. > > OK. I was prepared to have to argue my case a bit more, but if you > think this is OK then I will stop arguing ;) > > > > > If the arch has more or bigger registers to save in the signal frame, > > > the chances are that they're going to get saved in some userspace stack > > > frames too. So I suspect that the user signal handler stack usage may > > > scale up to some extent rather than being a constant. > > > > > > > > > To help people migrate without unpleasant surprises, I also figured it > > > would be a good idea to make sure that sysconf (_SC_MINSIGSTKSZ) >= > > > legacy MINSIGSTKSZ, and sysconf (_SC_SIGSTKSZ) >= legacy SIGSTKSZ. > > > This should makes it safer to use sysconf (_SC_MINSIGSTKSZ) as a > > > drop-in replacement for MINSIGSTKSZ, etc. > > > > > > (To explain: AT_MINSIGSTKSZ may actually be < MINSIGSTKSZ on AArch64. > > > My idea was that sysconf () should hide this surprise, but people who > > > really want to know the kernel value would tolerate some > > > nonportability and read AT_MINSIGSTKSZ directly.) > > > > > > > > > So then: > > > > > > kernel_minsigstksz = getauxval(AT_MINSIGSTKSZ); > > > minsigstksz = LEGACY_MINSIGSTKSZ; > > > if (kernel_minsigstksz > minsigstksz) > > > minsistksz = kernel_minsigstksz; > > > > > > sigstksz = LEGACY_SIGSTKSZ; > > > if (minsigstksz * 4 > sigstksz) > > > sigstksz = minsigstksz * 4; > > > > I updated users/hjl/AT_MINSIGSTKSZ branch with > > > > +@item _SC_MINSIGSTKSZ > > +@standards{GNU, unistd.h} > > Can we specify these more agressively now? > > > +Inquire about the signal stack size used by the kernel. > > I think we've already concluded that this should included all mandatory > overheads, including those imposed by the compiler and glibc? > > e.g.: > > --8<-- > > The returned value is the minimum number of bytes of free stack space > required in order to gurantee successful, non-nested handling of a > single signal whose handler is an empty function. > > -->8-- > > > + > > +@item _SC_SIGSTKSZ > > +@standards{GNU, unistd.h} > > +Inquire about the default signal stack size for a signal handler. > > Similarly: > > --8<-- > > The returned value is the suggested minimum number of bytes of stack > space required for a signal stack. > > This is not guaranteed to be enough for any specific purpose other than > the invocation of a single, non-nested, empty handler, but nonetheless > should be enough for basic scenarios involving simple signal handlers > and very low levels of signal nesting (say, 2 or 3 levels at the very > most). > > This value is provided for developer convenience and to ease migration > from the legacy SIGSTKSZ constant. Programs requiring stronger > guarantees should avoid using it if at all possible. > > -->8-- Done. > > If these descriptions are too wordy, we might want to move some of it > out to signal.texi, though. > > > > > case _SC_MINSIGSTKSZ: > > assert (GLRO(dl_minsigstacksize) != 0); > > return GLRO(dl_minsigstacksize); > > > > case _SC_SIGSTKSZ: > > { > > /* Return MAX (MINSIGSTKSZ, sysconf (_SC_MINSIGSTKSZ)) * 4. */ > > long int minsigstacksize = GLRO(dl_minsigstacksize); > > _Static_assert (__builtin_constant_p (MINSIGSTKSZ), > > "MINSIGSTKSZ is constant"); > > if (minsigstacksize < MINSIGSTKSZ) > > minsigstacksize = MINSIGSTKSZ; > > return minsigstacksize * 4; > > } > > > > > > > > (Or something like that, unless the architecture provides its own > > > definitions. ia64's MINSIGSTKSZ is enormous, so it probably doesn't > > > want this.) > > > > > > > > > Also: should all these values be rounded up to a multiple of the > > > architecture's preferred stack alignment? > > > > Kernel should provide a properly aligned AT_MINSIGSTKSZ. > > OK. Can you comment on Chang S. Bae's series? I wasn't sure that the > proposal produces an aligned value for AT_MINSIGSTKSZ on x86, but maybe > I just worked it out wrong. In Linux kernel, the signal frame data is composed of the following areas and laid out as: ------------------------------ | alignment padding | ------------------------------ | xsave buffer | <<<<<<< This must be 64 byte aligned ------------------------------ | fsave header (32-bit only) | ------------------------------ | siginfo + ucontext | ------------------------------ In order to get the proper alignment for xsave buffer, the signal stake size should be rounded up to 64 byte aligned. In glibc, I have /* Size of xsave buffer. */ unsigned int sigframe_size = ebx; if (64 bit) /* NB: sizeof(struct rt_sigframe) in Linux kernel. */ sigframe_size += 440; else /* NB: sizeof(struct sigframe_ia32) + sizeof(struct fregs_state)) in Linux kernel. */ sigframe_size += 736 + 112; endif /* Round up to 64-byte aligned. */ sigframe_size = ALIGN_UP (sigframe_size, 64); GLRO(dl_minsigstacksize) = sigframe_size; Kernel should do something similar. > > > > Should the preferred stack alignment also be exposed through sysconf? > > > Portable code otherwise has no way to know this, though if the > > > preferred alignment is <= the minimum malloc()/alloca() alignment then > > > this is generally not an issue.) > > > > Good question. But it is orthogonal to the signal stack size issue. > > Ack. > > There are various other brokennesses around this area, such as the fact > that the register data may now run off the end of the mcontext_t object > that is supposed to contain it. Ideally we should fork ucontext_t or > mcontext_t into two types, one for the ucontext API and one for the > signal API (which are anyway highly incompatible with each other). > > If this stuff is addressed, I guess we could bundle it under a more > general feature test macro. But it's probably best to nail down this > series in something like its current form first. Agreed. > > I'll follow up on libc-alpha with a wishlist, but that will be a > separate conversation... > > [...] > > Cheers > ---Dave -- H.J.