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

[James Y Knight <jyknight@google.com>]

[-- 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