From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14131 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Szabolcs Nagy Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP Date: Thu, 16 May 2019 09:48:50 +0200 Message-ID: <20190516074850.GD16415@port70.net> References: <20190514020131.GC16415@port70.net> <20190516002051.GX23599@brightrain.aerifal.cx> 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="103124"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Mutt/1.10.1 (2018-07-13) To: musl@lists.openwall.com Original-X-From: musl-return-14147-gllmg-musl=m.gmane.org@lists.openwall.com Thu May 16 09:49:08 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 1hRB8U-000Qev-GF for gllmg-musl@m.gmane.org; Thu, 16 May 2019 09:49:06 +0200 Original-Received: (qmail 24251 invoked by uid 550); 16 May 2019 07:49:03 -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 24226 invoked from network); 16 May 2019 07:49:03 -0000 Mail-Followup-To: musl@lists.openwall.com Content-Disposition: inline In-Reply-To: <20190516002051.GX23599@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:14131 Archived-At: * Rich Felker [2019-05-15 20:20:51 -0400]: > 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? any time there are 'alignment gaps' in the static tls area for shared libs, since tls_offset += dso.tls.size is the only update to tls_offset which can be misaligned, it is not updated when dso.tls.offset is aligned up. the only exception is tls in the application: tls_offset is aligned up after the app tls. (which my introduce bigger gap than necessary) i can make the tls_offset fix a separate change from the p_vaddr%p_align!=0 fix. > > --- > > 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? to 'satisfy the formula' which uses p_align, using the max alignment seemed unnecessary overalignment. if all modules follow the same formula and static linking too then you only have to prove that one thing to work. > > #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