Date: Sun, 29 Oct 2017 20:40:46 +0100 From: Andreas Tobler <andreast-list@fgznet.ch> To: Konstantin Belousov <kostikbel@gmail.com>, Tijl Coosemans <tijl@FreeBSD.org> Cc: freebsd-current@FreeBSD.org, gerald@FreeBSD.org Subject: Re: Segfault in _Unwind_* code called from pthread_exit Message-ID: <9a724da4-70f1-4330-9a77-619739008a14@fgznet.ch> In-Reply-To: <20171029191358.GU2566@kib.kiev.ua> References: <20170823163707.096f93ab@kalimero.tijl.coosemans.org> <20170824154235.GD1700@kib.kiev.ua> <20170824180830.199885b0@kalimero.tijl.coosemans.org> <20170825173851.09116ddc@kalimero.tijl.coosemans.org> <20170825234442.GO1700@kib.kiev.ua> <20170826202813.1240a1ef@kalimero.tijl.coosemans.org> <20170826184034.GR1700@kib.kiev.ua> <20171029182351.502f53cf@kalimero.tijl.coosemans.org> <20171029191358.GU2566@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------CE61152F0D9894BFC6C1A38A
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
On 29.10.17 20:13, Konstantin Belousov wrote:
> On Sun, Oct 29, 2017 at 06:23:51PM +0100, Tijl Coosemans wrote:
>> On Sat, 26 Aug 2017 21:40:34 +0300 Konstantin Belousov <kostikbel@gmail.com> wrote:
>>> On Sat, Aug 26, 2017 at 08:28:13PM +0200, Tijl Coosemans wrote:
>>>> I did consider using
>>>> a CFI directive (see patch below) and it works, but it's architecture
>>>> specific and it's inserted after the function prologue so there's still
>>>> a window of a few instructions where a stack unwinder will try to use
>>>> the return address.
>>>>
>>>> Index: lib/libthr/thread/thr_create.c
>>>> ===================================================================
>>>> --- lib/libthr/thread/thr_create.c (revision 322802)
>>>> +++ lib/libthr/thread/thr_create.c (working copy)
>>>> @@ -251,6 +251,7 @@ create_stack(struct pthread_attr *pattr)
>>>> static void
>>>> thread_start(struct pthread *curthread)
>>>> {
>>>> + __asm(".cfi_undefined %rip");
>>>> sigset_t set;
>>>>
>>>> if (curthread->attr.suspend == THR_CREATE_SUSPENDED)
>>>
>>> I like this approach much more than the previous patch. What can be
>>> done is to provide asm trampoline which calls thread_start(). There you
>>> can add the .cfi_undefined right at the entry.
>>>
>>> It is somewhat more work than just setting the return address on the
>>> kernel-constructed pseudo stack frame, but I believe this is ultimately
>>> correct way. You still can do it only on some arches, if you do not
>>> have incentive to code asm for all of them.
>>
>> Ok, but then there are two ways to implement the trampoline:
>>
>> 1)
>> movq $0,(%rsp)
>> jmp thread_start
>> 2)
>> subq $8,%rsp
>> call thread_start
>> /* NOTREACHED */
>>
>> With 1) you're setting the return address to zero anyway, so you might
>> as well do that in the kernel like my first patch. With 2) you're
>> setting up a new call frame, basically redoing what the kernel already
>> did and on i386 this also means copying the function argument.
> I do not quite understand the second variant, because the stack is not
> guaranteed to be zeroed, and it is often not if reused after the previously
> exited thread.
>
> The first variant is what I like, but perhaps we need to emulate the
> frame as well, i.e. push two zero longs.
>
> Currently kernel does not access the usermode stack for the new thread
> unless dictated by ABI (i.e. it does not touch it for 64bit process
> on amd64, but have to for 32bit). I like this property. Also, the
> previous paragraph is indicative: we do not really know in kernel
> what ABI the userspace follows. It might want frame, may be it does
> not need it. It could use other register than %rbp as the frame base,
> etc.
>
>>
>> Do you have any preference (or better alternatives), because I think I
>> still prefer my first patch. It's the caller's job to set up the call
>> frame, in this case the kernel. And if the kernel handles it then it
>> also works with (hypothetical) implementations other than libthr.
>>
>>> Also crt1 probably should get the same treatment, despite we already set
>>> %rbp to zero AFAIR.
>>
>> I haven't checked but I imagine the return address of the process entry
>> point is always zero because the stack is all zeros.
> Stack is not zero. The environment and argument strings and auxv are copied
> at top, and at the bottom the ps_strings structure is located, so it
> is not.
>
> If you commit your existing patch as is, I will not resent. But I do think
> that stuff that can be done in usermode, should be done in usermode, esp.
> when the amount of efforts is same.
Attached what I have for libgcc. It can be applied to gcc5-8, should
give no issues. The mentioned tc from this thread and mine,
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82635 do pass.
What do you think?
Andreas
--------------CE61152F0D9894BFC6C1A38A
Content-Type: text/plain; charset=UTF-8; x-mac-type="0"; x-mac-creator="0";
name="libgcc-fbsd-unwind.diff"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
filename="libgcc-fbsd-unwind.diff"
SW5kZXg6IGxpYmdjYy9jb25maWcvaTM4Ni9mcmVlYnNkLXVud2luZC5oCj09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT0KLS0tIGxpYmdjYy9jb25maWcvaTM4Ni9mcmVlYnNkLXVud2luZC5oCShyZXZpc2lvbiAy
NTQyMDUpCisrKyBsaWJnY2MvY29uZmlnL2kzODYvZnJlZWJzZC11bndpbmQuaAkod29ya2lu
ZyBjb3B5KQpAQCAtMjgsNyArMjgsMTAgQEAKIAogI2luY2x1ZGUgPHN5cy90eXBlcy5oPgog
I2luY2x1ZGUgPHNpZ25hbC5oPgorI2luY2x1ZGUgPHVuaXN0ZC5oPgorI2luY2x1ZGUgPHN5
cy9zeXNjdGwuaD4KICNpbmNsdWRlIDxzeXMvdWNvbnRleHQuaD4KKyNpbmNsdWRlIDxzeXMv
dXNlci5oPgogI2luY2x1ZGUgPG1hY2hpbmUvc2lnZnJhbWUuaD4KIAogI2RlZmluZSBSRUdf
TkFNRShyZWcpCXNmX3VjLnVjX21jb250ZXh0Lm1jXyMjIHJlZwpAQCAtMzYsMzggKzM5LDQ2
IEBACiAjaWZkZWYgX194ODZfNjRfXwogI2RlZmluZSBNRF9GQUxMQkFDS19GUkFNRV9TVEFU
RV9GT1IgeDg2XzY0X2ZyZWVic2RfZmFsbGJhY2tfZnJhbWVfc3RhdGUKIAorc3RhdGljIGlu
dAoreDg2XzY0X291dHNpZGVfc2lndHJhbXBfcmFuZ2UgKHVuc2lnbmVkIGNoYXIgKnBjKQor
eworICBzdGF0aWMgaW50IHNpZ3RyYW1wX3JhbmdlX2RldGVybWluZWQgPSAwOworICBzdGF0
aWMgdW5zaWduZWQgY2hhciAqc2lndHJhbXBfc3RhcnQsICpzaWd0cmFtcF9lbmQ7CisKKyAg
aWYgKHNpZ3RyYW1wX3JhbmdlX2RldGVybWluZWQgPT0gMCkKKyAgICB7CisgICAgICBzdHJ1
Y3Qga2luZm9fc2lndHJhbXAga3N0ID0gezB9OworICAgICAgc2l6ZV90IGxlbiA9IHNpemVv
ZiAoa3N0KTsKKyAgICAgIGludCBtaWJbNF0gPSB7IENUTF9LRVJOLCBLRVJOX1BST0MsIEtF
Uk5fUFJPQ19TSUdUUkFNUCwgZ2V0cGlkKCkgfTsKKworICAgICAgc2lndHJhbXBfcmFuZ2Vf
ZGV0ZXJtaW5lZCA9IDE7CisgICAgICBpZiAoc3lzY3RsIChtaWIsIDQsICZrc3QsICZsZW4s
IE5VTEwsIDApID09IDApCisgICAgICB7CisgICAgICAgIHNpZ3RyYW1wX3JhbmdlX2RldGVy
bWluZWQgPSAyOworICAgICAgICBzaWd0cmFtcF9zdGFydCA9IGtzdC5rc2lndHJhbXBfc3Rh
cnQ7CisgICAgICAgIHNpZ3RyYW1wX2VuZCAgID0ga3N0LmtzaWd0cmFtcF9lbmQ7CisgICAg
ICB9CisgICAgfQorICBpZiAoc2lndHJhbXBfcmFuZ2VfZGV0ZXJtaW5lZCA8IDIpICAvKiBz
eXNjdGwgZmFpbGVkIGlmIDwgMiAqLworICAgIHJldHVybiAxOworCisgIHJldHVybiAocGMg
PCBzaWd0cmFtcF9zdGFydCB8fCBwYyA+PSBzaWd0cmFtcF9lbmQpOworfQorCiBzdGF0aWMg
X1Vud2luZF9SZWFzb25fQ29kZQogeDg2XzY0X2ZyZWVic2RfZmFsbGJhY2tfZnJhbWVfc3Rh
dGUKIChzdHJ1Y3QgX1Vud2luZF9Db250ZXh0ICpjb250ZXh0LCBfVW53aW5kX0ZyYW1lU3Rh
dGUgKmZzKQogewogICBzdHJ1Y3Qgc2lnZnJhbWUgKnNmOwotICBsb25nIG5ld19jZmE7Cisg
IF9VbndpbmRfUHRyIG5ld19jZmE7CiAKLSAgLyogUHJpb3IgdG8gRnJlZUJTRCA5LCB0aGUg
c2lnbmFsIHRyYW1wb2xpbmUgd2FzIGxvY2F0ZWQgaW1tZWRpYXRlbHkKLSAgICAgYmVmb3Jl
IHRoZSBwc19zdHJpbmdzLiAgVG8gc3VwcG9ydCBub24tZXhlY3V0YWJsZSBzdGFja3Mgb24g
QU1ENjQsCi0gICAgIHRoZSBzaWd0cmFtcCB3YXMgbW92ZWQgdG8gYSBzaGFyZWQgcGFnZSBm
b3IgRnJlZUJTRCA5LiAgVW5mb3J0dW5hdGVseQotICAgICB0aGlzIG1lYW5zIGxvb2tpbmcg
ZnJhbWUgcGF0dGVybnMgYWdhaW4gKHN5cy9hbWQ2NC9hbWQ2NC9zaWd0cmFtcC5TKQotICAg
ICByYXRoZXIgdGhhbiB1c2luZyB0aGUgcm9idXN0IGFuZCBjb252ZW5pZW50IEtFUk5fUFNf
U1RSSU5HUyB0cmljay4KLQotICAgICA8cGMgKyAwMD46ICBsZWEgICAgIDB4MTAoJXJzcCks
JXJkaQotICAgICA8cGMgKyAwNT46ICBwdXNocSAgICQweDAKLSAgICAgPHBjICsgMTc+OiAg
bW92ICAgICAkMHgxYTEsJXJheAotICAgICA8cGMgKyAxND46ICBzeXNjYWxsCi0KLSAgICAg
SWYgd2UgY2FuJ3QgZmluZCB0aGlzIHBhdHRlcm4sIHdlJ3JlIGF0IHRoZSBlbmQgb2YgdGhl
IHN0YWNrLgotICAqLwotCi0gIGlmICghKCAgICoodW5zaWduZWQgaW50ICopKGNvbnRleHQt
PnJhKSAgICAgID09IDB4MjQ3YzhkNDgKLSAgICAgICAgJiYgKih1bnNpZ25lZCBpbnQgKiko
Y29udGV4dC0+cmEgKyAgNCkgPT0gMHg0ODAwNmExMAotICAgICAgICAmJiAqKHVuc2lnbmVk
IGludCAqKShjb250ZXh0LT5yYSArICA4KSA9PSAweDAxYTFjMGM3Ci0gICAgICAgICYmICoo
dW5zaWduZWQgaW50ICopKGNvbnRleHQtPnJhICsgMTIpID09IDB4MDUwZjAwMDAgKSkKKyAg
aWYgKHg4Nl82NF9vdXRzaWRlX3NpZ3RyYW1wX3JhbmdlKGNvbnRleHQtPnJhKSkKICAgICBy
ZXR1cm4gX1VSQ19FTkRfT0ZfU1RBQ0s7CiAKICAgc2YgPSAoc3RydWN0IHNpZ2ZyYW1lICop
IGNvbnRleHQtPmNmYTsKICAgbmV3X2NmYSA9IHNmLT5SRUdfTkFNRShyc3ApOwogICBmcy0+
cmVncy5jZmFfaG93ID0gQ0ZBX1JFR19PRkZTRVQ7Ci0gIC8qIFJlZ2lzdGVyIDcgaXMgcnNw
ICAqLwotICBmcy0+cmVncy5jZmFfcmVnID0gNzsKKyAgZnMtPnJlZ3MuY2ZhX3JlZyA9ICBf
X0xJQkdDQ19TVEFDS19QT0lOVEVSX1JFR05VTV9fOwogICBmcy0+cmVncy5jZmFfb2Zmc2V0
ID0gbmV3X2NmYSAtIChsb25nKSBjb250ZXh0LT5jZmE7CiAKICAgLyogVGhlIFNWUjQgcmVn
aXN0ZXIgbnVtYmVyaW5nIG1hY3JvcyBhcmVuJ3QgdXNhYmxlIGluIGxpYmdjYy4gICovCg==
--------------CE61152F0D9894BFC6C1A38A--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?9a724da4-70f1-4330-9a77-619739008a14>