* [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP @ 2019-05-14 2:01 Szabolcs Nagy 2019-05-16 0:20 ` Rich Felker 0 siblings, 1 reply; 8+ messages in thread From: Szabolcs Nagy @ 2019-05-14 2:01 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 368 bytes --] 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. [-- Attachment #2: 0001-fix-tls-offsets-when-p_vaddr-p_align-0-for-TLS_ABOVE.patch --] [-- Type: text/x-diff, Size: 2933 bytes --] From 8c94fcbc9faeb8b07132506757c3d3973652420e Mon Sep 17 00:00:00 2001 From: Szabolcs Nagy <nsz@port70.net> 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). 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. --- 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; #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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP 2019-05-14 2:01 [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP Szabolcs Nagy @ 2019-05-16 0:20 ` Rich Felker 2019-05-16 7:48 ` Szabolcs Nagy 0 siblings, 1 reply; 8+ messages in thread From: Rich Felker @ 2019-05-16 0:20 UTC (permalink / raw) To: musl 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 <nsz@port70.net> > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP 2019-05-16 0:20 ` Rich Felker @ 2019-05-16 7:48 ` Szabolcs Nagy 2019-05-16 13:22 ` Rich Felker 0 siblings, 1 reply; 8+ messages in thread From: Szabolcs Nagy @ 2019-05-16 7:48 UTC (permalink / raw) To: musl * Rich Felker <dalias@libc.org> [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 <nsz@port70.net> > > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP 2019-05-16 7:48 ` Szabolcs Nagy @ 2019-05-16 13:22 ` Rich Felker 2019-05-16 22:51 ` Szabolcs Nagy 0 siblings, 1 reply; 8+ messages in thread From: Rich Felker @ 2019-05-16 13:22 UTC (permalink / raw) To: musl On Thu, May 16, 2019 at 09:48:50AM +0200, Szabolcs Nagy wrote: > * Rich Felker <dalias@libc.org> [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 <nsz@port70.net> > > > 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. I'd like that, both because it was non-obvious to me that this was another major change, and because it fixes a separate, major user-facing bug (this is what was breaking rust, and could break lots of other things) rather than the new issue with lld's Bionic hack. > > > > --- > > > 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. OK, this sounds good. After reading it less tired it makes sense to me too. Thanks for the explanation. Rich ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP 2019-05-16 13:22 ` Rich Felker @ 2019-05-16 22:51 ` Szabolcs Nagy 2019-05-17 1:50 ` Rich Felker 0 siblings, 1 reply; 8+ messages in thread From: Szabolcs Nagy @ 2019-05-16 22:51 UTC (permalink / raw) To: musl [-- Attachment #1: Type: text/plain, Size: 430 bytes --] * Rich Felker <dalias@libc.org> [2019-05-16 09:22:46 -0400]: > > i can make the tls_offset fix a separate change from > > the p_vaddr%p_align!=0 fix. > > I'd like that, both because it was non-obvious to me that this was > another major change, and because it fixes a separate, major > user-facing bug (this is what was breaking rust, and could break lots > of other things) rather than the new issue with lld's Bionic hack. ok [-- Attachment #2: 0001-fix-static-tls-offsets-of-shared-libs-on-TLS_ABOVE_T.patch --] [-- Type: text/x-diff, Size: 2089 bytes --] From 36d6bcf5b1a0add3f825aec57b72630520a18ccb Mon Sep 17 00:00:00 2001 From: Szabolcs Nagy <nsz@port70.net> Date: Thu, 16 May 2019 17:15:33 +0000 Subject: [PATCH 1/2] fix static tls offsets of shared libs on TLS_ABOVE_TP targets tls_offset should always point to the end of the allocated static tls area, but this was not handled correctly on "tls variant 1" targets in the dynamic linker: after application tls was allocated, tls_offset was aligned up, potentially wasting tls space. (alignment may be needed at the begining of the tls area, not at the end, but that will be fixed separately as it is unlikely to affect real binaries.) when static tls was allocated for a shared library, tls_offset was only updated with the size of the tls segment which does not include alignment gaps, which can easily happen if the tls size update for one library leaves tls_offset misaligned for the next one. this can cause oob access in __copy_tls or arbitrary breakage at tls access. (the issue was observed on aarch64 with rust binaries) --- ldso/dynlink.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/ldso/dynlink.c b/ldso/dynlink.c index 42a5470d..566b69e6 100644 --- a/ldso/dynlink.c +++ b/ldso/dynlink.c @@ -1128,7 +1128,7 @@ static struct dso *load_library(const char *name, struct dso *needed_by) #ifdef TLS_ABOVE_TP p->tls.offset = tls_offset + ( (tls_align-1) & -(tls_offset + (uintptr_t)p->tls.image) ); - tls_offset += p->tls.size; + tls_offset = p->tls.offset + p->tls.size; #else tls_offset += p->tls.size + p->tls.align - 1; tls_offset -= (tls_offset + (uintptr_t)p->tls.image) @@ -1798,9 +1798,7 @@ _Noreturn void __dls3(size_t *sp) #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) ); + 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) -- 2.21.0 [-- Attachment #3: 0002-fix-tls-offsets-when-p_vaddr-p_align-0-on-TLS_ABOVE_.patch --] [-- Type: text/x-diff, Size: 2412 bytes --] From 28dd0b581b4d8b0903447b5e169155f11be94cf7 Mon Sep 17 00:00:00 2001 From: Szabolcs Nagy <nsz@port70.net> Date: Mon, 13 May 2019 18:47:11 +0000 Subject: [PATCH 2/2] fix tls offsets when p_vaddr%p_align != 0 on TLS_ABOVE_TP targets 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 is handled 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. --- ldso/dynlink.c | 7 ++++--- src/env/__init_tls.c | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/ldso/dynlink.c b/ldso/dynlink.c index 566b69e6..f9a7cc06 100644 --- a/ldso/dynlink.c +++ b/ldso/dynlink.c @@ -1126,8 +1126,8 @@ 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) ); + p->tls.offset = tls_offset + ( (p->tls_align-1) & + (-tls_offset + (uintptr_t)p->tls.image) ); tls_offset = p->tls.offset + p->tls.size; #else tls_offset += p->tls.size + p->tls.align - 1; @@ -1797,7 +1797,8 @@ _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); + 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 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP 2019-05-16 22:51 ` Szabolcs Nagy @ 2019-05-17 1:50 ` Rich Felker 2019-05-17 12:32 ` Szabolcs Nagy 0 siblings, 1 reply; 8+ messages in thread From: Rich Felker @ 2019-05-17 1:50 UTC (permalink / raw) To: musl On Fri, May 17, 2019 at 12:51:18AM +0200, Szabolcs Nagy wrote: > * Rich Felker <dalias@libc.org> [2019-05-16 09:22:46 -0400]: > > > i can make the tls_offset fix a separate change from > > > the p_vaddr%p_align!=0 fix. > > > > I'd like that, both because it was non-obvious to me that this was > > another major change, and because it fixes a separate, major > > user-facing bug (this is what was breaking rust, and could break lots > > of other things) rather than the new issue with lld's Bionic hack. > > ok Thanks! > From 28dd0b581b4d8b0903447b5e169155f11be94cf7 Mon Sep 17 00:00:00 2001 > From: Szabolcs Nagy <nsz@port70.net> > Date: Mon, 13 May 2019 18:47:11 +0000 > Subject: [PATCH 2/2] fix tls offsets when p_vaddr%p_align != 0 on TLS_ABOVE_TP > targets > > 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 is handled > 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. > --- > ldso/dynlink.c | 7 ++++--- > src/env/__init_tls.c | 3 ++- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/ldso/dynlink.c b/ldso/dynlink.c > index 566b69e6..f9a7cc06 100644 > --- a/ldso/dynlink.c > +++ b/ldso/dynlink.c > @@ -1126,8 +1126,8 @@ 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) ); > + p->tls.offset = tls_offset + ( (p->tls_align-1) & ~~~~~~~~~ This should be tls.align. I can fix it up though when applying. Rich ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP 2019-05-17 1:50 ` Rich Felker @ 2019-05-17 12:32 ` Szabolcs Nagy 2019-05-17 16:01 ` Fangrui Song 0 siblings, 1 reply; 8+ messages in thread From: Szabolcs Nagy @ 2019-05-17 12:32 UTC (permalink / raw) To: musl * Rich Felker <dalias@libc.org> [2019-05-16 21:50:43 -0400]: > On Fri, May 17, 2019 at 12:51:18AM +0200, Szabolcs Nagy wrote: > > + p->tls.offset = tls_offset + ( (p->tls_align-1) & > ~~~~~~~~~ > > This should be tls.align. I can fix it up though when applying. yes, my bad. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP 2019-05-17 12:32 ` Szabolcs Nagy @ 2019-05-17 16:01 ` Fangrui Song 0 siblings, 0 replies; 8+ messages in thread From: Fangrui Song @ 2019-05-17 16:01 UTC (permalink / raw) To: musl On 2019-05-17, Szabolcs Nagy wrote: >* Rich Felker <dalias@libc.org> [2019-05-16 21:50:43 -0400]: >> On Fri, May 17, 2019 at 12:51:18AM +0200, Szabolcs Nagy wrote: >> > + p->tls.offset = tls_offset + ( (p->tls_align-1) & >> ~~~~~~~~~ >> >> This should be tls.align. I can fix it up though when applying. > >yes, my bad. I have verified 0001-fix-tls-offsets-when-p_vaddr-p_align-0-for-TLS_ABOVE.patch With the following lld patch, p_vaddr%p_align!=0, the reproduce program in https://bugs.llvm.org/show_bug.cgi?id=41527 works. (a local exec variable defined in executable accessed by another module via `extern __thread int a` (initial exec/generic dynamic)) diff --git i/ELF/InputSection.cpp w/ELF/InputSection.cpp index 1bf67e2a3..ff365ef0c 100644 --- i/ELF/InputSection.cpp +++ w/ELF/InputSection.cpp @@ -594,7 +594,8 @@ static int64_t getTlsTpOffset() { // NB: While the ARM/AArch64 ABI formally has a 2-word TCB size, lld // effectively increases the TCB size to 8 words for Android compatibility. // It accomplishes this by increasing the segment's alignment. - return alignTo(Config->Wordsize * 2, Out::TlsPhdr->p_align); + return alignTo(Config->Wordsize * 2, Out::TlsPhdr->p_align, + Out::TlsPhdr->FirstSec->Addr); case EM_386: case EM_X86_64: // Variant 2. The TLS segment is located just before the thread pointer. (I hope someone can improve my qemu workflow: qemu-system-aarch64 -M virt -cpu cortex-a57 -nographic -smp 1 -m 2048 -kernel vmlinuz-vanilla -initrd initramfs-vanilla -append "console=ttyAMA0 ip=dhcp alpine_repo=http://dl-cdn.alpinelinux.org/alpine/edge/main") ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-05-17 16:01 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-14 2:01 [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP Szabolcs Nagy 2019-05-16 0:20 ` Rich Felker 2019-05-16 7:48 ` Szabolcs Nagy 2019-05-16 13:22 ` Rich Felker 2019-05-16 22:51 ` Szabolcs Nagy 2019-05-17 1:50 ` Rich Felker 2019-05-17 12:32 ` Szabolcs Nagy 2019-05-17 16:01 ` Fangrui Song
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).