Re: [PATCH v3] elf: Fix slow tls access after dlopen [BZ #19924]

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 29/08/23 11:09, Szabolcs Nagy wrote:
> The 08/23/2023 16:06, Adhemerval Zanella Netto wrote:
>> The fix sound reasonable, and I did not see any regression on some different
>> architectures (aarch64, powerpc, sparc, x86).  However I am not sure how
>> compreensible our own regression tests are for this issue, since afaik
>> there is no concurrent tests that stress concurrent dlopen module and
>> TLS access.  It also does fix the performance issue from bug report.
>>
>> Below some minor comments.
> ...
>>> --- a/elf/dl-reloc.c
>>> +++ b/elf/dl-reloc.c
>>> @@ -114,9 +114,7 @@ _dl_try_allocate_static_tls (struct link_map *map, bool optional)
>>>  #ifdef SHARED
>>>        if (__builtin_expect (THREAD_DTV()[0].counter != GL(dl_tls_generation),
>>>  			    0))
>>> -	/* Update the slot information data for at least the generation of
>>> -	   the DSO we are allocating data for.  */
>>> -	(void) _dl_update_slotinfo (map->l_tls_modid);
>>> +	(void) _dl_update_slotinfo (map->l_tls_modid, GL(dl_tls_generation));
>>
>> Shouldn't it use atomic_load_relaxed on GL(dl_tls_generation) for consistency?
> 
> _dl_try_allocate_static_tls is called during tlsdesc reloc
> processing.
> 
> relocation processing always runs with the GL(dl_load_lock)
> held and then no concurrent write to the gen count is possible.
> 
> loads outside the lock have to use atomics to sync with the
> atomic store under the lock, loads under the lock can be
> normal loads.

Fair enough, maybe add a small comment with this very explanation.

> 
>>>  struct link_map *
>>> -_dl_update_slotinfo (unsigned long int req_modid)
>>> +_dl_update_slotinfo (unsigned long int req_modid, size_t new_gen)
>>>  {
>>>    struct link_map *the_map = NULL;
>>>    dtv_t *dtv = THREAD_DTV ();
>>>  
>>> -  /* The global dl_tls_dtv_slotinfo array contains for each module
>>> -     index the generation counter current when the entry was created.
>>> +  /* CONCURRENCY NOTES:
>>> +
>>> +     The global dl_tls_dtv_slotinfo array contains for each module
>>
>> Maybe use the correct name, dl_tls_dtv_slotinfo_list, so it is easier
>> to reference on the code.
> 
> ok. (it was this way in the orig comment, but using the full
> identifier is probably better)
> 
>>
>>> +     index the generation counter current when that entry was updated.
>>>       This array never shrinks so that all module indices which were
>>
>> I think it should 'that' here (it specifies the module indices and there
>> is no comma).
> 
> what do you mean?

[...] so that all module indices which were [...].  But I just realized this
is not from patch, so fell free to ignore it.

> 
>>
>>> -     valid at some time can be used to access it.  Before the first
>>> -     use of a new module index in this function the array was extended
>>> -     appropriately.  Access also does not have to be guarded against
>>> -     modifications of the array.  It is assumed that pointer-size
>>> -     values can be read atomically even in SMP environments.  It is
>>> -     possible that other threads at the same time dynamically load
>>> -     code and therefore add to the slotinfo list.  This is a problem
>>> -     since we must not pick up any information about incomplete work.
>>> -     The solution to this is to ignore all dtv slots which were
>>> -     created after the one we are currently interested.  We know that
>>> -     dynamic loading for this module is completed and this is the last
>>> -     load operation we know finished.  */
>>> -  unsigned long int idx = req_modid;
>>> +     valid at some time can be used to access it.  Concurrent loading
>>> +     and unloading of modules can update slotinfo entries or extend
>>> +     the array.  The updates happen under the GL(dl_load_tls_lock) and
>>> +     finish with the release store of the generation counter to
>>> +     GL(dl_tls_generation) which is synchronized with the load of
>>> +     new_gen in the caller.  So updates up to new_gen are synchronized
>>> +     but updates for later generations may not be.
>>> +
>>> +     Here we update the thread dtv from old_gen (== dtv[0].counter) to
>>> +     new_gen generation.  For this each dtv[i] entry is either set to
>>
>> Maybe a comma after 'For this'.
> 
> ok
> 
>>> +     an unallocated state (set), or left unmodified (nop).  Where (set)
>>> +     may resize the dtv first if modid i >= dtv[-1].counter. The rules
>>> +     for the decision between (set) and (nop) are
>>> +
>>> +     (1) If slotinfo entry i is concurrently updated then either (set)
>>> +         or (nop) is valid: TLS access cannot use dtv[i] unless it is
>>> +         synchronized with a generation > new_gen.
>>> +
>>> +     Otherwise if the generation of slotinfo entry i is gen and the
>>
>> Maybe a comma after 'Otherwise'.
> 
> ok