Fix phdr_tls_data for static binaries; Fix recursion on rtld_bind_lock in dynamic dl_iterate_phdr
ClosedPublic
Actions

Authored by kib on Apr 7 2021, 8:23 AM.

Details

Reviewers

emaste
mhorne
andrew
jkim
jhibbits
bdragon

Commits

rGe8b9c508b7ae: rtld: use _get_tp() in __tls_get_addr()
rG7cb32a0d0343: rtld: avoid recursing on rtld_bind_lock for write
rG895080484248: rtld: allow to use tls_get_addr_slow() from context where rtld_bind_lock is…
rG85d846b369f4: rtld: style tls_get_addr_slow
rGdbd2053026a6: libc dl_iterate_phdr(): dlpi_tls_data is wrong
rGca46b5698e8a: libc: implement __tls_get_addr() for static binaries
rG06d8a116bd6b: libc: add _get_tp() private function
rG99c2ce7ef12f: rtld: define TLS_DTV_OFFSET on all architectures
rG7f7489eba391: libc: include rtld.h into static implementations of rtld interface
rG93c14c55ec0d: libc: constify dummy error message string for dlfcn
rG34ca6025ddfe: rtld_lock.h: Expand scope for IN_RTLD to avoid some conflicts with libc
rGf61ecf60cfce: rtld/x86/reloc.c: style
rG38e0610df7d5: rtld_lock.h: remove tautological extern's

Summary

This change make phdr_tls_data to be the address of the TLS segment for current thread, both for static and dynamic binaries.

Also it adds _get_tp() libc function that returns TCB. Its existence makes many things in TLS handling easier (but I did not converted all __tls_get_address() to use _get_tp()).

Per-commit split of the diff can be seen at https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/phdr_tls_data

I would appreciate both review and even more, test of the patch on non-x86 architectures. In fact, even checking that TLS still works is great. Since there is a regression in dl_iterate_phdr() fixed by this change, I want to commit this ASAP (and do not want to revert the buggy commit due to versioning changes included)

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kib requested review of this revision.Apr 7 2021, 8:23 AM

kib created this revision.

andrew added inline comments.Apr 7 2021, 10:35 AM

lib/libc/arm/gen/_get_tp.c
45–47 ↗	(On Diff #86947)	How far back are you planning to MFC this? This is needed in 12, but not 13 or later. I've created D29624 to remove it from HEAD.

kib marked an inline comment as done.Apr 7 2021, 12:19 PM

kib added inline comments.

lib/libc/arm/gen/_get_tp.c
45–47 ↗	(On Diff #86947)	13 definitely. So far rtld code was synced between 12/13/HEAD, so I will do put some efforts to MFC to 12. I do not think that removal of ARM_TP_ADDRESS would be hard to handle, although I cannot test the merge result. I mean, it might be slightly simpler if removal was done after my changes go in.

mhorne added inline comments.Apr 7 2021, 4:02 PM

lib/libc/gen/tls.c
44 ↗	(On Diff #86947)	This results in a redefinition error for `TLS_TCB_SIZE` since it is defined in `rtld_machdep.h` for some archs, and then again in this file.
116 ↗	(On Diff #86947)	Can you use `DTV_OFFSET` defined above? I could be wrong, but it seems like doing so would be enough to avoid the `rtld.h` include.

The version of the patch survived tinderbox

Testing libexec/rtld-elf, lib/csu, lib/libthr on powerpc*.

Looks good on powerpc/powerpc and powerpc/powerpcspe (32 bit). Will continue testing other powerpc variants.

By the way, doesn't the new _get_tp need to be exposed in the FBSDprivate_1.0 namespace in the Symbol.map files like _set_tp is? (Especially on mips, where it can be a sysarch still?)

In D29623#664356, @bdragon wrote:

Testing libexec/rtld-elf, lib/csu, lib/libthr on powerpc*.

Looks good on powerpc/powerpc and powerpc/powerpcspe (32 bit). Will continue testing other powerpc variants.

Thank you for the testing.

By the way, doesn't the new _get_tp need to be exposed in the FBSDprivate_1.0 namespace in the Symbol.map files like _set_tp is? (Especially on mips, where it can be a sysarch still?)

So far consumers of _get_tp() are either libc itself, or rtld which links to the special static version of libc. In other words, there is no in-tree consumer that would use the symbol in the libc.so.

Oh! So it's not a contract between libc and rtld, it's just internal functionality that is used in both static libc as well as rtld, so it ends up being hidden visibility and just a common piece of code that both happen to pull in. Makes sense to me now.

powerpc/powerpc64 tested. I also built powerpc/powerpc64le, but won't get a chance to test it tonight. I don't expect it to be any different than powerpc64 though, since powerpc64 and powerpc64le share code.

powerpcspe and powerpc64 also got through all of the lib/libc tests with no additional failures (on top of the handful of preexising failures that I haven't gotten around to sorting out yet that have been around for months)

No objections from a PowerPC standpoint.

This revision is now accepted and ready to land.Apr 8 2021, 3:17 AM

Rebase after removal of ARM_TP_ADDRESS

This revision now requires review to proceed.Apr 8 2021, 5:39 PM

arichardson added a subscriber: arichardson.Apr 8 2021, 6:23 PM

The latest version boots okay on riscv. I ran the libc and rtld test suites as well.

This revision is now accepted and ready to land.Apr 8 2021, 6:24 PM

Retested powerpc/powerpcspe. Still good.

madpilot added a subscriber: madpilot.Apr 8 2021, 7:14 PM

fluffy added a subscriber: fluffy.Apr 9 2021, 11:11 AM

pi added a subscriber: pi.Apr 9 2021, 11:18 AM

Closed by commit rG38e0610df7d5: rtld_lock.h: remove tautological extern's (authored by kib). · Explain WhyApr 9 2021, 8:47 PM