* [PATCH] Fix the use of sigaltstack to return to the saved main stack. @ 2019-07-09 19:01 James Y Knight 2019-07-09 19:30 ` Rich Felker 0 siblings, 1 reply; 10+ messages in thread From: James Y Knight @ 2019-07-09 19:01 UTC (permalink / raw) To: musl [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack. 2019-07-09 19:01 [PATCH] Fix the use of sigaltstack to return to the saved main stack James Y Knight @ 2019-07-09 19:30 ` Rich Felker 2019-07-10 18:04 ` James Y Knight 0 siblings, 1 reply; 10+ messages in thread From: Rich Felker @ 2019-07-09 19:30 UTC (permalink / raw) To: musl 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack. 2019-07-09 19:30 ` Rich Felker @ 2019-07-10 18:04 ` James Y Knight 2019-07-10 18:39 ` Rich Felker 0 siblings, 1 reply; 10+ messages in thread From: James Y Knight @ 2019-07-10 18:04 UTC (permalink / raw) To: musl [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack. 2019-07-10 18:04 ` James Y Knight @ 2019-07-10 18:39 ` Rich Felker 2019-07-10 20:11 ` James Y Knight 0 siblings, 1 reply; 10+ messages in thread From: Rich Felker @ 2019-07-10 18:39 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack. 2019-07-10 18:39 ` Rich Felker @ 2019-07-10 20:11 ` James Y Knight 2019-07-10 21:23 ` Szabolcs Nagy 0 siblings, 1 reply; 10+ messages in thread From: James Y Knight @ 2019-07-10 20:11 UTC (permalink / raw) To: musl [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack. 2019-07-10 20:11 ` James Y Knight @ 2019-07-10 21:23 ` Szabolcs Nagy 2019-07-10 21:48 ` Rich Felker 2019-07-12 9:18 ` Florian Weimer 0 siblings, 2 replies; 10+ messages in thread From: Szabolcs Nagy @ 2019-07-10 21:23 UTC (permalink / raw) To: musl * 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack. 2019-07-10 21:23 ` Szabolcs Nagy @ 2019-07-10 21:48 ` Rich Felker 2019-07-11 15:51 ` James Y Knight 2019-07-12 9:18 ` Florian Weimer 1 sibling, 1 reply; 10+ messages in thread From: Rich Felker @ 2019-07-10 21:48 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack. 2019-07-10 21:48 ` Rich Felker @ 2019-07-11 15:51 ` James Y Knight 0 siblings, 0 replies; 10+ messages in thread From: James Y Knight @ 2019-07-11 15:51 UTC (permalink / raw) To: musl [-- 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack. 2019-07-10 21:23 ` Szabolcs Nagy 2019-07-10 21:48 ` Rich Felker @ 2019-07-12 9:18 ` Florian Weimer 2019-07-12 16:06 ` Rich Felker 1 sibling, 1 reply; 10+ messages in thread From: Florian Weimer @ 2019-07-12 9:18 UTC (permalink / raw) To: musl * 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack. 2019-07-12 9:18 ` Florian Weimer @ 2019-07-12 16:06 ` Rich Felker 0 siblings, 0 replies; 10+ messages in thread From: Rich Felker @ 2019-07-12 16:06 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-07-12 16:06 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-09 19:01 [PATCH] Fix the use of sigaltstack to return to the saved main stack James Y Knight 2019-07-09 19:30 ` Rich Felker 2019-07-10 18:04 ` James Y Knight 2019-07-10 18:39 ` Rich Felker 2019-07-10 20:11 ` James Y Knight 2019-07-10 21:23 ` Szabolcs Nagy 2019-07-10 21:48 ` Rich Felker 2019-07-11 15:51 ` James Y Knight 2019-07-12 9:18 ` Florian Weimer 2019-07-12 16:06 ` Rich Felker
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).