From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14129 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP Date: Wed, 15 May 2019 20:20:51 -0400 Message-ID: <20190516002051.GX23599@brightrain.aerifal.cx> References: <20190514020131.GC16415@port70.net> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="209253"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-14145-gllmg-musl=m.gmane.org@lists.openwall.com Thu May 16 02:21:07 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 1hR48x-000sLB-92 for gllmg-musl@m.gmane.org; Thu, 16 May 2019 02:21:07 +0200 Original-Received: (qmail 27822 invoked by uid 550); 16 May 2019 00:21:04 -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 27801 invoked from network); 16 May 2019 00:21:03 -0000 Content-Disposition: inline In-Reply-To: <20190514020131.GC16415@port70.net> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:14129 Archived-At: On Tue, May 14, 2019 at 04:01:31AM +0200, Szabolcs Nagy wrote: > this came up because lld changed tls alignment on aarch64 as a > workaround for a bionic abi issue https://reviews.llvm.org/D53906 > but lld does not handle p_vaddr%p_align!=0 right so it broke on glibc > https://reviews.llvm.org/D61824 > > the patch is untested (bfd linker cannot seem to create problematic > elf objects), but at least there are no regressions with libc-test. > >From 8c94fcbc9faeb8b07132506757c3d3973652420e Mon Sep 17 00:00:00 2001 > From: Szabolcs Nagy > Date: Mon, 13 May 2019 18:47:11 +0000 > Subject: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP > > currently the bfd linker does not seem to create tls segments where > p_vaddr%p_align != 0, but this is valid in ELF and then the runtime > computed tls offset must satisfy > > offset%p_align == (base+p_vaddr)%p_align > > and in case of local exec tls (main executable) the smallest such > offset must be used (otherwise it is incompatible with the offset > computed by the static linker). the !TLS_ABOVE_TP case handled this > correctly (the offset is negative then in the formula). > > the ldso code for TLS_ABOVE_TP is changed so the static tls offset > of each module satisfies the formula and tls_offset always points > to the end of the allocated static tls area (and not aligned up to > tls_align or MIN_TLS_ALIGN). I guess this saves some wasted memory? > the tls_offset computation was wrong > when multiple modules were loaded with static tls and in some the > tls segment p_memsz%p_align != 0. I don't understand this part. Are you saying we're currently misaligning TLS for some libraries now? > --- > ldso/dynlink.c | 13 ++++++------- > src/env/__init_tls.c | 3 ++- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/ldso/dynlink.c b/ldso/dynlink.c > index 42a5470d..6dc39483 100644 > --- a/ldso/dynlink.c > +++ b/ldso/dynlink.c > @@ -1126,9 +1126,9 @@ static struct dso *load_library(const char *name, struct dso *needed_by) > p->tls_id = ++tls_cnt; > tls_align = MAXP2(tls_align, p->tls.align); > #ifdef TLS_ABOVE_TP > - p->tls.offset = tls_offset + ( (tls_align-1) & > - -(tls_offset + (uintptr_t)p->tls.image) ); > - tls_offset += p->tls.size; > + p->tls.offset = tls_offset + ( (p->tls.align-1) & > + (-tls_offset + (uintptr_t)p->tls.image) ); > + tls_offset = p->tls.offset + p->tls.size; Is there a motivation for the seemingly independent change from use of tls_align to use of p->tls.align here? > #else > tls_offset += p->tls.size + p->tls.align - 1; > tls_offset -= (tls_offset + (uintptr_t)p->tls.image) > @@ -1797,10 +1797,9 @@ _Noreturn void __dls3(size_t *sp) > app.tls_id = tls_cnt = 1; > #ifdef TLS_ABOVE_TP > app.tls.offset = GAP_ABOVE_TP; > - app.tls.offset += -GAP_ABOVE_TP & (app.tls.align-1); > - tls_offset = app.tls.offset + app.tls.size > - + ( -((uintptr_t)app.tls.image + app.tls.size) > - & (app.tls.align-1) ); > + app.tls.offset += (-GAP_ABOVE_TP + (uintptr_t)app.tls.image) > + & (app.tls.align-1); > + tls_offset = app.tls.offset + app.tls.size; > #else > tls_offset = app.tls.offset = app.tls.size > + ( -((uintptr_t)app.tls.image + app.tls.size) > diff --git a/src/env/__init_tls.c b/src/env/__init_tls.c > index 5f12500c..772baba3 100644 > --- a/src/env/__init_tls.c > +++ b/src/env/__init_tls.c > @@ -115,7 +115,8 @@ static void static_init_tls(size_t *aux) > & (main_tls.align-1); > #ifdef TLS_ABOVE_TP > main_tls.offset = GAP_ABOVE_TP; > - main_tls.offset += -GAP_ABOVE_TP & (main_tls.align-1); > + main_tls.offset += (-GAP_ABOVE_TP + (uintptr_t)main_tls.image) > + & (main_tls.align-1); > #else > main_tls.offset = main_tls.size; > #endif > -- > 2.21.0 > I think you're probably right about all these things and I want to apply this, but I also want to understand it a bit better first. Rich