fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP

Submitted by Szabolcs Nagy on May 16, 2019, 10:51 p.m.

Details

Message ID 20190516225117.GF16415@port70.net
State New
Series "fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP"
Headers show

Commit Message

Szabolcs Nagy May 16, 2019, 10:51 p.m.
* 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

Patch hide | download patch | download mbox

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


Comments

Rich Felker May 17, 2019, 1:50 a.m.
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
Szabolcs Nagy May 17, 2019, 12:32 p.m.
* 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.