[PATCH][BZ #19329] Fix race between tls allocation at thread creation and dlopen

[Szabolcs Nagy]

Changes in patch v2:

* The listp->next race is fixed.

* GL(dl_tls_generation) is loaded before GL(dl_tls_max_dtv_idx).
   (matters if several dlopen happens during _dl_allocate_tls_init
   between those two loads.)

* more detailed analysis below.

I can trigger the bug on x86_64 by loading a library with >50 deps
with tls and concurrently creating >1000 threads, this was not turned
into a glibc test case, but attached to the bug report.
(On aarch64 the failure happens to be easier to trigger.)


The following failures can be triggered:

Inconsistency detected by ld.so: dl-tls.c: 493: _dl_allocate_tls_init: Assertion `listp->slotinfo[cnt].gen <= 
_rtld_local._dl_tls_generation' failed!

and

Inconsistency detected by ld.so: dl-tls.c: 525: _dl_allocate_tls_init: Assertion `listp != NULL' failed!

dlopen modifies tls related dynamic linker data structures while holding
the GL(dl_load_lock) if the loaded library has tls.

Meanwhile at thread creation the same globals are accessed when the dtv
and tls is set up for the new thread in _dl_allocate_tls_init without
holding any locks.

At least the following global objects may have conflicting access:

   GL(dl_tls_max_dtv_idx)
   GL(dl_tls_generation)
   listp->slotinfo[i].map
   listp->slotinfo[i].gen
   listp->next

where listp points to a node in GL(dl_tls_dtv_slotinfo_list).

The race window for the first assert failure above is short compared to
dlopen and thread creation, so it rarely happens, but the probability
can be increased if the loaded library has a lot of dependencies with
tls, and if the deps don't fit into the same listp->slotinfo array then
the second assert failure starts to happen.

The current sequence of events is:

dlopen:

   The modids of the loaded library and its dependencies are set one by
   one to ++GL(dl_tls_max_dtv_idx) (assuming they have tls).

   Then these modules are added to GL(dl_tls_dtv_slotinfo_list) one by
   one with the same generation number: GL(dl_tls_generation)+1.

   Then the GL(dl_tls_generation) is increased.

pthread_create:

   The dtv for the thread is allocated assuming GL(dl_tls_max_dtv_idx) is
   the max modid that will be added to the dtv here.

   The GL(dl_tls_dtv_slotinfo_list) is walked to initialize dtv[modid]
   for all modules in the list and initialize the related tls.

   At the end dtv[0].counter is set to the max generation number seen,
   all modules with less-than-or-equal number must have initialized dtv
   and tls in this thread.

The patch uses atomics to add modules to the slotinfo list and to increase
GL(dl_tls_generation) during dlopen and similarly uses atomics to safely
walk the slotinfo list in pthread_create.  The dtv and tls are initialized
for all modules up to the observed GL(dl_tls_generation), modules with
larger generation number (concurrently loaded modules) are ignored.


Further issues:

dlclose also modifies the slotinfo list in unsafe ways and i don't
immediately see a lockfree way to synchronize that with thread
creation. (using the GL(dl_load_lock) at thread creation does not work
because it can deadlock if pthread_create is called from a ctor while
dlopen holds the lock.)
i.e. this patch assumes dlclose is not called concurrently with
pthread_create.

Two (incorrect) assertions were removed from the code and i don't
see an easy way to do similar runtime checks to guard against similar
races or memory corruptions.

I did not review all accesses to the problematic globals there are most
likely other problems (e.g. GL(dl_tls_max_dtv_idx) is modified without
atomics, tls access (__tls_get_addr) does not use atomics to access the
same globals, etc)

Glibc does not try to do worst-case allocation of tls for existing
threads whenever a library is loaded so allocation might be needed at
the first access to tls objects, making it non-as-safe and possibly oom
crash.  The dtv and tls of the modules that are ignored in this patch
will be lazy initialized.


Changelog:

2016-01-11  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	[BZ #19329]
	* elf/dl-open.c (dl_open_worker): Write GL(dl_tls_generation)
	atomically.
	* elf/dl-tls.c (_dl_allocate_tls_init): Read GL(dl_tls_generation),
	GL(dl_tls_max_dtv_idx), slotinfo entries and listp->next atomically.
	(_dl_add_to_slotinfo): Write the slotinfo entries and listp->next
	atomically.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dtv2.diff
Type: text/x-patch
Size: 3946 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20160111/70894317/attachment.bin>