Re: [PATCH] elf: Fix DTV gap reuse logic (BZ #27135)

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

On 08/07/2021 10:04, Szabolcs Nagy wrote:
> The 07/08/2021 09:20, Adhemerval Zanella wrote:
>>
>>
>> On 08/07/2021 08:00, Szabolcs Nagy wrote:
>>> The 07/08/2021 08:00, Florian Weimer wrote:
>>>> * Adhemerval Zanella:
>>>>> diff --git a/elf/dl-tls.c b/elf/dl-tls.c
>>>>> index 2b5161d10a..4ec4c7f38c 100644
>>>>> --- a/elf/dl-tls.c
>>>>> +++ b/elf/dl-tls.c
>>>>> @@ -157,7 +157,11 @@ _dl_next_tls_modid (void)
>>>>>  	      }
>>>>>  
>>>>>  	    if (result - disp < runp->len)
>>>>> -	      break;
>>>>> +	      {
>>>>> +		/* Mark the entry as used, so any dependency see it.  */
>>>>> +		runp->slotinfo[result - disp].map = (struct link_map *) -1;
>>>>> +		break;
>>>>> +	      }
>>>>>  
>>>>>  	    disp += runp->len;
>>>>>  	  }
>>>>
>>>> Which dependency?
>>
>> Any shared library dependency being loaded after this.
>>
>>>>
>>>> I think the special value -1 needs to be documented on the slotinfo
>>>> struct member.
>>>>
>>>> When Carlos and I discussed this, we couldn't quite decide whether it
>>>> was appropriate just to assign the actual link map value here.
>>>
>>> slotinfo[].map is only accessed without locks during
>>> _dl_update_slotinfo at tls access and then it is
>>> only used to get the map for the module in which
>>> tls is accessed. (and to skip NULL maps when resizing
>>> dtv but that's just a minor optimization).
>>>
>>> so the only concurrent .map access does not really
>>> care about its value for an incomplete module.
>>>
>>> so we only need to reason locally what happens within
>>> one dlopen call to decide what can be put temporarily
>>> in .map and the final value only has to be stored
>>> before the lock is released. (with bug 19924 fixed
>>> the final .map and .gen values likely have to be
>>> stored a bit earlier: before the global gen count is
>>> updated)
>>>
>>> the tricky bit is to check if the slotinfo state is
>>> rolled back right in case of dlopen failures. but it
>>> seems to me that modid assignment happens after the
>>> point where failures are handled via dlclose which
>>> will fixup the slotinfo state (i.e. after unlock
>>> unused slotinfo entries are marked with .map == NULL).
>>
>> That is my understanding as well and I tried to check if the
>> rollback is correctly done by adding more cases on the
>> tst-tls20.c. 
>>
>>>
>>> so i think _dl_next_tls_modid() can be changed to
>>> _dl_assign_tls_modid(map) which writes map to the
>>> slotinfo .map.
>> What do you mean by '_dl_assign_tls_modid()' in this case ?
> 
> currently the only use is
> 
>  map->l_tls_modid = _dl_next_tls_modid ();
> 
> which can be changed to
> 
>  _dl_assign_tls_modid (map);
> 
> which sets map->l_tls_modid as well as slotinfo[i].map = map
> marking the slotinfo entry non-free (with relaxed atomic
> store since it can be read concurrently).
> 
> i don't know if 'assign' is the right word, but the point is
> that we don't just get the next modid, but allocate it too
> for the given map.

I see, this should work since we just need either a sentinel value
or a valid link_map to indicate that the slot is being already
taken.  For the rollback in case of failure remove_slotinfo() should
handle it (it just check for non NULL).

I will send an updated patch.