[PATCH] Fix the use of sigaltstack to return to the saved main stack.

[PATCH] Fix the use of sigaltstack to return to the saved main stack.

[James Y Knight]

[-- Attachment #1.1: Type: text/plain, Size: 392 bytes --]

Previously, musl would reject the call, because the main stack has
ss_size == 0 and ss_flags == SS_DISABLE.

We could condition on ss_flags not containing SS_DISABLE, but instead,
simply remove the ss_size check, as the kernel performs the same check,
anyhow.

[[Note: I've also included the corresponding patch to libc-tests. I'm not
sure if this is OK, or if I should send it separately?]]

[-- Attachment #1.2: Type: text/html, Size: 476 bytes --]

[-- Attachment #2: 0001-Fix-the-use-of-sigaltstack-to-return-to-the-saved-ma.patch --]
[-- Type: text/x-patch, Size: 1162 bytes --]

From fe70a508fe945cb1a44f8a6bbd87ee295637447b Mon Sep 17 00:00:00 2001
From: James Y Knight <jyknight@google.com>
Date: Tue, 9 Jul 2019 14:59:01 -0400
Subject: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

Previously, musl would reject the call, because the main stack has
ss_size == 0 and ss_flags == SS_DISABLE.

We could condition on ss_flags not containing SS_DISABLE, but instead,
simply remove the ss_size check, as the kernel performs the same check,
anyhow.
---
 src/signal/sigaltstack.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/signal/sigaltstack.c b/src/signal/sigaltstack.c
index cfa3f5c1..d8e8eb0b 100644
--- a/src/signal/sigaltstack.c
+++ b/src/signal/sigaltstack.c
@@ -4,15 +4,9 @@
 
 int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
 {
-	if (ss) {
-		if (ss->ss_size < MINSIGSTKSZ) {
-			errno = ENOMEM;
-			return -1;
-		}
-		if (ss->ss_flags & SS_ONSTACK) {
-			errno = EINVAL;
-			return -1;
-		}
+	if (ss && (ss->ss_flags & SS_ONSTACK)) {
+		errno = EINVAL;
+		return -1;
 	}
 	return syscall(SYS_sigaltstack, ss, old);
 }
-- 
2.22.0.410.gd8fdbe21b5-goog


[-- Attachment #3: 0001-Verify-that-returning-to-the-original-stack-doesn-t-.patch --]
[-- Type: text/x-patch, Size: 1165 bytes --]

From 9272ed8666f930dc2f24d05b1fd248584bbf495f Mon Sep 17 00:00:00 2001
From: James Y Knight <jyknight@google.com>
Date: Tue, 9 Jul 2019 14:31:24 -0400
Subject: [PATCH] Verify that returning to the original stack doesn't return an
 error (e.g. ENOMEM).

---
 src/regression/sigaltstack.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/regression/sigaltstack.c b/src/regression/sigaltstack.c
index bfdc44a..6847454 100644
--- a/src/regression/sigaltstack.c
+++ b/src/regression/sigaltstack.c
@@ -30,7 +30,7 @@ static void handler(int sig)
 
 int main(void)
 {
-	stack_t ss;
+	stack_t ss, oldss;
 	struct sigaction sa;
 
 	ss.ss_sp = stack;
@@ -39,7 +39,7 @@ int main(void)
 	sa.sa_handler = handler;
 	sa.sa_flags = SA_ONSTACK;
 
-	T(sigaltstack(&ss, 0));
+	T(sigaltstack(&ss, &oldss));
 	T(sigfillset(&sa.sa_mask));
 	T(sigaction(SIGUSR1, &sa, 0));
 	T(raise(SIGUSR1));
@@ -56,7 +56,7 @@ int main(void)
 		t_error("sigaltstack with bad ss_flags should have failed with EINVAL, "
 			"got %s\n", strerror(errno));
 	errno = 0;
-	T(sigaltstack(0, 0));
+	T(sigaltstack(oldss, 0));
 
 	return t_status;
 }
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

[Rich Felker]

On Tue, Jul 09, 2019 at 03:01:50PM -0400, James Y Knight wrote:
> Previously, musl would reject the call, because the main stack has
> ss_size == 0 and ss_flags == SS_DISABLE.
> 
> We could condition on ss_flags not containing SS_DISABLE, but instead,
> simply remove the ss_size check, as the kernel performs the same check,
> anyhow.

Are you sure the kernel does? I'm pretty sure the reason the code is
here is that the kernel either does not check it, or does not perform
the check correctly in some special case. Sadly the commit messages in
musl were not as good back at the time when the code was written.

Unless we have good evidence the test isn't needed, I think the right
check is just making the error conditional on !(ss_flags &
SS_DISABLE). POSIX specifies: "If it is set to SS_DISABLE, the stack
is disabled and ss_sp and ss_size are ignored." Here, "set to" is
probably something the resolution of Austin Group issue 1187 failed to
fix; it should probably be "includes" rather than "is set to". But I'm
not sure it makes sense to have any flags set alongside SS_DISABLE
anyway.

> [[Note: I've also included the corresponding patch to libc-tests. I'm not
> sure if this is OK, or if I should send it separately?]]

I think that's fine.

Rich



> From fe70a508fe945cb1a44f8a6bbd87ee295637447b Mon Sep 17 00:00:00 2001
> From: James Y Knight <jyknight@google.com>
> Date: Tue, 9 Jul 2019 14:59:01 -0400
> Subject: [PATCH] Fix the use of sigaltstack to return to the saved main stack.
> 
> Previously, musl would reject the call, because the main stack has
> ss_size == 0 and ss_flags == SS_DISABLE.
> 
> We could condition on ss_flags not containing SS_DISABLE, but instead,
> simply remove the ss_size check, as the kernel performs the same check,
> anyhow.
> ---
>  src/signal/sigaltstack.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/src/signal/sigaltstack.c b/src/signal/sigaltstack.c
> index cfa3f5c1..d8e8eb0b 100644
> --- a/src/signal/sigaltstack.c
> +++ b/src/signal/sigaltstack.c
> @@ -4,15 +4,9 @@
>  
>  int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
>  {
> -	if (ss) {
> -		if (ss->ss_size < MINSIGSTKSZ) {
> -			errno = ENOMEM;
> -			return -1;
> -		}
> -		if (ss->ss_flags & SS_ONSTACK) {
> -			errno = EINVAL;
> -			return -1;
> -		}
> +	if (ss && (ss->ss_flags & SS_ONSTACK)) {
> +		errno = EINVAL;
> +		return -1;
>  	}
>  	return syscall(SYS_sigaltstack, ss, old);
>  }
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
> 

> From 9272ed8666f930dc2f24d05b1fd248584bbf495f Mon Sep 17 00:00:00 2001
> From: James Y Knight <jyknight@google.com>
> Date: Tue, 9 Jul 2019 14:31:24 -0400
> Subject: [PATCH] Verify that returning to the original stack doesn't return an
>  error (e.g. ENOMEM).
> 
> ---
>  src/regression/sigaltstack.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/regression/sigaltstack.c b/src/regression/sigaltstack.c
> index bfdc44a..6847454 100644
> --- a/src/regression/sigaltstack.c
> +++ b/src/regression/sigaltstack.c
> @@ -30,7 +30,7 @@ static void handler(int sig)
>  
>  int main(void)
>  {
> -	stack_t ss;
> +	stack_t ss, oldss;
>  	struct sigaction sa;
>  
>  	ss.ss_sp = stack;
> @@ -39,7 +39,7 @@ int main(void)
>  	sa.sa_handler = handler;
>  	sa.sa_flags = SA_ONSTACK;
>  
> -	T(sigaltstack(&ss, 0));
> +	T(sigaltstack(&ss, &oldss));
>  	T(sigfillset(&sa.sa_mask));
>  	T(sigaction(SIGUSR1, &sa, 0));
>  	T(raise(SIGUSR1));
> @@ -56,7 +56,7 @@ int main(void)
>  		t_error("sigaltstack with bad ss_flags should have failed with EINVAL, "
>  			"got %s\n", strerror(errno));
>  	errno = 0;
> -	T(sigaltstack(0, 0));
> +	T(sigaltstack(oldss, 0));
>  
>  	return t_status;
>  }
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
> 

Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

[James Y Knight]

[-- Attachment #1: Type: text/plain, Size: 5321 bytes --]

On Tue, Jul 9, 2019 at 3:30 PM Rich Felker <dalias@libc.org> wrote:

> On Tue, Jul 09, 2019 at 03:01:50PM -0400, James Y Knight wrote:
> > Previously, musl would reject the call, because the main stack has
> > ss_size == 0 and ss_flags == SS_DISABLE.
> >
> > We could condition on ss_flags not containing SS_DISABLE, but instead,
> > simply remove the ss_size check, as the kernel performs the same check,
> > anyhow.
>
> Are you sure the kernel does? I'm pretty sure the reason the code is
> here is that the kernel either does not check it, or does not perform
> the check correctly in some special case. Sadly the commit messages in
> musl were not as good back at the time when the code was written.
>

As far back as the first git version (2.6.12-rc2), the kernel checks this
condition. I haven't looked back any further...

However, I note now that musl uses different values for MINSIGSTKSIZE than
the kernel does, on some architectures.

The usual value in the kernel is 2048. Only a few architectures set a
differing value:
alpha:4096
arm64:5120
ia64:131027
sparc:4096

Musl usually uses 2048 as well, but sets other values on these
architectures:
arm64:6144
powerpc:4096
powerpc64:4096
s390x:4096

(Musl doesn't support alpha, ia64, or sparc, so it's not using a lower
value than the kernel anywhere, at least).

If it's important that stacks smaller than musl's MINSIGSTKSIZE be
rejected, despite them being large enough for the kernel, then I suppose
the check should be retained. Let me know -- I'll make a new patch
implementing your suggestion if you still think that's the way to go.

Unless we have good evidence the test isn't needed, I think the right
> check is just making the error conditional on !(ss_flags &
> SS_DISABLE). POSIX specifies: "If it is set to SS_DISABLE, the stack
> is disabled and ss_sp and ss_size are ignored." Here, "set to" is
> probably something the resolution of Austin Group issue 1187 failed to
> fix; it should probably be "includes" rather than "is set to". But I'm
> not sure it makes sense to have any flags set alongside SS_DISABLE
> anyway.


>
> [[Note: I've also included the corresponding patch to libc-tests. I'm not
> > sure if this is OK, or if I should send it separately?]]
>
> I think that's fine.
>
> Rich
>
>
>
> > From fe70a508fe945cb1a44f8a6bbd87ee295637447b Mon Sep 17 00:00:00 2001
> > From: James Y Knight <jyknight@google.com>
> > Date: Tue, 9 Jul 2019 14:59:01 -0400
> > Subject: [PATCH] Fix the use of sigaltstack to return to the saved main
> stack.
> >
> > Previously, musl would reject the call, because the main stack has
> > ss_size == 0 and ss_flags == SS_DISABLE.
> >
> > We could condition on ss_flags not containing SS_DISABLE, but instead,
> > simply remove the ss_size check, as the kernel performs the same check,
> > anyhow.
> > ---
> >  src/signal/sigaltstack.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/signal/sigaltstack.c b/src/signal/sigaltstack.c
> > index cfa3f5c1..d8e8eb0b 100644
> > --- a/src/signal/sigaltstack.c
> > +++ b/src/signal/sigaltstack.c
> > @@ -4,15 +4,9 @@
> >
> >  int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
> >  {
> > -     if (ss) {
> > -             if (ss->ss_size < MINSIGSTKSZ) {
> > -                     errno = ENOMEM;
> > -                     return -1;
> > -             }
> > -             if (ss->ss_flags & SS_ONSTACK) {
> > -                     errno = EINVAL;
> > -                     return -1;
> > -             }
> > +     if (ss && (ss->ss_flags & SS_ONSTACK)) {
> > +             errno = EINVAL;
> > +             return -1;
> >       }
> >       return syscall(SYS_sigaltstack, ss, old);
> >  }
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >
>
> > From 9272ed8666f930dc2f24d05b1fd248584bbf495f Mon Sep 17 00:00:00 2001
> > From: James Y Knight <jyknight@google.com>
> > Date: Tue, 9 Jul 2019 14:31:24 -0400
> > Subject: [PATCH] Verify that returning to the original stack doesn't
> return an
> >  error (e.g. ENOMEM).
> >
> > ---
> >  src/regression/sigaltstack.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/regression/sigaltstack.c b/src/regression/sigaltstack.c
> > index bfdc44a..6847454 100644
> > --- a/src/regression/sigaltstack.c
> > +++ b/src/regression/sigaltstack.c
> > @@ -30,7 +30,7 @@ static void handler(int sig)
> >
> >  int main(void)
> >  {
> > -     stack_t ss;
> > +     stack_t ss, oldss;
> >       struct sigaction sa;
> >
> >       ss.ss_sp = stack;
> > @@ -39,7 +39,7 @@ int main(void)
> >       sa.sa_handler = handler;
> >       sa.sa_flags = SA_ONSTACK;
> >
> > -     T(sigaltstack(&ss, 0));
> > +     T(sigaltstack(&ss, &oldss));
> >       T(sigfillset(&sa.sa_mask));
> >       T(sigaction(SIGUSR1, &sa, 0));
> >       T(raise(SIGUSR1));
> > @@ -56,7 +56,7 @@ int main(void)
> >               t_error("sigaltstack with bad ss_flags should have failed
> with EINVAL, "
> >                       "got %s\n", strerror(errno));
> >       errno = 0;
> > -     T(sigaltstack(0, 0));
> > +     T(sigaltstack(oldss, 0));
>

(And sorry -- just noticed that I forgot to commit the last fix in this
repo, before generating this patch. That should've said "&oldss".)

>
> >       return t_status;
> >  }
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >
>
>

[-- Attachment #2: Type: text/html, Size: 7566 bytes --]

Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

[Rich Felker]

On Wed, Jul 10, 2019 at 02:04:18PM -0400, James Y Knight wrote:
> On Tue, Jul 9, 2019 at 3:30 PM Rich Felker <dalias@libc.org> wrote:
> 
> > On Tue, Jul 09, 2019 at 03:01:50PM -0400, James Y Knight wrote:
> > > Previously, musl would reject the call, because the main stack has
> > > ss_size == 0 and ss_flags == SS_DISABLE.
> > >
> > > We could condition on ss_flags not containing SS_DISABLE, but instead,
> > > simply remove the ss_size check, as the kernel performs the same check,
> > > anyhow.
> >
> > Are you sure the kernel does? I'm pretty sure the reason the code is
> > here is that the kernel either does not check it, or does not perform
> > the check correctly in some special case. Sadly the commit messages in
> > musl were not as good back at the time when the code was written.
> >
> 
> As far back as the first git version (2.6.12-rc2), the kernel checks this
> condition. I haven't looked back any further...
> 
> However, I note now that musl uses different values for MINSIGSTKSIZE than
> the kernel does, on some architectures.
> 
> The usual value in the kernel is 2048. Only a few architectures set a
> differing value:
> alpha:4096
> arm64:5120
> ia64:131027
> sparc:4096
> 
> Musl usually uses 2048 as well, but sets other values on these
> architectures:
> arm64:6144
> powerpc:4096
> powerpc64:4096
> s390x:4096
> 
> (Musl doesn't support alpha, ia64, or sparc, so it's not using a lower
> value than the kernel anywhere, at least).
> 
> If it's important that stacks smaller than musl's MINSIGSTKSIZE be
> rejected, despite them being large enough for the kernel, then I suppose
> the check should be retained. Let me know -- I'll make a new patch
> implementing your suggestion if you still think that's the way to go.

It is important. It's both a normative requirement of POSIX, and a
matter of the smaller sizes accepted by the kernel being unsafe --
they don't actually fit a signal context due to the arch's register
file being huge, or having a reservation that it might be huge in the
future.

Rich

Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

[James Y Knight]

[-- Attachment #1.1: Type: text/plain, Size: 848 bytes --]

Updated patches attached.

W.r.t.:
> Here, "set to" is
> probably something the resolution of Austin Group issue 1187 failed to
> fix; it should probably be "includes" rather than "is set to". But I'm
> not sure it makes sense to have any flags set alongside SS_DISABLE
> anyway.

While the SS_AUTODISARM flag has no effect if specified alongside
SS_DISABLE, the kernel still accepts and stores it. So A subsequent call to
sigaltstack can return SS_DISABLE|SS_AUTODISARM in the "old" flags value.
To avoid the case where the old value returned from sigaltstack is not
accepted back as the input, I used the "includes" semantics here.


On Wed, Jul 10, 2019 at 2:39 PM Rich Felker <dalias@libc.org> wrote:

> On Wed, Jul 10, 2019 at 02:04:18PM -0400, James Y Knight wrote:
> > On Tue, Jul 9, 2019 at 3:30 PM Rich Felker <dalias@libc.org> wrote:
>
>

[-- Attachment #1.2: Type: text/html, Size: 1371 bytes --]

[-- Attachment #2: 0001-Fix-the-use-of-sigaltstack-to-return-to-the-saved-ma.patch --]
[-- Type: text/x-patch, Size: 1347 bytes --]

From f6a83b477b5b1b7bd1706327f9d6de19b6efe386 Mon Sep 17 00:00:00 2001
From: James Y Knight <jyknight@google.com>
Date: Wed, 10 Jul 2019 15:22:58 -0400
Subject: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

Previously, musl would reject the call with -ENOMEM, because the main
stack typically has ss_size == 0 and ss_flags == SS_DISABLE.
---
 src/signal/sigaltstack.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/signal/sigaltstack.c b/src/signal/sigaltstack.c
index cfa3f5c1..b0d799d2 100644
--- a/src/signal/sigaltstack.c
+++ b/src/signal/sigaltstack.c
@@ -4,11 +4,19 @@
 
 int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
 {
+	// We must check requirements which Linux fails to verify in the syscall
+	// itself.
 	if (ss) {
-		if (ss->ss_size < MINSIGSTKSZ) {
+		// The syscall does already check against MINSIGSTKSZ, however,
+		// the kernel's value is smaller than musl's value on some
+		// architectures. Thus, although this check may appear
+		// redundant, it is not.
+		if (!(ss->ss_flags & SS_DISABLE) && ss->ss_size < MINSIGSTKSZ) {
 			errno = ENOMEM;
 			return -1;
 		}
+		// Linux ignores SS_ONSTACK on input, but POSIX requires an
+		// error.
 		if (ss->ss_flags & SS_ONSTACK) {
 			errno = EINVAL;
 			return -1;
-- 
2.22.0.410.gd8fdbe21b5-goog


[-- Attachment #3: 0001-Verify-that-returning-to-the-original-stack-doesn-t-.patch --]
[-- Type: text/x-patch, Size: 1166 bytes --]

From 313618c8e00df106528e24cabc38379b4783f51e Mon Sep 17 00:00:00 2001
From: James Y Knight <jyknight@google.com>
Date: Tue, 9 Jul 2019 14:31:24 -0400
Subject: [PATCH] Verify that returning to the original stack doesn't return an
 error (e.g. ENOMEM).

---
 src/regression/sigaltstack.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/regression/sigaltstack.c b/src/regression/sigaltstack.c
index bfdc44a..2bfe329 100644
--- a/src/regression/sigaltstack.c
+++ b/src/regression/sigaltstack.c
@@ -30,7 +30,7 @@ static void handler(int sig)
 
 int main(void)
 {
-	stack_t ss;
+	stack_t ss, oldss;
 	struct sigaction sa;
 
 	ss.ss_sp = stack;
@@ -39,7 +39,7 @@ int main(void)
 	sa.sa_handler = handler;
 	sa.sa_flags = SA_ONSTACK;
 
-	T(sigaltstack(&ss, 0));
+	T(sigaltstack(&ss, &oldss));
 	T(sigfillset(&sa.sa_mask));
 	T(sigaction(SIGUSR1, &sa, 0));
 	T(raise(SIGUSR1));
@@ -56,7 +56,7 @@ int main(void)
 		t_error("sigaltstack with bad ss_flags should have failed with EINVAL, "
 			"got %s\n", strerror(errno));
 	errno = 0;
-	T(sigaltstack(0, 0));
+	T(sigaltstack(&oldss, 0));
 
 	return t_status;
 }
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

[Szabolcs Nagy]

* James Y Knight <jyknight@google.com> [2019-07-10 16:11:23 -0400]:
>  int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
>  {
> +	// We must check requirements which Linux fails to verify in the syscall
> +	// itself.
>  	if (ss) {
> -		if (ss->ss_size < MINSIGSTKSZ) {
> +		// The syscall does already check against MINSIGSTKSZ, however,
> +		// the kernel's value is smaller than musl's value on some
> +		// architectures. Thus, although this check may appear
> +		// redundant, it is not.

the comment does not make sense to me, the check is obviously
not redundant.

MINSIGSTKSZ is a libc api, has nothing to do with the kernel

the kernel also defines a MINSIGSZTKSZ but musl is an
abstraction layer higher, the linux limit should not be
observable to users, only the limit defined by musl,
which ensures not only that the kernel can deliver a
signal but also reserves space of any current or future
hackery the c runtime may need to do around signal handling,
so that trivial c language signal handler is guaranteed
to work.

this is the only reasonable way to make such limit useful.
if it were only a kernel limit, then application code would
have to guess the libc signal handling overhead and add that
to the MINSIGSZTKSZ when allocating signal stacks.

Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

[Rich Felker]

On Wed, Jul 10, 2019 at 11:23:19PM +0200, Szabolcs Nagy wrote:
> * James Y Knight <jyknight@google.com> [2019-07-10 16:11:23 -0400]:
> >  int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
> >  {
> > +	// We must check requirements which Linux fails to verify in the syscall
> > +	// itself.
> >  	if (ss) {
> > -		if (ss->ss_size < MINSIGSTKSZ) {
> > +		// The syscall does already check against MINSIGSTKSZ, however,
> > +		// the kernel's value is smaller than musl's value on some
> > +		// architectures. Thus, although this check may appear
> > +		// redundant, it is not.
> 
> the comment does not make sense to me, the check is obviously
> not redundant.

Yes. Also, in musl, we generally document motivations like this as
part of commit messages rather than comments. This ties them to the
timeline of changes, to the author, and prevents them from sticking
around when code changes and they no longer make sense.

James, could you submit this patch just as the minimal change to
correct the current bug? If additional documentation of why things are
the way they are is needed that can be done separately.

> MINSIGSTKSZ is a libc api, has nothing to do with the kernel
> 
> the kernel also defines a MINSIGSZTKSZ but musl is an
> abstraction layer higher, the linux limit should not be
> observable to users, only the limit defined by musl,
> which ensures not only that the kernel can deliver a
> signal but also reserves space of any current or future
> hackery the c runtime may need to do around signal handling,
> so that trivial c language signal handler is guaranteed
> to work.
> 
> this is the only reasonable way to make such limit useful.
> if it were only a kernel limit, then application code would
> have to guess the libc signal handling overhead and add that
> to the MINSIGSZTKSZ when allocating signal stacks.

In this case it's more that the kernel values are just wrong. libc
isn't imposing a stronger limit here because of libc code needing
stack, but because the kernel values don't account for signal frame
size. The kernel values presumably can't be changed because the
syscall interface is stable/locked, and it's risky to change for libc
too after it's set (see the issue with whether the x86 values are
right in the presence of AVX512 -- that's why on later archs we
imposed stronger limits).

Rich

Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

[James Y Knight]

[-- Attachment #1.1: Type: text/plain, Size: 3944 bytes --]

On Wed, Jul 10, 2019 at 5:48 PM Rich Felker <dalias@libc.org> wrote:

> On Wed, Jul 10, 2019 at 11:23:19PM +0200, Szabolcs Nagy wrote:
> > * James Y Knight <jyknight@google.com> [2019-07-10 16:11:23 -0400]:
> > >  int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
> > >  {
> > > +   // We must check requirements which Linux fails to verify in the
> syscall
> > > +   // itself.
> > >     if (ss) {
> > > -           if (ss->ss_size < MINSIGSTKSZ) {
> > > +           // The syscall does already check against MINSIGSTKSZ,
> however,
> > > +           // the kernel's value is smaller than musl's value on some
> > > +           // architectures. Thus, although this check may appear
> > > +           // redundant, it is not.
> >
> > the comment does not make sense to me, the check is obviously
> > not redundant


It wasn't obvious to me. Before I sent the first patch, I looked into why
this check was there, and did not find the reason. Only after further
investigation did I discover why it was not redundant. It seemed like it
may not have been obvious to Rich, either (Or rather, I guess it was
obvious to him that the check was surely needed for -some- reason, yet, not
why it was needed.)


> Yes. Also, in musl, we generally document motivations like this as

part of commit messages rather than comments. This ties them to the
> timeline of changes, to the author, and prevents them from sticking
> around when code changes and they no longer make sense.


I'd say that the commit message should document the motivation for why a
particular change was made, but that the code comments should document the
motivation for why the code is as it currently is.

James, could you submit this patch just as the minimal change to

correct the current bug? If additional documentation of why things are
> the way they are is needed that can be done separately.


Nevertheless -- done, and attached the one-line change. :)


> > MINSIGSTKSZ is a libc api, has nothing to do with the kernel
> >
> > the kernel also defines a MINSIGSZTKSZ but musl is an
> > abstraction layer higher, the linux limit should not be
> > observable to users, only the limit defined by musl,
> > which ensures not only that the kernel can deliver a
> > signal but also reserves space of any current or future
> > hackery the c runtime may need to do around signal handling,
> > so that trivial c language signal handler is guaranteed
> > to work.
> >
> > this is the only reasonable way to make such limit useful.
> > if it were only a kernel limit, then application code would
> > have to guess the libc signal handling overhead and add that
> > to the MINSIGSZTKSZ when allocating signal stacks.
>
> In this case it's more that the kernel values are just wrong. libc
> isn't imposing a stronger limit here because of libc code needing
> stack, but because the kernel values don't account for signal frame
> size. The kernel values presumably can't be changed because the
> syscall interface is stable/locked, and it's risky to change for libc
> too after it's set (see the issue with whether the x86 values are
> right in the presence of AVX512 -- that's why on later archs we
> imposed stronger limits).
>
>
Yea, it looks to me from kernel commit messages that the kernel did intend
MINSIGSTKSZ to be high enough for the kernel data itself, and for libc, and
for user-code to be able to make at least one reasonably-sized user stack
frame.

It seems like it might be almost a lost-cause to try to guarantee that any
particular static minimum value will work, since the amount of CPU state
data can now vary dramatically depending on whether vector extensions are
used. And with the AT_MINSIGSTKSZ auxv value now communicating a
dynamically-computed number from the kernel at program startup, perhaps
MINSIGSTKSZ should be treated more as a historical curiosity than an
actually useful number. But this is now getting into a whole other issue...

[-- Attachment #1.2: Type: text/html, Size: 5468 bytes --]

[-- Attachment #2: 0001-Fix-the-use-of-sigaltstack-to-return-to-the-saved-ma.patch --]
[-- Type: text/x-patch, Size: 1144 bytes --]

From 716ab22ae9613a65bf5b4df73474fa2ffc748995 Mon Sep 17 00:00:00 2001
From: James Y Knight <jyknight@google.com>
Date: Thu, 11 Jul 2019 11:48:08 -0400
Subject: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

Previously, musl would reject the call with -ENOMEM, because the main
stack typically has ss_size == 0 and ss_flags == SS_DISABLE.

Note -- it may seem that the check against MINSIGSTKSZ is redundant,
as Linux also checks against MINSIGSTKSZ within the syscall. However,
that is not the case, because on some platforms, Musl has set
different (larger) values for MINSIGSTKSZ than the kernel.
---
 src/signal/sigaltstack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/signal/sigaltstack.c b/src/signal/sigaltstack.c
index cfa3f5c1..d3a6e821 100644
--- a/src/signal/sigaltstack.c
+++ b/src/signal/sigaltstack.c
@@ -5,7 +5,7 @@
 int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
 {
 	if (ss) {
-		if (ss->ss_size < MINSIGSTKSZ) {
+		if (!(ss->ss_flags & SS_DISABLE) && ss->ss_size < MINSIGSTKSZ) {
 			errno = ENOMEM;
 			return -1;
 		}
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

[Florian Weimer]

* Szabolcs Nagy:

> the comment does not make sense to me, the check is obviously
> not redundant.
>
> MINSIGSTKSZ is a libc api, has nothing to do with the kernel
>
> the kernel also defines a MINSIGSZTKSZ but musl is an
> abstraction layer higher, the linux limit should not be
> observable to users, only the limit defined by musl,
> which ensures not only that the kernel can deliver a
> signal but also reserves space of any current or future
> hackery the c runtime may need to do around signal handling,
> so that trivial c language signal handler is guaranteed
> to work.

Please keep in mind that the kernel stack requirements for delivering a
signal vary and tend to increase over time, with newer CPU generations
with larger register files.  It leads to bugs to pretend this value is
constant.

Thanks,
Florian

Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack.

[Rich Felker]

On Fri, Jul 12, 2019 at 11:18:49AM +0200, Florian Weimer wrote:
> * Szabolcs Nagy:
> 
> > the comment does not make sense to me, the check is obviously
> > not redundant.
> >
> > MINSIGSTKSZ is a libc api, has nothing to do with the kernel
> >
> > the kernel also defines a MINSIGSZTKSZ but musl is an
> > abstraction layer higher, the linux limit should not be
> > observable to users, only the limit defined by musl,
> > which ensures not only that the kernel can deliver a
> > signal but also reserves space of any current or future
> > hackery the c runtime may need to do around signal handling,
> > so that trivial c language signal handler is guaranteed
> > to work.
> 
> Please keep in mind that the kernel stack requirements for delivering a
> signal vary and tend to increase over time, with newer CPU generations
> with larger register files.  It leads to bugs to pretend this value is
> constant.

If that's really going to be the position, then we need to standardize
removal of the macro as mandatory, and replacement by a sysconf. I
think that's a mistake though. Unbounded increase in register file
size destroys the potential for tolerable task-switch performance,
even for tasks in the same process that should switch very fast. My
(probably unpopular) opinion is that kernel just should not support
use of ridiculous huge register file additions like AVX512, or should
do so only for processes tagged as compatible with it (which would be
built with an ABI having a larger value of the constant).

Note that some archs like aarch64 explicitly reserved an upper bound
for future expansion, which is IMO already way too large, but at least
it's a bound.

Rich