[PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Manjeet Pawar]

MINSIGSTKSZ and SIGSTKSZ for ARM64 are not correctly set in latest kernel.
This patch fixes this issue.

This issue is reported in LTP (testcase: sigaltstack02.c).
Testcase failed when sigaltstack() called with stack size "MINSIGSTKSZ - 1"
Since in Glibc-2.22, MINSIGSTKSZ is set to 5120 but in kernel
it is set to 2048 so testcase gets failed.

Testcase Output:
sigaltstack02 1  TPASS  :  stgaltstack() fails, Invalid Flag value,errno:22
sigaltstack02 2  TFAIL  :  sigaltstack() returned 0, expected -1,errno:12

Reported Issue in Glibc Bugzilla:
https://sourceware.org/bugzilla/show_bug.cgi?id=16850

Bugfix in Glibc-2.22:
https://sourceware.org/git/gitweb.cgi?p=glibc.git;a=blob;f=sysdeps/unix/
sysv/linux/aarch64/bits/sigstack.h;h=8f2fb76e3e81734ef8a9cf9ae40daf4705
f31c35;hb=b763f6ae859ecea70a5dacb8ad45c71d5f667e2e

Signed-off-by: Akhilesh Kumar <akhilesh.k@samsung.com>
Signed-off-by: Manjeet Pawar <manjeet.p@samsung.com>
Signed-off-by: Rohit Thapliyal <r.thapliyal@samsung.com>

---
v1 -> Changes in uapi overall header
v2 -> Changes done in arm64 headers

 arch/arm64/include/uapi/asm/signal.h |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/include/uapi/asm/signal.h b/arch/arm64/include/uapi/asm/signal.h
index 8d1e723..5ac56cf 100644
--- a/arch/arm64/include/uapi/asm/signal.h
+++ b/arch/arm64/include/uapi/asm/signal.h
@@ -21,4 +21,9 @@
 
 #include <asm-generic/signal.h>
 
+#undef MINSIGSTKSZ
+#undef SIGSTKSZ
+#define MINSIGSTKSZ 5120
+#define SIGSTKSZ    16384
+
 #endif
-- 
1.7.9.5

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Arnd Bergmann]

On Tuesday 06 October 2015 11:05:43 Manjeet Pawar wrote:
> MINSIGSTKSZ and SIGSTKSZ for ARM64 are not correctly set in latest kernel.
> This patch fixes this issue.
> 
> This issue is reported in LTP (testcase: sigaltstack02.c).
> Testcase failed when sigaltstack() called with stack size "MINSIGSTKSZ - 1"
> Since in Glibc-2.22, MINSIGSTKSZ is set to 5120 but in kernel
> it is set to 2048 so testcase gets failed.
> 
> Testcase Output:
> sigaltstack02 1  TPASS  :  stgaltstack() fails, Invalid Flag value,errno:22
> sigaltstack02 2  TFAIL  :  sigaltstack() returned 0, expected -1,errno:12
> 
> Reported Issue in Glibc Bugzilla:
> https://sourceware.org/bugzilla/show_bug.cgi?id=16850
> 
> Bugfix in Glibc-2.22:
> https://sourceware.org/git/gitweb.cgi?p=glibc.git;a=blob;f=sysdeps/unix/
> sysv/linux/aarch64/bits/sigstack.h;h=8f2fb76e3e81734ef8a9cf9ae40daf4705
> f31c35;hb=b763f6ae859ecea70a5dacb8ad45c71d5f667e2e
> 
> Signed-off-by: Akhilesh Kumar <akhilesh.k@samsung.com>
> Signed-off-by: Manjeet Pawar <manjeet.p@samsung.com>
> Signed-off-by: Rohit Thapliyal <r.thapliyal@samsung.com>

This looks correct now. A few more points though:

* My first thought would have been to do this by first defining the
  two symbols before the #include, and then adding an #ifdef in
  the generic file. Both approaches work though, any other opinions
  on this?

* It seems that PowerPC has the same bug. Care to fix that as well?

* Do we need to backport this to stable?

* Can you explain in the changelog how the numbers were decided?
  I don't see any other architecture using 5kb and cannot see why
  it has to be this value rather than something else.

	Arnd

> ---
> v1 -> Changes in uapi overall header
> v2 -> Changes done in arm64 headers
> 
>  arch/arm64/include/uapi/asm/signal.h |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/include/uapi/asm/signal.h b/arch/arm64/include/uapi/asm/signal.h
> index 8d1e723..5ac56cf 100644
> --- a/arch/arm64/include/uapi/asm/signal.h
> +++ b/arch/arm64/include/uapi/asm/signal.h
> @@ -21,4 +21,9 @@
>  
>  #include <asm-generic/signal.h>
>  
> +#undef MINSIGSTKSZ
> +#undef SIGSTKSZ
> +#define MINSIGSTKSZ 5120
> +#define SIGSTKSZ    16384
> +
>  #endif
> 

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Catalin Marinas]

On Tue, Oct 06, 2015 at 09:49:29AM +0200, Arnd Bergmann wrote:
> On Tuesday 06 October 2015 11:05:43 Manjeet Pawar wrote:
> > MINSIGSTKSZ and SIGSTKSZ for ARM64 are not correctly set in latest kernel.
> > This patch fixes this issue.
> > 
> > This issue is reported in LTP (testcase: sigaltstack02.c).
> > Testcase failed when sigaltstack() called with stack size "MINSIGSTKSZ - 1"
> > Since in Glibc-2.22, MINSIGSTKSZ is set to 5120 but in kernel
> > it is set to 2048 so testcase gets failed.
> > 
> > Testcase Output:
> > sigaltstack02 1  TPASS  :  stgaltstack() fails, Invalid Flag value,errno:22
> > sigaltstack02 2  TFAIL  :  sigaltstack() returned 0, expected -1,errno:12
> > 
> > Reported Issue in Glibc Bugzilla:
> > https://sourceware.org/bugzilla/show_bug.cgi?id=16850
> > 
> > Bugfix in Glibc-2.22:
> > https://sourceware.org/git/gitweb.cgi?p=glibc.git;a=blob;f=sysdeps/unix/
> > sysv/linux/aarch64/bits/sigstack.h;h=8f2fb76e3e81734ef8a9cf9ae40daf4705
> > f31c35;hb=b763f6ae859ecea70a5dacb8ad45c71d5f667e2e
> > 
> > Signed-off-by: Akhilesh Kumar <akhilesh.k@samsung.com>
> > Signed-off-by: Manjeet Pawar <manjeet.p@samsung.com>
> > Signed-off-by: Rohit Thapliyal <r.thapliyal@samsung.com>
> 
> This looks correct now. A few more points though:
> 
> * My first thought would have been to do this by first defining the
>   two symbols before the #include, and then adding an #ifdef in
>   the generic file. Both approaches work though, any other opinions
>   on this?

That's what I was thinking as well. Maybe with a single #ifndef
MINSIGSTKSZ to cover both macros.

> * Do we need to backport this to stable?

I think it does.

-- 
Catalin

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Dave Martin]

On Tue, Oct 06, 2015 at 09:49:29AM +0200, Arnd Bergmann wrote:
> On Tuesday 06 October 2015 11:05:43 Manjeet Pawar wrote:
> > MINSIGSTKSZ and SIGSTKSZ for ARM64 are not correctly set in latest kernel.
> > This patch fixes this issue.
> > 
> > This issue is reported in LTP (testcase: sigaltstack02.c).
> > Testcase failed when sigaltstack() called with stack size "MINSIGSTKSZ - 1"
> > Since in Glibc-2.22, MINSIGSTKSZ is set to 5120 but in kernel
> > it is set to 2048 so testcase gets failed.
> > 
> > Testcase Output:
> > sigaltstack02 1  TPASS  :  stgaltstack() fails, Invalid Flag value,errno:22
> > sigaltstack02 2  TFAIL  :  sigaltstack() returned 0, expected -1,errno:12
> > 
> > Reported Issue in Glibc Bugzilla:
> > https://sourceware.org/bugzilla/show_bug.cgi?id=16850
> > 
> > Bugfix in Glibc-2.22:
> > https://sourceware.org/git/gitweb.cgi?p=glibc.git;a=blob;f=sysdeps/unix/
> > sysv/linux/aarch64/bits/sigstack.h;h=8f2fb76e3e81734ef8a9cf9ae40daf4705
> > f31c35;hb=b763f6ae859ecea70a5dacb8ad45c71d5f667e2e
> > 
> > Signed-off-by: Akhilesh Kumar <akhilesh.k@samsung.com>
> > Signed-off-by: Manjeet Pawar <manjeet.p@samsung.com>
> > Signed-off-by: Rohit Thapliyal <r.thapliyal@samsung.com>
> 
> This looks correct now. A few more points though:
> 
> * My first thought would have been to do this by first defining the
>   two symbols before the #include, and then adding an #ifdef in
>   the generic file. Both approaches work though, any other opinions
>   on this?
> 
> * It seems that PowerPC has the same bug. Care to fix that as well?
> 
> * Do we need to backport this to stable?
> 
> * Can you explain in the changelog how the numbers were decided?
>   I don't see any other architecture using 5kb and cannot see why
>   it has to be this value rather than something else.

glibc quietly "fixed" this earlier this year, by inventing these numbers
and putting them in the glibc headers. [1]

Except for a moribund architecture that will never be extended I
think that the idea of MINSIGSTKSZ is badly flawed -- a #define
for not-necessarily-quite-enough-stack-to-realistically-take-a-signal
is a pretty useless concept even if the signal frame never grows, and
it looks like it is little used in practice.

Most arches are fairly generous with SIGSTKSZ, though there is no
correct answer for exactly how generous they should be in order to
be future proof.  (ia64 is a case in point, where an generous but
misspelled number got baked in as ABI -- after misspelling the
number was still generous; the precise value is irrelevant).

Since this bug hasn't been reported until now, I suspect that
MINSIGSTKSZ is used very rarely or not at all by real userspace
software.  I wonder whether we can get away with simply raising
MINSIGSTKSZ to match SIGSTKSZ, since it's clear that any software
using MINSIGSTKSZ was already broken.

[1] https://sourceware.org/ml/libc-alpha/2015-04/msg00033.html

Cheers
---Dave

[...]

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Arnd Bergmann]

On Tuesday 06 October 2015 11:31:34 Dave Martin wrote:
> On Tue, Oct 06, 2015 at 09:49:29AM +0200, Arnd Bergmann wrote:

> > * Can you explain in the changelog how the numbers were decided?
> >   I don't see any other architecture using 5kb and cannot see why
> >   it has to be this value rather than something else.
> 
> glibc quietly "fixed" this earlier this year, by inventing these numbers
> and putting them in the glibc headers. [1]

I saw the commit, but the changelog is not really useful.

> Except for a moribund architecture that will never be extended I
> think that the idea of MINSIGSTKSZ is badly flawed -- a #define
> for not-necessarily-quite-enough-stack-to-realistically-take-a-signal
> is a pretty useless concept even if the signal frame never grows, and
> it looks like it is little used in practice.

Right, even if we modified the constants in the kernel/glibc
headers at that point, it would remain broken for new kernels
and old user space.

> Since this bug hasn't been reported until now, I suspect that
> MINSIGSTKSZ is used very rarely or not at all by real userspace
> software.  I wonder whether we can get away with simply raising
> MINSIGSTKSZ to match SIGSTKSZ, since it's clear that any software
> using MINSIGSTKSZ was already broken.

I think it makes sense to stick with the traditional definition
of MINSIGSTKSZ == "the minimum amount that you will always need,
add whatever you require yourself" and SIGSTKSZ == "Should be
enough for a couple of function calls". If we want to be conservative
in the kernel, using 8192 and 32768, to stay with the x4 ratio
that everyone else uses would be my first pick, though I'm not
particularly attached to those values.

	Arnd

> [1] https://sourceware.org/ml/libc-alpha/2015-04/msg00033.html

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Andreas Schwab]

Arnd Bergmann <arnd@arndb.de> writes:

> I think it makes sense to stick with the traditional definition
> of MINSIGSTKSZ == "the minimum amount that you will always need,
> add whatever you require yourself" and SIGSTKSZ == "Should be
> enough for a couple of function calls".

The python3 testsuite wants to put two signal frames in a SIGSTKSZ
stack.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Dave Martin]

On Tue, Oct 06, 2015 at 12:59:45PM +0200, Andreas Schwab wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > I think it makes sense to stick with the traditional definition
> > of MINSIGSTKSZ == "the minimum amount that you will always need,
> > add whatever you require yourself" and SIGSTKSZ == "Should be
> > enough for a couple of function calls".
> 
> The python3 testsuite wants to put two signal frames in a SIGSTKSZ
> stack.

Whether it's valid to expect SIGSTKSZ to be big enough for that is
debatable.

But I guess that SIGSTKSZ = MINSIGSTKSZ * 4 provides some insurance
against such assumptions (doubtless the python testsuite is not
the only code affected).

Cheers
---Dave

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Andreas Schwab]

Dave Martin <Dave.Martin@arm.com> writes:

> On Tue, Oct 06, 2015 at 12:59:45PM +0200, Andreas Schwab wrote:
>> Arnd Bergmann <arnd@arndb.de> writes:
>> 
>> > I think it makes sense to stick with the traditional definition
>> > of MINSIGSTKSZ == "the minimum amount that you will always need,
>> > add whatever you require yourself" and SIGSTKSZ == "Should be
>> > enough for a couple of function calls".
>> 
>> The python3 testsuite wants to put two signal frames in a SIGSTKSZ
>> stack.
>
> Whether it's valid to expect SIGSTKSZ to be big enough for that is
> debatable.

This is tracked in <https://bugs.python.org/issue22503>.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Dave Martin]

On Tue, Oct 06, 2015 at 01:52:24PM +0200, Andreas Schwab wrote:
> Dave Martin <Dave.Martin@arm.com> writes:
> 
> > On Tue, Oct 06, 2015 at 12:59:45PM +0200, Andreas Schwab wrote:
> >> Arnd Bergmann <arnd@arndb.de> writes:
> >> 
> >> > I think it makes sense to stick with the traditional definition
> >> > of MINSIGSTKSZ == "the minimum amount that you will always need,
> >> > add whatever you require yourself" and SIGSTKSZ == "Should be
> >> > enough for a couple of function calls".
> >> 
> >> The python3 testsuite wants to put two signal frames in a SIGSTKSZ
> >> stack.
> >
> > Whether it's valid to expect SIGSTKSZ to be big enough for that is
> > debatable.
> 
> This is tracked in <https://bugs.python.org/issue22503>.

Fair enough...

Cheers
---Dave

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Dave Martin]

On Tue, Oct 06, 2015 at 12:51:24PM +0200, Arnd Bergmann wrote:
> On Tuesday 06 October 2015 11:31:34 Dave Martin wrote:
> > On Tue, Oct 06, 2015 at 09:49:29AM +0200, Arnd Bergmann wrote:
> 
> > > * Can you explain in the changelog how the numbers were decided?
> > >   I don't see any other architecture using 5kb and cannot see why
> > >   it has to be this value rather than something else.
> > 
> > glibc quietly "fixed" this earlier this year, by inventing these numbers
> > and putting them in the glibc headers. [1]
> 
> I saw the commit, but the changelog is not really useful.
> 
> > Except for a moribund architecture that will never be extended I
> > think that the idea of MINSIGSTKSZ is badly flawed -- a #define
> > for not-necessarily-quite-enough-stack-to-realistically-take-a-signal
> > is a pretty useless concept even if the signal frame never grows, and
> > it looks like it is little used in practice.
> 
> Right, even if we modified the constants in the kernel/glibc
> headers at that point, it would remain broken for new kernels
> and old user space.
> 
> > Since this bug hasn't been reported until now, I suspect that
> > MINSIGSTKSZ is used very rarely or not at all by real userspace
> > software.  I wonder whether we can get away with simply raising
> > MINSIGSTKSZ to match SIGSTKSZ, since it's clear that any software
> > using MINSIGSTKSZ was already broken.
> 
> I think it makes sense to stick with the traditional definition
> of MINSIGSTKSZ == "the minimum amount that you will always need,
> add whatever you require yourself" and SIGSTKSZ == "Should be
> enough for a couple of function calls". If we want to be conservative
> in the kernel, using 8192 and 32768, to stay with the x4 ratio

That "x4" only makes sense if you expect to put copies of the signal
frame on the stack during handling -- otherwise the extra overhead
won't scale proportional to MINSIGSTKSZ at all.  OTOH there's no
better answer I can think of...

> that everyone else uses would be my first pick, though I'm not
> particularly attached to those values.

Maybe we could do something like that as a stopgap solution, while
coming up with a runtime discovery mechanism independently of this
patch.  The latter would require some buy-in from glibc, so I guess
it couldn't happen overnight.

Cheers
---Dave

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Arnd Bergmann]

On Tuesday 06 October 2015 12:51:24 Arnd Bergmann wrote:
> 
> I think it makes sense to stick with the traditional definition
> of MINSIGSTKSZ == "the minimum amount that you will always need,
> add whatever you require yourself" and SIGSTKSZ == "Should be
> enough for a couple of function calls". If we want to be conservative
> in the kernel, using 8192 and 32768, to stay with the x4 ratio
> that everyone else uses would be my first pick, though I'm not
> particularly attached to those values.
> 
> 

On second thought, it really seems to late to make up our minds
about the size now that glibc has already established 5KB as the
minimum size. If we set it to 8KB/32KB, not just the testcase but
real applications would start failing when they use the 5KB
constant from glibc.

	Arnd

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Dave Martin]

On Fri, Oct 09, 2015 at 10:41:43AM +0200, Arnd Bergmann wrote:
> On Tuesday 06 October 2015 12:51:24 Arnd Bergmann wrote:
> > 
> > I think it makes sense to stick with the traditional definition
> > of MINSIGSTKSZ == "the minimum amount that you will always need,
> > add whatever you require yourself" and SIGSTKSZ == "Should be
> > enough for a couple of function calls". If we want to be conservative
> > in the kernel, using 8192 and 32768, to stay with the x4 ratio
> > that everyone else uses would be my first pick, though I'm not
> > particularly attached to those values.
> > 
> > 
> 
> On second thought, it really seems to late to make up our minds
> about the size now that glibc has already established 5KB as the
> minimum size. If we set it to 8KB/32KB, not just the testcase but
> real applications would start failing when they use the 5KB
> constant from glibc.

I agree for MINSIGSTKSZ.  We could still raise SIGSTKSZ if we think
that will be more future-proof (SIGSTKSZ would be less than the magic
4*MINSIGSTKSZ that most arches assume, unless SIGSTKSZ is made >=20KB).

Those might be independent changes.  The definition of MINSIGSTKSZ
is definitely broken right now, whereas SIGSTKSZ could be debated,
but isn't actually broken.

Cheers
---Dave

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Will Deacon]

On Tue, Oct 06, 2015 at 11:05:43AM +0530, Manjeet Pawar wrote:
> MINSIGSTKSZ and SIGSTKSZ for ARM64 are not correctly set in latest kernel.
> This patch fixes this issue.
> 
> This issue is reported in LTP (testcase: sigaltstack02.c).
> Testcase failed when sigaltstack() called with stack size "MINSIGSTKSZ - 1"
> Since in Glibc-2.22, MINSIGSTKSZ is set to 5120 but in kernel
> it is set to 2048 so testcase gets failed.
> 
> Testcase Output:
> sigaltstack02 1  TPASS  :  stgaltstack() fails, Invalid Flag value,errno:22
> sigaltstack02 2  TFAIL  :  sigaltstack() returned 0, expected -1,errno:12

This test passes for me. What are you doing to trigger the failure?

Will

Re: [PATCHv2] ARM64:Fix MINSIGSTKSZ and SIGSTKSZ

[Manjeet Pawar]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 704 bytes --]

  
>> This looks correct now. A few more points though:
>> 
>> * My first thought would have been to do this by first defining the
>>   two symbols before the #include, and then adding an #ifdef in
>>   the generic file. Both approaches work though, any other opinions
>>   on this?

>That's what I was thinking as well. Maybe with a single #ifndef
>MINSIGSTKSZ to cover both macros.

I am sharing another patch which adds both macro in arm64/.../signal.h with new values and put a check with #ifndef in uapi/.../signal.h.
       
  
         
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥