Bug 254774

Summary: [rtld] dl_iterate_phdr: dlpi_tls_data should be the iterated module's TLS image instead of TLS initialization image
Product: Base System Reporter: maskray <emacsray>
Component: miscAssignee: Konstantin Belousov <kib>
Status: Closed FIXED    
Severity: Affects Only Me CC: bdragon, kib, vangyzen
Priority: ---    
Version: Unspecified   
Hardware: Any   
OS: Any   
Bug Depends on:    
Bug Blocks: 254927    

Description maskray 2021-04-04 23:51:19 UTC
glibc d78efd9f369a8fc46229fc9224e10e3781eecc43 (2006-02) introduced dlpi_tls_modid and dlpi_tls_data.
dlpi_tls_data is the allocated TLS image associated to the module dlpi_tls_modid.

The Linux manpage documents the two members as:

               size_t dlpi_tls_modid;
                               /* If there is a PT_TLS segment, its module
                                  ID as used in TLS relocations, else zero */
               void  *dlpi_tls_data;
                               /* The address of the calling thread's instance
                                  of this module's PT_TLS segment, if it has
                                  one and it has been allocated in the calling
                                  thread, otherwise a null pointer */

In FreeBSD, dlpi_tls_data is however the TLS initialization image as part of the underlying file.


I plan to use dl_iterate_phdr in compiler-rt's sanitizer runtime to simplify TLS handling,
but unfortunately FreeBSD dlpi_tls_data does not have the desired semantics :(
Comment 1 maskray 2021-04-05 01:02:53 UTC
Note: dlpi_tls_data is essentially __tls_get_addr((tls_mod_off_t[]){1,0}) on both TLS Variant I and Variant II architectures.

musl has the same problem. Rich has acknoledged the problem and will fix it with:


diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index aaadcce0..b66ad537 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -2331,7 +2331,7 @@ int dl_iterate_phdr(int(*callback)(struct dl_phdr_info *info, size_t size, void
 		info.dlpi_adds      = gencnt;
 		info.dlpi_subs      = 0;
 		info.dlpi_tls_modid = current->tls_id;
-		info.dlpi_tls_data  = current->tls.image;
+		info.dlpi_tls_data = __tls_get_addr((tls_mod_off_t[]){current->tls_id,0});
 
 		ret = (callback)(&info, sizeof (info), data);
 
diff --git a/src/ldso/dl_iterate_phdr.c b/src/ldso/dl_iterate_phdr.c
index 86c87ef8..9546dd36 100644
--- a/src/ldso/dl_iterate_phdr.c
+++ b/src/ldso/dl_iterate_phdr.c
@@ -1,5 +1,6 @@
 #include <elf.h>
 #include <link.h>
+#include "pthread_impl.h"
 #include "libc.h"
 
 #define AUX_CNT 38
@@ -35,7 +36,7 @@ static int static_dl_iterate_phdr(int(*callback)(struct dl_phdr_info *info, size
 	info.dlpi_subs  = 0;
 	if (tls_phdr) {
 		info.dlpi_tls_modid = 1;
-		info.dlpi_tls_data = (void *)(base + tls_phdr->p_vaddr);
+		info.dlpi_tls_data = __tls_get_addr((tls_mod_off_t[]){1,0});
 	} else {
 		info.dlpi_tls_modid = 0;
 		info.dlpi_tls_data = 0;
Comment 2 Brandon Bergren freebsd_committer freebsd_triage 2021-04-05 01:30:39 UTC
It looks like on ours, computing the address with __tls_get_addr will as a side effect do any deferred allocations / dtv resizing due to the tls_dtv_generation check happening at that time.

Not sure if it's desired or not that these outstanding changes happen whenever doing dl_iterate_phdr, or if it's more useful to give some insight as to what is currently allocated.
Comment 3 Konstantin Belousov freebsd_committer freebsd_triage 2021-04-05 03:13:20 UTC
(In reply to Brandon Bergren from comment #2)
The drv reallocation should not affect dl_iterate_phdr() result.

That said, for dl_iterate_phdr() for dynamic binary, the fix is indeed simple:
diff --git a/libexec/rtld-elf/rtld.c b/libexec/rtld-elf/rtld.c
index 733c3c80b70f..0ded117b1019 100644
--- a/libexec/rtld-elf/rtld.c
+++ b/libexec/rtld-elf/rtld.c
@@ -3909,13 +3909,16 @@ dlinfo(void *handle, int request, void *p)
 static void
 rtld_fill_dl_phdr_info(const Obj_Entry *obj, struct dl_phdr_info *phdr_info)
 {
+	tls_index ti;
 
 	phdr_info->dlpi_addr = (Elf_Addr)obj->relocbase;
 	phdr_info->dlpi_name = obj->path;
 	phdr_info->dlpi_phdr = obj->phdr;
 	phdr_info->dlpi_phnum = obj->phsize / sizeof(obj->phdr[0]);
 	phdr_info->dlpi_tls_modid = obj->tlsindex;
-	phdr_info->dlpi_tls_data = obj->tlsinit;
+	ti.ti_module = obj->tlsindex;
+	ti.ti_offset = 0;
+	phdr_info->dlpi_tls_data = __tls_get_addr(&ti);
 	phdr_info->dlpi_adds = obj_loads;
 	phdr_info->dlpi_subs = obj_loads - obj_count;
 }

But for static binary where libc TLS implementation is used, we do not provide
working __tls_get_addr().  Do you need this for static binaries, or it would
be only the consistency that is hurt if libc implementation is provided later?
Comment 4 maskray 2021-04-05 03:36:07 UTC
I want to use dlpi_tls_data for static TLS blocks (from the main executable and initially loaded shared objects).
Working dynamic TLS blocks will be nice to have for consistency.

I will try calling __tls_get_addr({tls_modid,0}) as a workaround, but it probably forces allocation (at least in glibc; I guess FreeBSD rtld/libc may be similar) so slightly inferior (but not matters for most not-too-size-concerned applications).

If you are interested, there is a complex story behind "why does sanitizer runtime need to know static/dynamic TLS blocks".
In the end I think we may need libc APIs. Once I know the context sufficiently well, I shall start a discussion on https://www.openwall.com/lists/libc-coord

Currently I have written down some notes in https://maskray.me/blog/2021-02-14-all-about-thread-local-storage#why-does-compiler-rt-need-to-know-tls-blocks
Precise static TLS blocks tracking is more important than precise dynamic TLS blocks.
Comment 5 Konstantin Belousov freebsd_committer freebsd_triage 2021-04-05 03:45:53 UTC
(In reply to maskray from comment #4)
No, I do not mean initial exec vs dynamic tls.  I mean, dl_iterate_phdr()
dlpi_tls_data value in static _binary_ vs dynamic binary.

The patch I posted is for dynamic binaries case.  For static linking of
final executable, where ld -static is used, I did some WIP but more efforts
are required to finish fixing dlpi_tls_data.
Comment 6 maskray 2021-04-05 04:09:22 UTC
(In reply to Konstantin Belousov from comment #5)
Ah, nice, thanks. Fixing libc.so should suffice for sanitizers - sanitizers need dlsym(RTLD_NEXT, ...) and cannot be used with -static (libc.a).
Comment 7 commit-hook freebsd_committer freebsd_triage 2021-04-06 00:38:08 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=d36d6816151705907393889d661cbfd25c630ca8

commit d36d6816151705907393889d661cbfd25c630ca8
Author:     Konstantin Belousov <kib@FreeBSD.org>
AuthorDate: 2021-04-05 03:05:44 +0000
Commit:     Konstantin Belousov <kib@FreeBSD.org>
CommitDate: 2021-04-06 00:23:08 +0000

    rtld dl_iterate_phdr(): dlpi_tls_data is wrong

    dl_iterate_phdr() dlpi_tls_data should provide the TLS module segment
    address, and not the TLS init segment address as it does now.

    Reported by:    emacsray@gmail.com
    PR:     254774
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week

 lib/libc/gen/dl_iterate_phdr.3 | 7 +++++--
 libexec/rtld-elf/Symbol.map    | 1 +
 libexec/rtld-elf/rtld.1        | 7 +++++++
 libexec/rtld-elf/rtld.c        | 8 +++++++-
 sys/sys/param.h                | 2 +-
 5 files changed, 21 insertions(+), 4 deletions(-)
Comment 8 Eric van Gyzen freebsd_committer freebsd_triage 2021-04-09 19:36:35 UTC
This seems to have introduced bug 254927.
Comment 9 commit-hook freebsd_committer freebsd_triage 2021-04-09 20:48:09 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=dbd2053026a6af28adb1aa32e27603ae0d4efea6

commit dbd2053026a6af28adb1aa32e27603ae0d4efea6
Author:     Konstantin Belousov <kib@FreeBSD.org>
AuthorDate: 2021-04-05 03:38:07 +0000
Commit:     Konstantin Belousov <kib@FreeBSD.org>
CommitDate: 2021-04-09 20:46:24 +0000

    libc dl_iterate_phdr(): dlpi_tls_data is wrong

    This is the same change as d36d681615170590, but for libc static implementaion
    of dl_iterate_phdr().

    Reported by:    emacsray@gmail.com
    PR:     254774
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 week
    Differential revision:  https://reviews.freebsd.org/D29623

 lib/libc/gen/dlfcn.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Comment 10 Mark Linimon freebsd_committer freebsd_triage 2021-04-12 07:39:56 UTC
^Triage: over to committer for possible MFC consideration.