[PATCH 09/11] elf: Add glibc-hwcaps subdirectory support to ld.so cache processing

[Adhemerval Zanella]

On 01/12/2020 17:45, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> On 09/11/2020 15:41, Florian Weimer via Libc-alpha wrote:
>>> This recognizes the DL_CACHE_HWCAP_EXTENSION flag and picks up
>>> the supported cache entry with the highest priority.
>>
>> Maybe add a brief description of what DL_CACHE_HWCAP_EXTENSION aims to
>> do?
> 
> It's documented in sysdeps/generic/dl-cache.h, added in the previous
> commit:
> 
> /* This bit in the hwcap field of struct file_entry_new indicates that
>    the lower 32 bits contain an index into the
>    cache_extension_tag_glibc_hwcaps section.  Older glibc versions do
>    not know about this HWCAP bit, so they will ignore these
>    entries.  */
> #define DL_CACHE_HWCAP_EXTENSION (1ULL << 62)

I meant to commit message, but I don't have a strong opinion about it.

> 
>>> +/* Return the priority of the cache_extension_tag_glibc_hwcaps section
>>> +   entry at INDEX.  Zero means do not use.  Otherwise, lower values
>>> +   indicate greater preference.  */
>>> +static uint32_t __attribute__ ((noinline, noclone))
>>> +glibc_hwcaps_priority (uint32_t index)
>>
>> I have a feeling I have asked it before, but why does it need noclone/noinline
>> here?
> 
> I don't recall such a discussion.  It's a leftover from previous
> debugging efforts.

Ack.

> 
>>> +{
>>> +  /* Using a zero-length array as an indicator that nothing has been
>>> +     loaded is not a problem: It does not lead to repeated
>>> +     initialization attempts because caches without an extension
>>> +     section are processed without calling this function (unless the
>>> +     file is corrupted).  */
>>> +  if (glibc_hwcaps_priorities_length == 0)
>>
>> Maybe an early exit here? It allow move the code one identation left
>> and makes it more readable.
> 
> I've moved this into a separate initialization function.

Ack.

> 
>>> +      while (left < left_end && right < right_end)
>>> +	{
>>> +	  uint32_t string_table_index = *left;
>>> +	  if (string_table_index < cachesize)
>>> +	    {
>>> +	      const char *left_name
>>> +		= (const char *) cache + string_table_index;
>>> +	      uint32_t left_name_length = strlen (left_name);
>>> +	      uint32_t to_compare;
>>> +	      if (left_name_length < right->name_length)
>>> +		to_compare = left_name_length;
>>> +	      else
>>> +		to_compare = right->name_length;
>>> +	      int cmp = memcmp (left_name, right->name, to_compare);
>>> +	      if (cmp == 0)
>>> +		{
>>> +		  if (left_name_length < right->name_length)
>>> +		    cmp = -1;
>>> +		  else if (left_name_length > right->name_length)
>>> +		    cmp = 1;
>>> +		}
>>> +	      if (cmp == 0)
>>> +		{
>>> +		  *result = right->priority;
>>> +		  ++result;
>>> +		  ++left;
>>> +		  ++right;
>>> +		}
>>
>> Maybe add as the 'else' within the 'cmp == 0' below?
>>
>>> +	      else if (cmp < 0)
>>> +		{
>>> +		  *result = 0;
>>> +		  ++result;
>>> +		  ++left;
>>> +		}
>>> +	      else
>>> +		++right;
> 
> You mean like this?
> 
> 	  if (cmp == 0)
> 	    {
> 	      *result = right->priority;
> 	      ++result;
> 	      ++left;
> 	    }
> 	  if (cmp < 0)
> 	    {
> 	      *result = 0;
> 	      ++result;
> 	      ++left;
> 	    }
>           if (cmp >= 0)
> 	    ++right;

The v5 seems more readable in fact.


> 
> I don't think that's an improvement.
> 
>>> +struct dl_hwcaps_priority *_dl_hwcaps_priorities;
>>> +uint32_t _dl_hwcaps_priorities_length;
>>> +
>>> +/* Allocate _dl_hwcaps_priorities and fill it with data.  */
>>> +static void
>>> +compute_priorities (size_t total_count, const char *prepend,
>>> +		    int32_t bitmask, const char *mask)
>>
>> I think bitmask should be a 'uint32' here.
> 
> Right, fixed.
> 
>>> +int
>>> +main (int argc, char **argv)
>>> +{
>>> +  /* Run ldconfig to populate the cache.  */
>>> +  {
>>> +    char *command = xasprintf ("%s/ldconfig", support_install_rootsbindir);
>>> +    if (system (command) != 0)
>>> +      return 1;
>>> +    free (command);
>>> +  }
>>> +
>>> +  /* Reuse tst-glibc-hwcaps.  Since this code is running in a
>>> +     container, we can launch it directly.  */
>>> +  char *path = xasprintf ("%s/elf/tst-glibc-hwcaps", support_objdir_root);
>>> +  execv (path, argv);
>>> +  printf ("error: execv of %s failed: %m\n", path);
>>> +  return 1;
>>> +}
>>
>> Ok, it should be simple enough to require use libsupport fork/timeout
>> check.
> 
> Yes, we run ldconfig without timeout elsewhere, and tst-glibc-hwcaps has
> its own timeout check.
> 
>> Should we test the case of shared memory removal, for instance:
>>
>>   1. Remove second override, without ldconfig
>>   2. Remove third override, run ldconfig
>>   3. Add both second and third back, run ldconfig, remove third
>>   4. Keep second, run ldconfig
> 
> I added this:
> 
> +  /* Remove the second override again, without running ldconfig.
> +     Ideally, this would revert to implementation 2.  However, in the
> +     current implementation, the cache returns exactly one file name
> +     which does not exist after unlinking, so the dlopen fails.  */
> +  xunlink ("/glibc-test/lib/glibc-hwcaps/prepend3/" SONAME);
> +  TEST_VERIFY (dlopen (SONAME, RTLD_NOW) == NULL);
> +  run_ldconfig ();
> +  system("/sbin/ldconfig -p");

I think this might a leftover of debugging.

> +  {
> +    /* After running ldconfig, the second implementation is available
> +       once more.  */
> +    void *handle = xdlopen (SONAME, RTLD_NOW);
> +    int (*marker1) (void) = xdlsym (handle, "marker1");
> +    TEST_COMPARE (marker1 (), 2);
> +    xdlclose (handle);
> +  }
> 
> I think I know what I have to do to fix this in elf/dl-load.c.
> 
> I feel this is a pre-existing issue.  It also applies to the legacy
> hwcaps subdirectories.  This only happens for ld.so.cache bringing in
> libraries from a non-searched directory, otherwise the LD_LIBRARY_PATH
> processing will find an implementation if it exists.  I'd like to fix it
> in a follow-up patch.

Fair enough.