diff mbox

[AARCH64,BZ,#16850] Increase MINSIGSTKSZ & SIGSTKSZ

Message ID 5465D638.2070403@arm.com
State Committed
Headers

Commit Message

Renlin Li Nov. 14, 2014, 10:15 a.m. UTC 
  Hi all,

This patch duplicates sysdeps/unix/sysv/linux/bits/sigstack.h into 
sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h and modifies only 
SIGSTCKSZ and MINSIGSTKSZ.

The ucontext_t is 4560 bytes, siginfo_t is 128 bytes. The required 
sigframe is far larger than the current default MINSIGSTKSZ (which is 
2048). Thus, I increase MINSIGSTKSZ to 8192, and adjust SIGSTCKSZ 
accordingly.

Glibc has been build and test on the model, no new issues.

Okay to commit?

ChangeLog:

2014-11-14  Renlin Li  <Renlin.Li@arm.com>
     [BZ #16850]
     * sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h: New.

Comments

Will Newton Nov. 14, 2014, 10:20 a.m. UTC | #1 
On 14 November 2014 10:15, Renlin Li <renlin.li@arm.com> wrote:
> Hi all,
>
> This patch duplicates sysdeps/unix/sysv/linux/bits/sigstack.h into
> sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h and modifies only SIGSTCKSZ
> and MINSIGSTKSZ.
>
> The ucontext_t is 4560 bytes, siginfo_t is 128 bytes. The required sigframe
> is far larger than the current default MINSIGSTKSZ (which is 2048). Thus, I
> increase MINSIGSTKSZ to 8192, and adjust SIGSTCKSZ accordingly.
>
> Glibc has been build and test on the model, no new issues.
>
> Okay to commit?
>
> ChangeLog:
>
> 2014-11-14  Renlin Li  <Renlin.Li@arm.com>
>     [BZ #16850]
>     * sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h: New.

I believe this should be synchronized with the value of this constant
in the kernel.
Roland McGrath Nov. 14, 2014, 6:21 p.m. UTC | #2 
All the linux/*/bits/sigstack.h headers differ only in these two values.
It would be nice to reduce the duplication of everything else.  For
example, make linux/bits/sigstack.h #include <bits/sigtsksz.h> and add the
new header for each machine.


Thanks,
Roland
Renlin Li Nov. 19, 2014, 11:45 a.m. UTC | #3 
On 14/11/14 10:20, Will Newton wrote:
> On 14 November 2014 10:15, Renlin Li <renlin.li@arm.com> wrote:
>> Hi all,
>>
>> This patch duplicates sysdeps/unix/sysv/linux/bits/sigstack.h into
>> sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h and modifies only SIGSTCKSZ
>> and MINSIGSTKSZ.
>>
>> The ucontext_t is 4560 bytes, siginfo_t is 128 bytes. The required sigframe
>> is far larger than the current default MINSIGSTKSZ (which is 2048). Thus, I
>> increase MINSIGSTKSZ to 8192, and adjust SIGSTCKSZ accordingly.
>>
>> Glibc has been build and test on the model, no new issues.
>>
>> Okay to commit?
>>
>> ChangeLog:
>>
>> 2014-11-14  Renlin Li  <Renlin.Li@arm.com>
>>      [BZ #16850]
>>      * sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h: New.
> I believe this should be synchronized with the value of this constant
> in the kernel.
>
I got some information from this page. Alan Modra holds the option that 
those two values in glibc could differ from kernel.

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1267062

Any suggestion?
Will Newton Nov. 19, 2014, 1:54 p.m. UTC | #4 
On 19 November 2014 11:45, Renlin Li <renlin.li@arm.com> wrote:
> On 14/11/14 10:20, Will Newton wrote:
>>
>> On 14 November 2014 10:15, Renlin Li <renlin.li@arm.com> wrote:
>>>
>>> Hi all,
>>>
>>> This patch duplicates sysdeps/unix/sysv/linux/bits/sigstack.h into
>>> sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h and modifies only
>>> SIGSTCKSZ
>>> and MINSIGSTKSZ.
>>>
>>> The ucontext_t is 4560 bytes, siginfo_t is 128 bytes. The required
>>> sigframe
>>> is far larger than the current default MINSIGSTKSZ (which is 2048). Thus,
>>> I
>>> increase MINSIGSTKSZ to 8192, and adjust SIGSTCKSZ accordingly.
>>>
>>> Glibc has been build and test on the model, no new issues.
>>>
>>> Okay to commit?
>>>
>>> ChangeLog:
>>>
>>> 2014-11-14  Renlin Li  <Renlin.Li@arm.com>
>>>      [BZ #16850]
>>>      * sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h: New.
>>
>> I believe this should be synchronized with the value of this constant
>> in the kernel.
>>
> I got some information from this page. Alan Modra holds the option that
> those two values in glibc could differ from kernel.
>
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1267062

It sounds like the kernel should still be updated in the AArch64 case,
but I think we can safely update glibc independently. The patch is OK
for me.
Marcus Shawcroft Nov. 19, 2014, 4:42 p.m. UTC | #5 
On 14 November 2014 10:15, Renlin Li <renlin.li@arm.com> wrote:
> Hi all,
>
> This patch duplicates sysdeps/unix/sysv/linux/bits/sigstack.h into
> sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h and modifies only SIGSTCKSZ
> and MINSIGSTKSZ.
>
> The ucontext_t is 4560 bytes, siginfo_t is 128 bytes. The required sigframe
> is far larger than the current default MINSIGSTKSZ (which is 2048). Thus, I
> increase MINSIGSTKSZ to 8192, and adjust SIGSTCKSZ accordingly.
>
> Glibc has been build and test on the model, no new issues.
>
> Okay to commit?
>
> ChangeLog:
>
> 2014-11-14  Renlin Li  <Renlin.Li@arm.com>
>     [BZ #16850]
>     * sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h: New.

Catalin,  We need to lift raise the values of MINSIGSTKSZ and
SIGSTCKSZ the former is currently smaller than a signal frame which
makes it  unusable.  These are defined in both glibc and in the kernel
but should be kept in sync.  The proposal in glibc is to lift these to
 8192 and 18384 respectively.  Thoughts?

Cheers
/Marcus
Catalin Marinas Nov. 19, 2014, 4:57 p.m. UTC | #6 
(cc'ing Arnd)

On Wed, Nov 19, 2014 at 04:42:59PM +0000, Marcus Shawcroft wrote:
> On 14 November 2014 10:15, Renlin Li <renlin.li@arm.com> wrote:
> > This patch duplicates sysdeps/unix/sysv/linux/bits/sigstack.h into
> > sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h and modifies only SIGSTCKSZ
> > and MINSIGSTKSZ.
> >
> > The ucontext_t is 4560 bytes, siginfo_t is 128 bytes. The required sigframe
> > is far larger than the current default MINSIGSTKSZ (which is 2048). Thus, I
> > increase MINSIGSTKSZ to 8192, and adjust SIGSTCKSZ accordingly.
> >
> > Glibc has been build and test on the model, no new issues.
> >
> > Okay to commit?
> >
> > ChangeLog:
> >
> > 2014-11-14  Renlin Li  <Renlin.Li@arm.com>
> >     [BZ #16850]
> >     * sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h: New.
> 
> Catalin,  We need to lift raise the values of MINSIGSTKSZ and
> SIGSTCKSZ the former is currently smaller than a signal frame which
> makes it  unusable.  These are defined in both glibc and in the kernel
> but should be kept in sync.  The proposal in glibc is to lift these to
>  8192 and 18384 respectively.  Thoughts?

The arm64 port uses the generic values but they could be defined,
probably with some #ifdef in the generic signal.h file.

There is however a slight ABI change as the do_sigaltstack() Linux
implementation checks whether the user ss_size is smaller than
MINSIGSTKSZ and exits. If we assume that the user code with a 2KB/4KB
ss_size for sigaltstack was already broken, we can make this change. But
I'd like to see Arnd's opinion as well.

Thanks.
diff mbox

Patch

diff --git a/NEWS b/NEWS
index c582c8e..bd02fa9 100644
--- a/NEWS
+++ b/NEWS
@@ -9,10 +9,10 @@  Version 2.21
 
 * The following bugs are resolved with this release:
 
-  6652, 12926, 14132, 14138, 14171, 15215, 15884, 17266, 17344, 17363,
-  17370, 17371, 17411, 17460, 17475, 17485, 17501, 17506, 17508, 17522,
-  17555, 17570, 17571, 17572, 17573, 17574, 17582, 17583, 17584, 17585,
-  17589, 17594.
+  6652, 12926, 14132, 14138, 14171, 15215, 15884, 16850, 17266, 17344,
+  17363, 17370, 17371, 17411, 17460, 17475, 17485, 17501, 17506, 17508,
+  17522, 17555, 17570, 17571, 17572, 17573, 17574, 17582, 17583, 17584,
+  17585, 17589, 17594.
 
 * New locales: tu_IN, bh_IN.
 
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..a73c56e
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/aarch64/bits/sigstack.h
@@ -0,0 +1,54 @@ 
+/* sigstack, sigaltstack definitions.
+   Copyright (C) 1998-2014 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	8192
+
+/* 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;