diff mbox

aarch64: Increase MINSIGSTKSZ and SIGSTKSZ

Message ID mvmwq1v2aqw.fsf@hawking.suse.de
State Committed
Headers

Commit Message

Andreas Schwab April 2, 2015, 8:30 a.m. UTC 
  [BZ #16850]
	* sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h: New file.
---
 sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h | 54 +++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h

Comments

Marcus Shawcroft April 2, 2015, 9:59 a.m. UTC | #1 
On 2 April 2015 at 09:30, Andreas Schwab <schwab@suse.de> wrote:
>         [BZ #16850]
>         * sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h: New file.
> ---
>  sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h | 54 +++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h

Hi.

Back in November,
https://sourceware.org/ml/libc-alpha/2014-11/msg00339.html, RenLin
proposed 8k for MINSTKSZ, a few days ago, off list, Szabolcs suggest
6k.  The ucontext is 4560 bytes and my understanding is that MINSTKSZ
only needs to be sufficient for the kernel to deliver the signal frame
which suggests that your proposed 5k is sufficient.

OK

/Marcus
Szabolcs Nagy April 2, 2015, 10:27 a.m. UTC | #2 
On 02/04/15 10:59, Marcus Shawcroft wrote:
> On 2 April 2015 at 09:30, Andreas Schwab <schwab@suse.de> wrote:
>>         [BZ #16850]
>>         * sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h: New file.
>> ---
>>  sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h | 54 +++++++++++++++++++++++++
>>  1 file changed, 54 insertions(+)
>>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h
> 
> Hi.
> 
> Back in November,
> https://sourceware.org/ml/libc-alpha/2014-11/msg00339.html, RenLin
> proposed 8k for MINSTKSZ, a few days ago, off list, Szabolcs suggest
> 6k.  The ucontext is 4560 bytes and my understanding is that MINSTKSZ
> only needs to be sufficient for the kernel to deliver the signal frame
> which suggests that your proposed 5k is sufficient.
> 
> OK
> 
> /Marcus
> 


the 5K min is enough for signal delivery by the kernel

but the way i read the standard, applications are expected to allocate
MINSIGSTKSZ + their own usage and it seems to be a stretch to
include all libc overhead, such as symbol relocation, in "own usage"
(historically the min size is small on many archs so probably it's
not an issue in practice)

note that having a huge difference between min and default does not
help: portable applications cannot expect large space above min
because most archs use min+6K for default stack size.
(so with 5K min i'd use 12K default, but this is just bikesheding)


(i note here that there is a glibc conformance bug in sigaltstack:
it does not check that the provided stack size is >= MINSIGSTKSZ
and relies on the kernel side doing the check which is broken
on every arch where the kernel has different limit

with my interpretation of MINSIGSTKSZ different kernel side
limit makes perfect sense

this bug is convenient now, because it means we can increase
the limit without affecting existing binaries, but i think it
should be fixed eventually)
Rich Felker April 2, 2015, 3:51 p.m. UTC | #3 
On Thu, Apr 02, 2015 at 11:27:47AM +0100, Szabolcs Nagy wrote:
> On 02/04/15 10:59, Marcus Shawcroft wrote:
> > On 2 April 2015 at 09:30, Andreas Schwab <schwab@suse.de> wrote:
> >>         [BZ #16850]
> >>         * sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h: New file.
> >> ---
> >>  sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h | 54 +++++++++++++++++++++++++
> >>  1 file changed, 54 insertions(+)
> >>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h
> > 
> > Hi.
> > 
> > Back in November,
> > https://sourceware.org/ml/libc-alpha/2014-11/msg00339.html, RenLin
> > proposed 8k for MINSTKSZ, a few days ago, off list, Szabolcs suggest
> > 6k.  The ucontext is 4560 bytes and my understanding is that MINSTKSZ
> > only needs to be sufficient for the kernel to deliver the signal frame
> > which suggests that your proposed 5k is sufficient.
> 
> the 5K min is enough for signal delivery by the kernel
> 
> but the way i read the standard, applications are expected to allocate
> MINSIGSTKSZ + their own usage and it seems to be a stretch to
> include all libc overhead, such as symbol relocation, in "own usage"
> (historically the min size is small on many archs so probably it's
> not an issue in practice)

The big detail to notice here is symbol relocation. If you do lazy
bindings, the resolver function needs space to backup any registers
that might ever be used as argument registers in case a
not-yet-resolved function is called from the signal handler. On an
implementation without lazy bindings, 6k (like we have on musl now) or
maybe even 5k is a suitable value for the minimum, but on glibc you
probably need at least 7k and maybe more, so 8k sounds like a good
choice to me.

Rich
Szabolcs Nagy April 2, 2015, 4:06 p.m. UTC | #4 
On 02/04/15 16:51, Rich Felker wrote:
> On Thu, Apr 02, 2015 at 11:27:47AM +0100, Szabolcs Nagy wrote:
>> On 02/04/15 10:59, Marcus Shawcroft wrote:
>>> On 2 April 2015 at 09:30, Andreas Schwab <schwab@suse.de> wrote:
>>>>         [BZ #16850]
>>>>         * sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h: New file.
>>>> ---
>>>>  sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h | 54 +++++++++++++++++++++++++
>>>>  1 file changed, 54 insertions(+)
>>>>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h
>>>
>>> Hi.
>>>
>>> Back in November,
>>> https://sourceware.org/ml/libc-alpha/2014-11/msg00339.html, RenLin
>>> proposed 8k for MINSTKSZ, a few days ago, off list, Szabolcs suggest
>>> 6k.  The ucontext is 4560 bytes and my understanding is that MINSTKSZ
>>> only needs to be sufficient for the kernel to deliver the signal frame
>>> which suggests that your proposed 5k is sufficient.
>>
>> the 5K min is enough for signal delivery by the kernel
>>
>> but the way i read the standard, applications are expected to allocate
>> MINSIGSTKSZ + their own usage and it seems to be a stretch to
>> include all libc overhead, such as symbol relocation, in "own usage"
>> (historically the min size is small on many archs so probably it's
>> not an issue in practice)
> 
> The big detail to notice here is symbol relocation. If you do lazy
> bindings, the resolver function needs space to backup any registers
> that might ever be used as argument registers in case a
> not-yet-resolved function is called from the signal handler. On an
> implementation without lazy bindings, 6k (like we have on musl now) or
> maybe even 5k is a suitable value for the minimum, but on glibc you
> probably need at least 7k and maybe more, so 8k sounds like a good
> choice to me.
> 

this is already committed, but i wonder what others think about
the two interpretations: only account for kernel saved context
needed for signal deliver vs. kernel context + worst case libc
overhead.

(i'd say this should be reported to the austingroup for clarification,
but i don't see any simple way to specify the stack requirements on
the c language level)

i think even with the 'kernel context only' definition there might
be a reason for different kernel vs libc limit because of cpu
extensions that the libc abi cares about but the installed kernel
may not. (so not checking the size in sigaltstack is observably
non-conforming).
Rich Felker April 2, 2015, 4:13 p.m. UTC | #5 
On Thu, Apr 02, 2015 at 05:06:14PM +0100, Szabolcs Nagy wrote:
> 
> 
> On 02/04/15 16:51, Rich Felker wrote:
> > On Thu, Apr 02, 2015 at 11:27:47AM +0100, Szabolcs Nagy wrote:
> >> On 02/04/15 10:59, Marcus Shawcroft wrote:
> >>> On 2 April 2015 at 09:30, Andreas Schwab <schwab@suse.de> wrote:
> >>>>         [BZ #16850]
> >>>>         * sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h: New file.
> >>>> ---
> >>>>  sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h | 54 +++++++++++++++++++++++++
> >>>>  1 file changed, 54 insertions(+)
> >>>>  create mode 100644 sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h
> >>>
> >>> Hi.
> >>>
> >>> Back in November,
> >>> https://sourceware.org/ml/libc-alpha/2014-11/msg00339.html, RenLin
> >>> proposed 8k for MINSTKSZ, a few days ago, off list, Szabolcs suggest
> >>> 6k.  The ucontext is 4560 bytes and my understanding is that MINSTKSZ
> >>> only needs to be sufficient for the kernel to deliver the signal frame
> >>> which suggests that your proposed 5k is sufficient.
> >>
> >> the 5K min is enough for signal delivery by the kernel
> >>
> >> but the way i read the standard, applications are expected to allocate
> >> MINSIGSTKSZ + their own usage and it seems to be a stretch to
> >> include all libc overhead, such as symbol relocation, in "own usage"
> >> (historically the min size is small on many archs so probably it's
> >> not an issue in practice)
> > 
> > The big detail to notice here is symbol relocation. If you do lazy
> > bindings, the resolver function needs space to backup any registers
> > that might ever be used as argument registers in case a
> > not-yet-resolved function is called from the signal handler. On an
> > implementation without lazy bindings, 6k (like we have on musl now) or
> > maybe even 5k is a suitable value for the minimum, but on glibc you
> > probably need at least 7k and maybe more, so 8k sounds like a good
> > choice to me.
> > 
> 
> this is already committed, but i wonder what others think about
> the two interpretations: only account for kernel saved context
> needed for signal deliver vs. kernel context + worst case libc
> overhead.

I think you're mistaken to describe lazy symbol resolution as "libc
overhead". There are plenty of libc functions which are going to
require more than 1k or so of stack space, and the intent of
MINSIGSTKSZ is clearly (at least based on existing practice) not to
include the needs of those functions, which makes sense because most
of them are not AS-safe and you would be hard pressed to find a setup
to meaningfully use them in signal handlers where an alt signal stack
is needed.

On the other hand, lazy binding comes into play even for your own
functions. Even calling the signal handler itself may involve lazy
binding if the address passed to sigaction was just a PLT slot. IMO
it's best though of as an implementation detail of the calling
convention, not "libc overhead".

> (i'd say this should be reported to the austingroup for clarification,
> but i don't see any simple way to specify the stack requirements on
> the c language level)

One necessary (but probably not sufficient) condition for specifying
it is that MINSIGSTKSZ needs to be sufficiently large that an empty
signal handler (body {}) cannot fail.

I'd be hesitant to actually ask for clarification since it's likely to
turn into a huge bikeshed...

> i think even with the 'kernel context only' definition there might
> be a reason for different kernel vs libc limit because of cpu
> extensions that the libc abi cares about but the installed kernel
> may not. (so not checking the size in sigaltstack is observably
> non-conforming).

Indeed. I think userspace needs to do the checking on systems where
the kernel is not imposing a fixed userspace.

Rich
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h b/sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h
new file mode 100644
index 0000000..8f2fb76
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h
@@ -0,0 +1,54 @@ 
+/* sigstack, sigaltstack definitions.
+   Copyright (C) 2015 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef _SIGNAL_H
+# error "Never include this file directly.  Use <signal.h> instead"
+#endif
+
+
+/* Structure describing a signal stack (obsolete).  */
+struct sigstack
+  {
+    void *ss_sp;		/* Signal stack pointer.  */
+    int ss_onstack;		/* Nonzero if executing on this stack.  */
+  };
+
+
+/* Possible values for `ss_flags.'.  */
+enum
+{
+  SS_ONSTACK = 1,
+#define SS_ONSTACK	SS_ONSTACK
+  SS_DISABLE
+#define SS_DISABLE	SS_DISABLE
+};
+
+/* Minimum stack size for a signal handler.  */
+#define MINSIGSTKSZ	5120
+
+/* System default stack size.  */
+#define SIGSTKSZ	16384
+
+
+/* Alternate, preferred interface.  */
+typedef struct sigaltstack
+  {
+    void *ss_sp;
+    int ss_flags;
+    size_t ss_size;
+  } stack_t;