Re: [PATCH][BZ #16613] Allow audit libraries to use TLS.

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Thu, Feb 20, 2014 at 10:24:04AM -0500, Carlos O'Donell wrote:
> This is a Fedora/RHEL specific patch which should go upstream
> since it fixes a real and generic problem.
> 
> At present the dynamic loader does not correctly account
> for TLS usage in audit libraries. This results in a crash
> in the dynamic loader if the audit library uses TLS.
> 
> The fix for this is to detect the usage of TLS in the audit
> library and bump the generation counter.
> 
> The following patch does just that. No regressions on x86-64,
> and a new test case added for just this case which segfaults
> without the patch but works with the patch.
> 
> OK to checkin?
> 
> 2014-02-20  Ulrich Drepper  <drepper@gmail.com>
> 	    Carlos O'Donell  <carlos@redhat.com>
> 
> 	[BZ #16613]
> 	* elf/dl-tls.c (_dl_count_modids): New function.
> 	* sysdeps/generic/ldsodefs.h: Declare _dl_count_modids.
> 	* elf/rtld.c (dl_main): Call _dl_count_modids to track TLS usage in
> 	audit library and increment generation counter.
> 	(_dl_allocate_tls_init): Add assertion to check TLS generation count.
> 	* elf/tst-audit9.c: New file.
> 	* elf/tst-auditmod9a.c: New file.
> 	* elf/tst-auditmod9b.c: New file.
> 	* elf/Makefile: Add rules to build and run tst-audit9.
>  
> diff --git a/elf/Makefile b/elf/Makefile
> index 6f4f2f8..932b479 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -144,7 +144,7 @@ tests += loadtest restest1 preloadtest loadfail multiload origtest resolvfail \
>  	 tst-dlmodcount tst-dlopenrpath tst-deep1 \
>  	 tst-dlmopen1 tst-dlmopen2 tst-dlmopen3 \
>  	 unload3 unload4 unload5 unload6 unload7 unload8 tst-global1 order2 \
> -	 tst-audit1 tst-audit2 tst-audit8 \
> +	 tst-audit1 tst-audit2 tst-audit8 tst-audit9 \
>  	 tst-stackguard1 tst-addr1 tst-thrlock \
>  	 tst-unique1 tst-unique2 tst-unique3 tst-unique4 \
>  	 tst-initorder tst-initorder2 tst-relsort1 tst-null-argv \
> @@ -203,6 +203,7 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>  		order2mod1 order2mod2 order2mod3 order2mod4 \
>  		tst-unique1mod1 tst-unique1mod2 \
>  		tst-unique2mod1 tst-unique2mod2 \
> +		tst-auditmod9a tst-auditmod9b \
>  		tst-unique3lib tst-unique3lib2 \
>  		tst-unique4lib \
>  		tst-initordera1 tst-initorderb1 \
> @@ -560,6 +561,8 @@ unload4mod1.so-no-z-defs = yes
>  ifuncmod1.so-no-z-defs = yes
>  ifuncmod5.so-no-z-defs = yes
>  ifuncmod6.so-no-z-defs = yes
> +tst-auditmod9a.so-no-z-defs = yes
> +tst-auditmod9b.so-no-z-defs = yes
>  
>  ifeq ($(build-shared),yes)
>  # Build all the modules even when not actually running test programs.
> @@ -998,6 +1001,10 @@ tst-audit1-ENV = LD_AUDIT=$(objpfx)tst-auditmod1.so
>  $(objpfx)tst-audit2.out: $(objpfx)tst-auditmod1.so
>  tst-audit2-ENV = LD_AUDIT=$(objpfx)tst-auditmod1.so
>  
> +$(objpfx)tst-audit9: $(libdl)
> +$(objpfx)tst-audit9.out: $(objpfx)tst-auditmod9a.so $(objpfx)tst-auditmod9b.so
> +tst-audit9-ENV = LD_AUDIT=$(objpfx)tst-auditmod9a.so
> +
>  $(objpfx)tst-audit8: $(common-objpfx)math/libm.so
>  $(objpfx)tst-audit8.out: $(objpfx)tst-auditmod1.so
>  tst-audit8-ENV = LD_AUDIT=$(objpfx)tst-auditmod1.so
> diff --git a/elf/dl-tls.c b/elf/dl-tls.c
> index 9454d06..cbafdd9 100644
> --- a/elf/dl-tls.c
> +++ b/elf/dl-tls.c
> @@ -105,6 +105,28 @@ _dl_next_tls_modid (void)
>  }
>  
>  
> +size_t
> +internal_function
> +_dl_count_modids (void)
> +{
> +  if (! __builtin_expect (GL(dl_tls_dtv_gaps), true))

This should use __glibc_{,un}likely.  Also, _dl_tls_next_modid has the
opposite expectation for the value of dl_tls_dtv_gaps:

  if (__builtin_expect (GL(dl_tls_dtv_gaps), false))

Since we're explicitly specifying the likelihood of the branch, you
might want to explain why each of those annotations are true, or fix
the annotation which isn't.

Siddhesh

> +    return GL(dl_tls_max_dtv_idx);
> +
> +  size_t n = 0;
> +  struct dtv_slotinfo_list *runp = GL(dl_tls_dtv_slotinfo_list);
> +  while (runp != NULL)
> +    {
> +      for (size_t i = 0; i < runp->len; ++i)
> +	if (runp->slotinfo[i].map != NULL)
> +	  ++n;
> +
> +      runp = runp->next;
> +    }
> +
> +  return n;
> +}
> +
> +
>  #ifdef SHARED
>  void
>  internal_function
> @@ -407,6 +429,7 @@ _dl_allocate_tls_init (void *result)
>  
>  	  /* Keep track of the maximum generation number.  This might
>  	     not be the generation counter.  */
> +	  assert (listp->slotinfo[cnt].gen <= GL(dl_tls_generation));
>  	  maxgen = MAX (maxgen, listp->slotinfo[cnt].gen);
>  
>  	  if (map->l_tls_offset == NO_TLS_OFFSET
> diff --git a/elf/rtld.c b/elf/rtld.c
> index aa50cab..7f1413a 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1569,6 +1569,10 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
>  	}
>      }
>  
> +  /* Keep track of the currently loaded modules to count how many
> +     non-audit modules which use TLS are loaded.  */
> +  size_t count_modids = _dl_count_modids ();
> +
>    /* Set up debugging before the debugger is notified for the first time.  */
>  #ifdef ELF_MACHINE_DEBUG_SETUP
>    /* Some machines (e.g. MIPS) don't use DT_DEBUG in this way.  */
> @@ -2220,7 +2224,8 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
>  	_dl_start_profile ();
>      }
>  
> -  if (!was_tls_init_tp_called && GL(dl_tls_max_dtv_idx) > 0)
> +  if ((!was_tls_init_tp_called && GL(dl_tls_max_dtv_idx) > 0)
> +      || count_modids != _dl_count_modids ())
>      ++GL(dl_tls_generation);
>  
>    /* Now that we have completed relocation, the initializer data
> diff --git a/elf/tst-audit9.c b/elf/tst-audit9.c
> new file mode 100644
> index 0000000..0982d8b
> --- /dev/null
> +++ b/elf/tst-audit9.c
> @@ -0,0 +1,8 @@
> +#include <dlfcn.h>
> +
> +int main(void)
> +{
> +  void *h = dlopen("$ORIGIN/tst-auditmod9b.so", RTLD_LAZY);
> +  int (*fp)(void) = dlsym(h, "f");
> +  return fp() - 1;
> +}
> diff --git a/elf/tst-auditmod9a.c b/elf/tst-auditmod9a.c
> new file mode 100644
> index 0000000..7f66e96
> --- /dev/null
> +++ b/elf/tst-auditmod9a.c
> @@ -0,0 +1,16 @@
> +#include <stdint.h>
> +
> +__thread int var;
> +
> +unsigned int
> +la_version (unsigned int v)
> +{
> +  return v;
> +}
> +
> +void
> +la_activity (uintptr_t *cookie, unsigned int flag)
> +{
> +  ++var;
> +}
> +
> diff --git a/elf/tst-auditmod9b.c b/elf/tst-auditmod9b.c
> new file mode 100644
> index 0000000..8eeeb49
> --- /dev/null
> +++ b/elf/tst-auditmod9b.c
> @@ -0,0 +1,6 @@
> +__thread int a;
> +
> +int f(void)
> +{
> +  return ++a;
> +}
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index ffeb093..65cd709 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -957,6 +957,9 @@ extern void _dl_sysdep_start_cleanup (void)
>  /* Determine next available module ID.  */
>  extern size_t _dl_next_tls_modid (void) internal_function attribute_hidden;
>  
> +/* Count the modules with TLS segments.  */
> +extern size_t _dl_count_modids (void) internal_function attribute_hidden;
> +
>  /* Calculate offset of the TLS blocks in the static TLS block.  */
>  extern void _dl_determine_tlsoffset (void) internal_function attribute_hidden;
>  
> ---
> 
> Cheers,
> Carlos.

Attachment: pgp8PCNbxhq2d.pgp
Description: PGP signature