From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14395 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: James Y Knight Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack. Date: Thu, 11 Jul 2019 11:51:07 -0400 Message-ID: References: <20190709193004.GQ1506@brightrain.aerifal.cx> <20190710183931.GT1506@brightrain.aerifal.cx> <20190710212319.GM21055@port70.net> <20190710214807.GY1506@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="000000000000a98b7b058d69c489" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="89885"; mail-complaints-to="usenet@blaine.gmane.org" To: musl@lists.openwall.com Original-X-From: musl-return-14411-gllmg-musl=m.gmane.org@lists.openwall.com Thu Jul 11 17:51:51 2019 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.89) (envelope-from ) id 1hlbMM-000NH5-Pn for gllmg-musl@m.gmane.org; Thu, 11 Jul 2019 17:51:50 +0200 Original-Received: (qmail 28358 invoked by uid 550); 11 Jul 2019 15:51:47 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 28340 invoked from network); 11 Jul 2019 15:51:46 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=9pVm/Hq2Eod+vhGeqPGzNeDBG6C0eAwiBbBePUVjvxc=; b=fnNIYMDJh/6YCgDUHTvjwIWlc34HGqLv32pEr6D4R1Edz8sNp1RQwVyEeRvkFyiFxP 4n8Q0u4F6nLTd8ksNN6f5uLjvWSzLotImiSznmGM2QgnLTYjIzzm9tRwFtU7VBZfTheD rRKrMzE1oGo0uG92I7NITLaL9cNBVtoVaPRE1OiqR1HchkfYVsWR9Q8AlOcxKr3cuSRl 0ddfLo7GTD1XRyHFYs8YV0l3zafwCBMrGKTl9hw0Qxh3C3ssgcHF7h9EHxUaro4Otz5m w/CRtt1GHg1Xqv3iyZGHCK9l3KpDFZcRhdB89f6mU3pcOfkU2NBSYGc/JxtZDrgBHUnp WSoQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=9pVm/Hq2Eod+vhGeqPGzNeDBG6C0eAwiBbBePUVjvxc=; b=G3T9543w1mdKxcv7i32zfqLhk5Nt++d74EtoSbvwtiABl/MaZyJx4sZYeIKnFVBxyX p8nX6vckXEklDX8HgQw3xFI02nLn58o3+c7K6cqt0hjl9e29zjPQhRXlx8UcRNIcMEfr SqRwmFyR4G6PFYYuyjIPO2BYlmmIJcDYM0qnikwGuG11eALCX2iIFMW1WdNUBBSRATuW WQHKSSDvr8lJ8wvlqeU7HSyYdm7EeuAbSBDXdPqr6EfY0Un3rLS1JGo3tEVZ/E3hx6FY Zo22y7FPis7B13bPgETBz/DqrMr0mLo2QN2uOdCiNN9VVhm6RPif43k0d/JKJKPZGWWi ytdA== X-Gm-Message-State: APjAAAUEPPazzDuyA59k2BIg3fqycKNaagYqKpQHSbPc3TEs+zQb/kGl rUr2mQMITwRdrI+NhjXsa28Ib2v+OBAtHkDd+IdtZlDEBbo= X-Google-Smtp-Source: APXvYqx6d79LPxSC0UKWShwOYg9obftYDvlethFVS561XM+jhiSO+Y0K9zSlC6taniQEoU/0OrC7nD8GIcp5fpwD6PM= X-Received: by 2002:ab0:699a:: with SMTP id t26mr4435364uaq.70.1562860294030; Thu, 11 Jul 2019 08:51:34 -0700 (PDT) In-Reply-To: <20190710214807.GY1506@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:14395 Archived-At: --000000000000a98b7b058d69c489 Content-Type: multipart/alternative; boundary="000000000000a98b77058d69c487" --000000000000a98b77058d69c487 Content-Type: text/plain; charset="UTF-8" On Wed, Jul 10, 2019 at 5:48 PM Rich Felker wrote: > On Wed, Jul 10, 2019 at 11:23:19PM +0200, Szabolcs Nagy wrote: > > * James Y Knight [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... --000000000000a98b77058d69c487 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
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]:
> >=C2=A0 int sigaltstack(const stack_t *restrict ss, stack_t *restri= ct old)
> >=C2=A0 {
> > +=C2=A0 =C2=A0// We must check requirements which Linux fails to = verify in the syscall
> > +=C2=A0 =C2=A0// itself.
> >=C2=A0 =C2=A0 =C2=A0if (ss) {
> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ss->ss_size <= MINSIGSTKSZ) {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0// The syscall does alr= eady check against MINSIGSTKSZ, however,
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0// the kernel's val= ue is smaller than musl's value on some
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0// architectures. Thus,= although this check may appear
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0// 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 discov= er 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.)
=C2=A0
Yes. = Also, in musl, we generally document motivations like this as
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex"> 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 motiva= tion for why a particular change was made, but that the code comments shoul= d document the motivation for why the code is as it currently is.

James, could y= ou 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. :)
=C2=A0
> 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 l= east one reasonably-sized user stack frame.

It see= ms like it might be almost a lost-cause to try to guarantee that any partic= ular 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-compute= d 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...
--000000000000a98b77058d69c487-- --000000000000a98b7b058d69c489 Content-Type: text/x-patch; charset="US-ASCII"; name="0001-Fix-the-use-of-sigaltstack-to-return-to-the-saved-ma.patch" Content-Disposition: attachment; filename="0001-Fix-the-use-of-sigaltstack-to-return-to-the-saved-ma.patch" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_jxyuk28w0 RnJvbSA3MTZhYjIyYWU5NjEzYTY1YmY1YjRkZjczNDc0ZmEyZmZjNzQ4OTk1IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBKYW1lcyBZIEtuaWdodCA8anlrbmlnaHRAZ29vZ2xlLmNvbT4K RGF0ZTogVGh1LCAxMSBKdWwgMjAxOSAxMTo0ODowOCAtMDQwMApTdWJqZWN0OiBbUEFUQ0hdIEZp eCB0aGUgdXNlIG9mIHNpZ2FsdHN0YWNrIHRvIHJldHVybiB0byB0aGUgc2F2ZWQgbWFpbiBzdGFj ay4KClByZXZpb3VzbHksIG11c2wgd291bGQgcmVqZWN0IHRoZSBjYWxsIHdpdGggLUVOT01FTSwg YmVjYXVzZSB0aGUgbWFpbgpzdGFjayB0eXBpY2FsbHkgaGFzIHNzX3NpemUgPT0gMCBhbmQgc3Nf ZmxhZ3MgPT0gU1NfRElTQUJMRS4KCk5vdGUgLS0gaXQgbWF5IHNlZW0gdGhhdCB0aGUgY2hlY2sg YWdhaW5zdCBNSU5TSUdTVEtTWiBpcyByZWR1bmRhbnQsCmFzIExpbnV4IGFsc28gY2hlY2tzIGFn YWluc3QgTUlOU0lHU1RLU1ogd2l0aGluIHRoZSBzeXNjYWxsLiBIb3dldmVyLAp0aGF0IGlzIG5v dCB0aGUgY2FzZSwgYmVjYXVzZSBvbiBzb21lIHBsYXRmb3JtcywgTXVzbCBoYXMgc2V0CmRpZmZl cmVudCAobGFyZ2VyKSB2YWx1ZXMgZm9yIE1JTlNJR1NUS1NaIHRoYW4gdGhlIGtlcm5lbC4KLS0t CiBzcmMvc2lnbmFsL3NpZ2FsdHN0YWNrLmMgfCAyICstCiAxIGZpbGUgY2hhbmdlZCwgMSBpbnNl cnRpb24oKyksIDEgZGVsZXRpb24oLSkKCmRpZmYgLS1naXQgYS9zcmMvc2lnbmFsL3NpZ2FsdHN0 YWNrLmMgYi9zcmMvc2lnbmFsL3NpZ2FsdHN0YWNrLmMKaW5kZXggY2ZhM2Y1YzEuLmQzYTZlODIx IDEwMDY0NAotLS0gYS9zcmMvc2lnbmFsL3NpZ2FsdHN0YWNrLmMKKysrIGIvc3JjL3NpZ25hbC9z aWdhbHRzdGFjay5jCkBAIC01LDcgKzUsNyBAQAogaW50IHNpZ2FsdHN0YWNrKGNvbnN0IHN0YWNr X3QgKnJlc3RyaWN0IHNzLCBzdGFja190ICpyZXN0cmljdCBvbGQpCiB7CiAJaWYgKHNzKSB7Ci0J CWlmIChzcy0+c3Nfc2l6ZSA8IE1JTlNJR1NUS1NaKSB7CisJCWlmICghKHNzLT5zc19mbGFncyAm IFNTX0RJU0FCTEUpICYmIHNzLT5zc19zaXplIDwgTUlOU0lHU1RLU1opIHsKIAkJCWVycm5vID0g RU5PTUVNOwogCQkJcmV0dXJuIC0xOwogCQl9Ci0tIAoyLjIyLjAuNDEwLmdkOGZkYmUyMWI1LWdv b2cKCg== --000000000000a98b7b058d69c489--