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

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

On 09/07/2021 12:05, Szabolcs Nagy wrote:
> The 07/09/2021 10:50, Adhemerval Zanella wrote:
>> Changes from previous version:
>>
>>   - Fix commit message and add a line about the bug fixes.
>>   - Use atomic operation while setting the slotinfo.
>>   - Use test_verbose on tst-tls20.c.
>>
>> ---
>>
>> This is updated version of the 572bd547d57a (reverted by 40ebfd016ad2)
>> that fixes the _dl_next_tls_modid issues.
>>
>> This issue with 572bd547d57a patch is the DTV entry will be only
>> update on dl_open_worker() with the update_tls_slotinfo() call after
>> all dependencies are being processed by _dl_map_object_deps().  However
>> _dl_map_object_deps() itself might call _dl_next_tls_modid(), and since
>> the _dl_tls_dtv_slotinfo_list::map is not yet set the entry will be
>> wrongly reused.
>>
>> This patch fixes by renaming the _dl_next_tls_modid() function to
>> _dl_assign_tls_modid() and by passing the link_map so it can set
>> the slotinfo value so a so subsequente _dl_next_tls_modid() call will
>> see the entry as allocated.
> 
> this paragraph still has 'so a so subsequente'
> and i would add the bug number into the first sentence.

Fixed.

> 
>>
>> The intermediary value is cleared up on remove_slotinfo() for the case
>> a library fails to load with RTLD_NOW.
>>
>> This patch fixes BZ #27135.
>>
>> Checked on x86_64-linux-gnu.
> 
> the patch looks ok to me, with the commit message
> and the comment issue below fixed.
> 
> Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>

Carlos, is it for push?

> 
>> +
>> +/* The following test check DTV gaps handling with shared libraries that has
>> +   dependencies.  It defines 5 different sets:
>> +
>> +   1. Single dependency:
>> +      mod0 -> mod1
>> +   2. Double dependency:
>> +      mod2 -> [mod3,mod4]
>> +   3. Double dependency with each dependency depent of another module:
>> +      mod5 -> [mod6,mod7] -> mod8
>> +   4. Long chain with one double dependency in the middle:
>> +      mod9 -> [mod10, mod11] -> mod12 -> mod13
>> +   5. Long chain with two double depedencies in the middle:
>> +      mod15 -> mod15 -> [mod16, mod17]
>> +      mod15 -> [mod18, mod19]
> 
> mod14 -> ...

Fixed.

> 
>> +
>> +   This does not cover all the possible gaps and configuration, but it
>> +   should check if different dynamic shared sets are placed correctly in
>> +   different gaps configurations.  */
>> +
>> +static int
>> +nmodules (uint32_t v)
>> +{
>> +  unsigned int r = 0;
>> +  while (v >>= 1)
>> +    r++;
>> +  return r + 1;
>> +}
>> +
>> +static inline bool
>> +is_mod_set (uint32_t g, uint32_t n)
>> +{
>> +  return (1U << (n - 1)) & g;
>> +}
>> +
>> +static void
>> +print_gap (uint32_t g)
>> +{
>> +  if (!test_verbose)
>> +    return;
>> +  printf ("gap: ");
>> +  int nmods = nmodules (g);
>> +  for (int n = 1; n <= nmods; n++)
>> +    printf ("%c", ((1 << (n - 1)) & g) == 0 ? 'G' : 'M');
>> +  printf ("\n");
>> +}
>> +
>> +static void
>> +do_test_dependency (void)
>> +{
>> +  /* Maps the module and its dependencies, use thread to access the TLS on
>> +     each loaded module.  */
>> +  static const int tlsmanydeps0[] = { 1 };
>> +  static const int tlsmanydeps1[] = { 3, 4 };
>> +  static const int tlsmanydeps2[] = { 6, 7, 8 };
>> +  static const int tlsmanydeps3[] = { 10, 11, 12 };
>> +  static const int tlsmanydeps4[] = { 15, 16, 17, 18, 19 };
>> +  static const struct tlsmanydeps_t
>> +  {
>> +    int modi;
>> +    int ndeps;
>> +    const int *deps;
>> +  } tlsmanydeps[] =
>> +  {
>> +    {  0, array_length (tlsmanydeps0), tlsmanydeps0 },
>> +    {  2, array_length (tlsmanydeps1), tlsmanydeps1 },
>> +    {  5, array_length (tlsmanydeps2), tlsmanydeps2 },
>> +    {  9, array_length (tlsmanydeps3), tlsmanydeps3 },
>> +    { 14, array_length (tlsmanydeps4), tlsmanydeps4 },
>> +  };