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

[Szabolcs Nagy]

On 13/01/16 02:34, Carlos O'Donell wrote:
> On 01/11/2016 11:32 AM, Szabolcs Nagy wrote:
>> 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.
>
> You are headed in the right direction. I like where this patch is going,
> but don't have enough time to review this in detail yet.
>
> At first glance your patch lacks sufficient concurrency documentation to
> be acceptable. You need to document which acquires the releases
> synchronizes-with. Please grep for "CONCURRENCY NOTES" to see the level
> of detail we need to maintain these kinds of changes.

Patch v3:

I added some concurrency notes.  There are some TODO items left in
the comments.

I decided not to change __tls_get_addr and dlclose code paths, this
patch only changes comments compared to v2.

Overall the patch only turns some load/stores into atomic and
changes their order (but not in a way that can be observed without
concurrently running pthread_create and dlopen).  Two branches
are changed: if slotinfo[i].gen >  GL(dl_tls_generation) then that
entry is skipped instead of abort, and if listp->next == NULL then
the iteration is finished instead of abort.

These changes might not be entirely safe: dlclose is still not
synced with pthread_create and there might be other bugs that can
cause large slotinfo[i].gen or early list->next==NULL, then the
current glibc code would abort and the new code will continue
without noticing this.

Note that the current asserts do not cover the concurrency bugs
completely: concurrent dlclose can (even with the patch) lead to
use after free, concurrently added modules can lead to read of
uninitialized slotinfo entry where map is already non-null, but
gen is 0 (and thus smaller than the current generation count)
which can lead to out-of-bounds write to the DTV or other memory
corruption.  (If the asserts were more helpfully protecting against
memory corruption, then i'd be reluctant removing them before a
complete fix for the concurrency issues.)

The acquire load of every slotinfo[i].map might be excessive use
of atomics in _dl_allocate_tls_init, but i think it is necessary
with the current design.

I'm OK if this gets rescheduled after 2.23.

2016-01-19  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: dtv3.diff
Type: text/x-patch
Size: 6522 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20160119/a2efbddf/attachment.bin>