Re: [OpenWrt-Devel] Alsa-lib (libasound) segfaults on TLS variable (musl on mips)

Re: [OpenWrt-Devel] Alsa-lib (libasound) segfaults on TLS variable (musl on mips)

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

* Ted Hess <thess@kitschensync.net> [2015-06-23 18:04:35 -0400]:
> Segfault in 'snd_lib_error_set_local' (error.c) referencing
> static __thread snd_local_error_handler_t local_error;
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x0041b164 in snd_lib_error_set_local ()
> (gdb) bt
> #0 0x0041b164 in snd_lib_error_set_local ()
> #1 0x0041fb68 in try_config ()
> #2 0x00420d80 in snd_device_name_hint ()
> #3 0x0040a3be in pcm_list ()
> #4 0x0040e92a in main ()
> (gdb) disas
> Dump of assembler code for function snd_lib_error_set_local:
> 0x0041b12c <+0>: lui gp,0x8
> 0x0041b130 <+4>: addiu gp,gp,23668
> 0x0041b134 <+8>: addu gp,gp,t9
> 0x0041b138 <+12>: addiu sp,sp,-16
> 0x0041b13c <+16>: lw t9,-29872(gp)
> 0x0041b140 <+20>: sw ra,12(sp)
> 0x0041b144 <+24>: sw s0,8(sp)
> 0x0041b148 <+28>: sw gp,0(sp)
> 0x0041b14c <+32>: move s0,a0
> 0x0041b150 <+36>: addiu a0,gp,-29376
> 0x0041b154 <+40>: jalr t9
> 0x0041b158 <+44>: nop
> 0x0041b15c <+48>: lui v1,0x0
> 0x0041b160 <+52>: addu v1,v1,v0
> => 0x0041b164 <+56>: lw v0,-32768(v1)
> 0x0041b168 <+60>: sw s0,-32768(v1)

thanks for the report

the bug is that mips tls access uses a hard coded -32768
offset relative to whatever __tls_get_addr returned.

and musl did not account for this offset.

the attached patch fixes the issue for me,
we will fix it in musl soon.

[-- Attachment #2: mips_tls_fix.diff --]
[-- Type: text/x-diff, Size: 1191 bytes --]

diff --git a/arch/mips/pthread_arch.h b/arch/mips/pthread_arch.h
index f8e35ae..626b9bb 100644
--- a/arch/mips/pthread_arch.h
+++ b/arch/mips/pthread_arch.h
@@ -13,4 +13,6 @@ static inline struct pthread *__pthread_self()
 #define TLS_ABOVE_TP
 #define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread) + 0x7000)
 
+#define DTV_OFFSET 0x8000
+
 #define CANCEL_REG_IP (3-(union {int __i; char __b;}){1}.__b)
diff --git a/src/thread/__tls_get_addr.c b/src/thread/__tls_get_addr.c
index 3633396..bcc9be3 100644
--- a/src/thread/__tls_get_addr.c
+++ b/src/thread/__tls_get_addr.c
@@ -1,6 +1,10 @@
 #include <stddef.h>
 #include "pthread_impl.h"
 
+#ifndef DTV_OFFSET
+#define DTV_OFFSET 0
+#endif
+
 void *__tls_get_addr(size_t *v)
 {
 	pthread_t self = __pthread_self();
@@ -8,9 +12,9 @@ void *__tls_get_addr(size_t *v)
 	__attribute__((__visibility__("hidden")))
 	void *__tls_get_new(size_t *);
 	if (v[0]<=(size_t)self->dtv[0])
-		return (char *)self->dtv[v[0]]+v[1];
-	return __tls_get_new(v);
+		return (char *)self->dtv[v[0]]+v[1]+DTV_OFFSET;
+	return (char *)__tls_get_new(v)+DTV_OFFSET;
 #else
-	return (char *)self->dtv[1]+v[1];
+	return (char *)self->dtv[1]+v[1]+DTV_OFFSET;
 #endif
 }

Re: Alsa-lib (libasound) segfaults on TLS variable (musl on mips)

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 579 bytes --]

* Szabolcs Nagy <nsz@port70.net> [2015-06-24 22:57:54 +0200]:
> the bug is that mips tls access uses a hard coded -32768
> offset relative to whatever __tls_get_addr returned.
> 
> and musl did not account for this offset.
> 

only affects mips shared objects with 'static __thread' variables,

extern __thread variables worked fine (and only those were
tested in the libc-tests).

> the attached patch fixes the issue for me,
> we will fix it in musl soon.

better patch attached that undoes the offset for extern tls
vars in relocs handling.. (and with more consistent naming)

[-- Attachment #2: mips_tls_fix_2.diff --]
[-- Type: text/x-diff, Size: 1678 bytes --]

diff --git a/arch/mips/pthread_arch.h b/arch/mips/pthread_arch.h
index f8e35ae..904a248 100644
--- a/arch/mips/pthread_arch.h
+++ b/arch/mips/pthread_arch.h
@@ -13,4 +13,6 @@ static inline struct pthread *__pthread_self()
 #define TLS_ABOVE_TP
 #define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread) + 0x7000)
 
+#define DTP_OFFSET 0x8000
+
 #define CANCEL_REG_IP (3-(union {int __i; char __b;}){1}.__b)
diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
index b77c6f6..b4ca410 100644
--- a/src/ldso/dynlink.c
+++ b/src/ldso/dynlink.c
@@ -337,7 +337,10 @@ static void do_relocs(struct dso *dso, size_t *rel, size_t rel_size, size_t stri
 			*reloc_addr = def.dso->tls_id;
 			break;
 		case REL_DTPOFF:
-			*reloc_addr = tls_val + addend;
+#ifndef DTP_OFFSET
+#define DTP_OFFSET 0
+#endif
+			*reloc_addr = tls_val + addend - DTP_OFFSET;
 			break;
 #ifdef TLS_ABOVE_TP
 		case REL_TPOFF:
diff --git a/src/thread/__tls_get_addr.c b/src/thread/__tls_get_addr.c
index 3633396..94ea03c 100644
--- a/src/thread/__tls_get_addr.c
+++ b/src/thread/__tls_get_addr.c
@@ -1,6 +1,10 @@
 #include <stddef.h>
 #include "pthread_impl.h"
 
+#ifndef DTP_OFFSET
+#define DTP_OFFSET 0
+#endif
+
 void *__tls_get_addr(size_t *v)
 {
 	pthread_t self = __pthread_self();
@@ -8,9 +12,9 @@ void *__tls_get_addr(size_t *v)
 	__attribute__((__visibility__("hidden")))
 	void *__tls_get_new(size_t *);
 	if (v[0]<=(size_t)self->dtv[0])
-		return (char *)self->dtv[v[0]]+v[1];
-	return __tls_get_new(v);
+		return (char *)self->dtv[v[0]]+v[1]+DTP_OFFSET;
+	return (char *)__tls_get_new(v)+DTP_OFFSET;
 #else
-	return (char *)self->dtv[1]+v[1];
+	return (char *)self->dtv[1]+v[1]+DTP_OFFSET;
 #endif
 }

[-- Attachment #3: Type: text/plain, Size: 172 bytes --]

_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel

Re: Re: [OpenWrt-Devel] Alsa-lib (libasound) segfaults on TLS variable (musl on mips)

[Rich Felker]

On Thu, Jun 25, 2015 at 01:08:48AM +0200, Szabolcs Nagy wrote:
> * Szabolcs Nagy <nsz@port70.net> [2015-06-24 22:57:54 +0200]:
> > the bug is that mips tls access uses a hard coded -32768
> > offset relative to whatever __tls_get_addr returned.
> > 
> > and musl did not account for this offset.
> > 
> 
> only affects mips shared objects with 'static __thread' variables,
> 
> extern __thread variables worked fine (and only those were
> tested in the libc-tests).
> 
> > the attached patch fixes the issue for me,
> > we will fix it in musl soon.
> 
> better patch attached that undoes the offset for extern tls
> vars in relocs handling.. (and with more consistent naming)

In principle powerpc looks like it should be affected too, but it's
not. This seems to be because powerpc has a R_PPC_DTPREL32 relocation
for the local-dynamic TLS access, allowing the implementaton to use
whatever offset it likes as long as it's internally consistent,
whereas mips has a hard-coded value in the GOT with no relocation
attached to it it. I'll try to check and see if other archs are
affected and apply this patch or a variant of it soon.

Rich

Re: Re: [OpenWrt-Devel] Alsa-lib (libasound) segfaults on TLS variable (musl on mips)

[Rich Felker]

On Thu, Jun 25, 2015 at 01:08:48AM +0200, Szabolcs Nagy wrote:
> * Szabolcs Nagy <nsz@port70.net> [2015-06-24 22:57:54 +0200]:
> > the bug is that mips tls access uses a hard coded -32768
> > offset relative to whatever __tls_get_addr returned.
> > 
> > and musl did not account for this offset.
> > 
> 
> only affects mips shared objects with 'static __thread' variables,
> 
> extern __thread variables worked fine (and only those were
> tested in the libc-tests).
> 
> > the attached patch fixes the issue for me,
> > we will fix it in musl soon.
> 
> better patch attached that undoes the offset for extern tls
> vars in relocs handling.. (and with more consistent naming)

> diff --git a/arch/mips/pthread_arch.h b/arch/mips/pthread_arch.h
> index f8e35ae..904a248 100644
> --- a/arch/mips/pthread_arch.h
> +++ b/arch/mips/pthread_arch.h
> @@ -13,4 +13,6 @@ static inline struct pthread *__pthread_self()
>  #define TLS_ABOVE_TP
>  #define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread) + 0x7000)
>  
> +#define DTP_OFFSET 0x8000
> +
>  #define CANCEL_REG_IP (3-(union {int __i; char __b;}){1}.__b)
> diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
> index b77c6f6..b4ca410 100644
> --- a/src/ldso/dynlink.c
> +++ b/src/ldso/dynlink.c
> @@ -337,7 +337,10 @@ static void do_relocs(struct dso *dso, size_t *rel, size_t rel_size, size_t stri
>  			*reloc_addr = def.dso->tls_id;
>  			break;
>  		case REL_DTPOFF:
> -			*reloc_addr = tls_val + addend;
> +#ifndef DTP_OFFSET
> +#define DTP_OFFSET 0
> +#endif
> +			*reloc_addr = tls_val + addend - DTP_OFFSET;
>  			break;
>  #ifdef TLS_ABOVE_TP
>  		case REL_TPOFF:
> diff --git a/src/thread/__tls_get_addr.c b/src/thread/__tls_get_addr.c
> index 3633396..94ea03c 100644
> --- a/src/thread/__tls_get_addr.c
> +++ b/src/thread/__tls_get_addr.c
> @@ -1,6 +1,10 @@
>  #include <stddef.h>
>  #include "pthread_impl.h"
>  
> +#ifndef DTP_OFFSET
> +#define DTP_OFFSET 0
> +#endif
> +
>  void *__tls_get_addr(size_t *v)
>  {
>  	pthread_t self = __pthread_self();
> @@ -8,9 +12,9 @@ void *__tls_get_addr(size_t *v)
>  	__attribute__((__visibility__("hidden")))
>  	void *__tls_get_new(size_t *);
>  	if (v[0]<=(size_t)self->dtv[0])
> -		return (char *)self->dtv[v[0]]+v[1];
> -	return __tls_get_new(v);
> +		return (char *)self->dtv[v[0]]+v[1]+DTP_OFFSET;
> +	return (char *)__tls_get_new(v)+DTP_OFFSET;
>  #else
> -	return (char *)self->dtv[1]+v[1];
> +	return (char *)self->dtv[1]+v[1]+DTP_OFFSET;
>  #endif
>  }

I think having additional instructions in __tls_get_addr is the wrong
approach. This function is the bottleneck for TLS access performance.
If the offset is needed, it should be stored in the dtv[i] pointers,
which then need to be adjusted at the point where they're setup.

Rich

Re: Re: [OpenWrt-Devel] Alsa-lib (libasound) segfaults on TLS variable (musl on mips)

[Rich Felker]

On Thu, Jun 25, 2015 at 01:06:42PM -0400, Rich Felker wrote:
> On Thu, Jun 25, 2015 at 01:08:48AM +0200, Szabolcs Nagy wrote:
> > * Szabolcs Nagy <nsz@port70.net> [2015-06-24 22:57:54 +0200]:
> > > the bug is that mips tls access uses a hard coded -32768
> > > offset relative to whatever __tls_get_addr returned.
> > > 
> > > and musl did not account for this offset.
> > > 
> > 
> > only affects mips shared objects with 'static __thread' variables,
> > 
> > extern __thread variables worked fine (and only those were
> > tested in the libc-tests).
> > 
> > > the attached patch fixes the issue for me,
> > > we will fix it in musl soon.
> > 
> > better patch attached that undoes the offset for extern tls
> > vars in relocs handling.. (and with more consistent naming)
> 
> > diff --git a/arch/mips/pthread_arch.h b/arch/mips/pthread_arch.h
> > index f8e35ae..904a248 100644
> > --- a/arch/mips/pthread_arch.h
> > +++ b/arch/mips/pthread_arch.h
> > @@ -13,4 +13,6 @@ static inline struct pthread *__pthread_self()
> >  #define TLS_ABOVE_TP
> >  #define TP_ADJ(p) ((char *)(p) + sizeof(struct pthread) + 0x7000)
> >  
> > +#define DTP_OFFSET 0x8000
> > +
> >  #define CANCEL_REG_IP (3-(union {int __i; char __b;}){1}.__b)
> > diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
> > index b77c6f6..b4ca410 100644
> > --- a/src/ldso/dynlink.c
> > +++ b/src/ldso/dynlink.c
> > @@ -337,7 +337,10 @@ static void do_relocs(struct dso *dso, size_t *rel, size_t rel_size, size_t stri
> >  			*reloc_addr = def.dso->tls_id;
> >  			break;
> >  		case REL_DTPOFF:
> > -			*reloc_addr = tls_val + addend;
> > +#ifndef DTP_OFFSET
> > +#define DTP_OFFSET 0
> > +#endif
> > +			*reloc_addr = tls_val + addend - DTP_OFFSET;
> >  			break;
> >  #ifdef TLS_ABOVE_TP
> >  		case REL_TPOFF:
> > diff --git a/src/thread/__tls_get_addr.c b/src/thread/__tls_get_addr.c
> > index 3633396..94ea03c 100644
> > --- a/src/thread/__tls_get_addr.c
> > +++ b/src/thread/__tls_get_addr.c
> > @@ -1,6 +1,10 @@
> >  #include <stddef.h>
> >  #include "pthread_impl.h"
> >  
> > +#ifndef DTP_OFFSET
> > +#define DTP_OFFSET 0
> > +#endif
> > +
> >  void *__tls_get_addr(size_t *v)
> >  {
> >  	pthread_t self = __pthread_self();
> > @@ -8,9 +12,9 @@ void *__tls_get_addr(size_t *v)
> >  	__attribute__((__visibility__("hidden")))
> >  	void *__tls_get_new(size_t *);
> >  	if (v[0]<=(size_t)self->dtv[0])
> > -		return (char *)self->dtv[v[0]]+v[1];
> > -	return __tls_get_new(v);
> > +		return (char *)self->dtv[v[0]]+v[1]+DTP_OFFSET;
> > +	return (char *)__tls_get_new(v)+DTP_OFFSET;
> >  #else
> > -	return (char *)self->dtv[1]+v[1];
> > +	return (char *)self->dtv[1]+v[1]+DTP_OFFSET;
> >  #endif
> >  }
> 
> I think having additional instructions in __tls_get_addr is the wrong
> approach. This function is the bottleneck for TLS access performance.
> If the offset is needed, it should be stored in the dtv[i] pointers,
> which then need to be adjusted at the point where they're setup.

While this would be slightly more efficient, it also has more
complexity and it's more invasive. I'll leave this open as an idea for
the future, but for now I've committed something close to nsz's patch
as commit 6ba5517a460c6c438f64d69464fdfc3269a4c91a.

If anything still seems broken, let me know.

Rich