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

[Florian Weimer]

* 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)

>> +/* 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.

>> +{
>> +  /* 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.

>> +      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;

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");
+  {
+    /* 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.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill