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

["Carlos O'Donell" <carlos at redhat dot com>]

On 01/14/2016 10:09 AM, Szabolcs Nagy wrote:
> 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.
>>
> 
> will there be time before 2.23?

I don't think there is time to review this sufficiently to get it into
2.23, 2.24 yes, but not 2.23.

Even then I will try to do a round of review, and I've asked Torvald if
he has any spare cycles to help review this.

>> 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.
>>
> 
> i wanted to avoid documenting all the mess in the dynamic linker,
> but i can improve the comments.

You must document *some* of the mess, and you must minimally document
everything you change in order to provide future documentation for
subsequent reviews of the concurrency code.

> i see the following bugs:
> 
> 1) pthread_create is not synced with dlopen
> 2) pthread_create is not synced with dlclose
> 3) tls access is not synced with dlopen
> 4) tls access is not synced with dlclose
> 5) tls access can oom crash
> 6) tls access is not as-safe
> 7) dlopen holds a global lock during ctors
> 
> i can fix 1) by adding some atomics (this is the current patch).
> 
> for 2) that's not enough, because dlclose has to wait with the
> free(link_map) while pthread_create is initializing the tls.
> 
> i guess 3) can be fixed similarly to 1) but i don't have a
> test case for that.
> 
> the rest is harder to fix.
> 
> is it ok to only fix 1) for 2.23?
> or should i try to fix 3) as well (races on the same globals)?

No no. We want to minimize the changes going into 2.23.

We should aim to fix only (1) right now if that is what you are
interested in fixing.

Everything we do should be incremental. At each step though we
might ask ourselves: How do we solve the more global problem
of getting rid of the load lock?

Do we all agree that the better future would be an atomic process
that can add loaded libraries to the link map list without having
to take a lock?

Cheers,
Carlos.