[PATCH 2/3] Add system-wide tunables: cache ld.so.cache

[PATCH 2/3] Add system-wide tunables: cache ld.so.cache

[DJ Delorie]

The purpose of this change is twofold:

1. The ld.so.cache is cached in memory and only re-read if/when
   it changes on disk.  This allows us to have much more intensive
   security checks in the future, without impacting performance as
   much.  It also allows for cases where the cache is corrupted -
   we continue using the last valid one.

2. We break out the load/check logic so that the cache can be
   loaded independently of the library lookup, such as for
   code that only needs to look at the extensions.
---
 elf/dl-cache.c | 252 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 167 insertions(+), 85 deletions(-)

diff --git a/elf/dl-cache.c b/elf/dl-cache.c
index 85f3f179ed..d90278889d 100644
--- a/elf/dl-cache.c
+++ b/elf/dl-cache.c
@@ -26,6 +26,8 @@
 #include <_itoa.h>
 #include <dl-hwcaps.h>
 #include <dl-isa-level.h>
+#include <sys/types.h>
+#include <sys/stat.h>
 
 #ifndef _DL_PLATFORMS_COUNT
 # define _DL_PLATFORMS_COUNT 0
@@ -35,6 +37,10 @@
 static struct cache_file *cache;
 static struct cache_file_new *cache_new;
 static size_t cachesize;
+static struct cache_extension_all_loaded ext;
+
+static struct stat cache_file_time;
+static struct stat new_cache_file_time;
 
 #ifdef SHARED
 /* This is used to cache the priorities of glibc-hwcaps
@@ -57,6 +63,7 @@ glibc_hwcaps_priorities_free (void)
     free (glibc_hwcaps_priorities);
   glibc_hwcaps_priorities = NULL;
   glibc_hwcaps_priorities_allocated = 0;
+  glibc_hwcaps_priorities_length = 0;
 }
 
 /* Ordered comparison of a hwcaps string from the cache on the left
@@ -88,10 +95,6 @@ glibc_hwcaps_compare (uint32_t left_index, struct dl_hwcaps_priority *right)
 static void
 glibc_hwcaps_priorities_init (void)
 {
-  struct cache_extension_all_loaded ext;
-  if (!cache_extension_load (cache_new, cache, cachesize, &ext))
-    return;
-
   uint32_t length = (ext.sections[cache_extension_tag_glibc_hwcaps].size
 		     / sizeof (uint32_t));
   if (length > glibc_hwcaps_priorities_allocated)
@@ -390,6 +393,159 @@ _dl_cache_libcmp (const char *p1, const char *p2)
   return *p1 - *p2;
 }
 
+/* Set the cache back to the "no cache" state, which may include
+   cleaning up a loaded cache.  */
+static void
+_dl_maybe_unload_ldsocache (void)
+{
+  if (cache != NULL)
+    __munmap (cache, cachesize);
+
+  cache = NULL;
+  cache_new = NULL;
+  cachesize = 0;
+
+#ifdef SHARED
+  glibc_hwcaps_priorities_free ();
+#endif
+}
+
+/* Returns TRUE if for any reason the cache needs to be reloaded
+   (including, the first time, loaded).  */
+static bool
+_dl_check_ldsocache_needs_loading (void)
+{
+  int rv;
+  static int copy_old_time = 0;
+
+  /* Save the previous stat every time.  We only care when this
+     changes, and we only stat it here, so we can get away with doing
+     the copy now instead of at every single return statement in this
+     function.  However, we only need to copy it if the previous stat
+     succeeded.  The only way this could be subverted is if the admin
+     moves the file aside, then moves it back, but CACHE would be set
+     to NULL in the interim so that would be detected.  */
+  if (copy_old_time)
+    cache_file_time = new_cache_file_time;
+  rv = __stat (LD_SO_CACHE, &new_cache_file_time);
+  copy_old_time = (rv >= 0);
+
+  /* No file to load, but there used to be.  Assume user intentionally
+     deleted the cache and act accordingly.  */
+  if (rv < 0 && cache != NULL)
+    {
+      _dl_maybe_unload_ldsocache ();
+      return false;
+    }
+
+  /* No file to load and no loaded cache, so nothing to do.  */
+  if (rv < 0)
+    return false;
+
+  /* Any file is better than no file (likely the first time
+     through).  */
+  if (cache == NULL)
+    return true;
+
+  /* At this point, NEW_CACHE_FILE_TIME is valid as well as
+     CACHE_FILE_TIME, so we compare them.  We list fields in the order
+     they're most likely to be different in.  */
+  return ((new_cache_file_time.st_mtime != cache_file_time.st_mtime)
+	  || (new_cache_file_time.st_ino != cache_file_time.st_ino)
+	  || (new_cache_file_time.st_size != cache_file_time.st_size)
+	  || (new_cache_file_time.st_dev != cache_file_time.st_dev)
+	   );
+}
+
+/* Attemps to load and validate the cache.  On return, CACHE is either
+   unchanged (still loaded or still not loaded) or valid.  */
+static void
+_dl_maybe_load_ldsocache (void)
+{
+  struct cache_file *tmp_cache = NULL;
+  struct cache_file_new *tmp_cache_new = NULL;
+  size_t tmp_cachesize = 0;
+  
+  /* Read the contents of the file.  */
+  void *file = _dl_sysdep_read_whole_file (LD_SO_CACHE, &tmp_cachesize,
+					   PROT_READ);
+
+  /* We can handle three different cache file formats here:
+     - only the new format
+     - the old libc5/glibc2.0/2.1 format
+     - the old format with the new format in it
+     The following checks if the cache contains any of these formats.  */
+  if (file != MAP_FAILED && tmp_cachesize > sizeof *cache_new
+      && memcmp (file, CACHEMAGIC_VERSION_NEW,
+		 sizeof CACHEMAGIC_VERSION_NEW - 1) == 0
+      /* Check for corruption, avoiding overflow.  */
+      && ((tmp_cachesize - sizeof *cache_new) / sizeof (struct file_entry_new)
+	  >= ((struct cache_file_new *) file)->nlibs))
+    {
+      if (! cache_file_new_matches_endian (file))
+	{
+	  __munmap (file, tmp_cachesize);
+	  return;
+	}
+
+      tmp_cache_new = file;
+      tmp_cache = file;
+    }
+  else if (file != MAP_FAILED && cachesize > sizeof *cache
+	   && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0
+	   /* Check for corruption, avoiding overflow.  */
+	   && ((tmp_cachesize - sizeof *cache) / sizeof (struct file_entry)
+	       >= ((struct cache_file *) file)->nlibs))
+    {
+      size_t offset;
+      /* Looks ok.  */
+      tmp_cache = file;
+
+      /* Check for new version.  */
+      offset = ALIGN_CACHE (sizeof (struct cache_file)
+			    + cache->nlibs * sizeof (struct file_entry));
+
+      tmp_cache_new = (struct cache_file_new *) ((void *) tmp_cache + offset);
+      if (tmp_cachesize < (offset + sizeof (struct cache_file_new))
+	  || memcmp (tmp_cache_new->magic, CACHEMAGIC_VERSION_NEW,
+		     sizeof CACHEMAGIC_VERSION_NEW - 1) != 0)
+	tmp_cache_new = NULL;
+      else
+	{
+	  if (! cache_file_new_matches_endian (tmp_cache_new))
+	    /* The old-format part of the cache is bogus as well
+	       if the endianness does not match.  (But it is
+	       unclear how the new header can be located if the
+	       endianness does not match.)  */
+	    {
+	      __munmap (file, tmp_cachesize);
+	      return;
+	    }
+	}
+    }
+  else
+    {
+      if (file != MAP_FAILED)
+	__munmap (file, tmp_cachesize);
+      return;
+    }
+
+  struct cache_extension_all_loaded tmp_ext;
+  if (!cache_extension_load (tmp_cache_new, tmp_cache, tmp_cachesize, &tmp_ext))
+    /* The extension is corrupt, so the cache is corrupt.  */
+    return;
+
+  /* If we've gotten here, the loaded cache is good and we need to
+     save it.  */
+  _dl_maybe_unload_ldsocache ();
+  cache = tmp_cache;
+  cache_new = tmp_cache_new;
+  cachesize = tmp_cachesize;
+  ext = tmp_ext;
+
+  assert (cache != NULL);
+}
+
 
 /* Look up NAME in ld.so.cache and return the file name stored there, or null
    if none is found.  The cache is loaded if it was not already.  If loading
@@ -405,81 +561,14 @@ _dl_load_cache_lookup (const char *name)
   if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_LIBS))
     _dl_debug_printf (" search cache=%s\n", LD_SO_CACHE);
 
-  if (cache == NULL)
-    {
-      /* Read the contents of the file.  */
-      void *file = _dl_sysdep_read_whole_file (LD_SO_CACHE, &cachesize,
-					       PROT_READ);
-
-      /* We can handle three different cache file formats here:
-	 - only the new format
-	 - the old libc5/glibc2.0/2.1 format
-	 - the old format with the new format in it
-	 The following checks if the cache contains any of these formats.  */
-      if (file != MAP_FAILED && cachesize > sizeof *cache_new
-	  && memcmp (file, CACHEMAGIC_VERSION_NEW,
-		     sizeof CACHEMAGIC_VERSION_NEW - 1) == 0
-	  /* Check for corruption, avoiding overflow.  */
-	  && ((cachesize - sizeof *cache_new) / sizeof (struct file_entry_new)
-	      >= ((struct cache_file_new *) file)->nlibs))
-	{
-	  if (! cache_file_new_matches_endian (file))
-	    {
-	      __munmap (file, cachesize);
-	      file = (void *) -1;
-	    }
-	  cache_new = file;
-	  cache = file;
-	}
-      else if (file != MAP_FAILED && cachesize > sizeof *cache
-	       && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0
-	       /* Check for corruption, avoiding overflow.  */
-	       && ((cachesize - sizeof *cache) / sizeof (struct file_entry)
-		   >= ((struct cache_file *) file)->nlibs))
-	{
-	  size_t offset;
-	  /* Looks ok.  */
-	  cache = file;
-
-	  /* Check for new version.  */
-	  offset = ALIGN_CACHE (sizeof (struct cache_file)
-				+ cache->nlibs * sizeof (struct file_entry));
-
-	  cache_new = (struct cache_file_new *) ((void *) cache + offset);
-	  if (cachesize < (offset + sizeof (struct cache_file_new))
-	      || memcmp (cache_new->magic, CACHEMAGIC_VERSION_NEW,
-			 sizeof CACHEMAGIC_VERSION_NEW - 1) != 0)
-	      cache_new = (void *) -1;
-	  else
-	    {
-	      if (! cache_file_new_matches_endian (cache_new))
-		{
-		  /* The old-format part of the cache is bogus as well
-		     if the endianness does not match.  (But it is
-		     unclear how the new header can be located if the
-		     endianness does not match.)  */
-		  cache = (void *) -1;
-		  cache_new = (void *) -1;
-		  __munmap (file, cachesize);
-		}
-	    }
-	}
-      else
-	{
-	  if (file != MAP_FAILED)
-	    __munmap (file, cachesize);
-	  cache = (void *) -1;
-	}
+  if (_dl_check_ldsocache_needs_loading ())
+    _dl_maybe_load_ldsocache ();
 
-      assert (cache != NULL);
-    }
-
-  if (cache == (void *) -1)
-    /* Previously looked for the cache file and didn't find it.  */
+  if (cache == NULL)
     return NULL;
 
   const char *best;
-  if (cache_new != (void *) -1)
+  if (cache_new != NULL)
     {
       const char *string_table = (const char *) cache_new;
       best = search_cache (string_table, cachesize,
@@ -505,7 +594,7 @@ _dl_load_cache_lookup (const char *name)
     return NULL;
 
   /* The double copy is *required* since malloc may be interposed
-     and call dlopen itself whose completion would unmap the data
+     and call dlopen itself whose completion may unmap the data
      we are accessing. Therefore we must make the copy of the
      mapping data without using malloc.  */
   char *temp;
@@ -523,14 +612,7 @@ _dl_load_cache_lookup (const char *name)
 void
 _dl_unload_cache (void)
 {
-  if (cache != NULL && cache != (struct cache_file *) -1)
-    {
-      __munmap (cache, cachesize);
-      cache = NULL;
-    }
-#ifdef SHARED
-  /* This marks the glibc_hwcaps_priorities array as out-of-date.  */
-  glibc_hwcaps_priorities_length = 0;
-#endif
+  /* Functionality is no longer needed, but kept for internal ABI for
+     now.  */
 }
 #endif
-- 
2.39.3

Re: [PATCH 2/3] Add system-wide tunables: cache ld.so.cache

[Adhemerval Zanella Netto]

On 19/04/24 00:49, DJ Delorie wrote:
> 
> The purpose of this change is twofold:
> 
> 1. The ld.so.cache is cached in memory and only re-read if/when
>    it changes on disk.  This allows us to have much more intensive
>    security checks in the future, without impacting performance as
>    much.  It also allows for cases where the cache is corrupted -
>    we continue using the last valid one.
> 
> 2. We break out the load/check logic so that the cache can be
>    loaded independently of the library lookup, such as for
>    code that only needs to look at the extensions.

It is not clear to me why we need the load/reload support for ld.so.cache
to support system-wide tunables.  The cache should either already being 
loaded during library resolution or loaded at  startup when tunables are 
parsed, specially if the idea is to add hardening features.,

It means that  we won't have the case where the case is not loaded during
process execution (due either if libraries were already loaded by 
RUNPATH/RPATH/LD_LIBRARY_PATH or --inhibit-cache).

> ---
>  elf/dl-cache.c | 252 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 167 insertions(+), 85 deletions(-)
> 
> diff --git a/elf/dl-cache.c b/elf/dl-cache.c
> index 85f3f179ed..d90278889d 100644
> --- a/elf/dl-cache.c
> +++ b/elf/dl-cache.c
> @@ -26,6 +26,8 @@
>  #include <_itoa.h>
>  #include <dl-hwcaps.h>
>  #include <dl-isa-level.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
>  
>  #ifndef _DL_PLATFORMS_COUNT
>  # define _DL_PLATFORMS_COUNT 0
> @@ -35,6 +37,10 @@
>  static struct cache_file *cache;
>  static struct cache_file_new *cache_new;
>  static size_t cachesize;
> +static struct cache_extension_all_loaded ext;
> +
> +static struct stat cache_file_time;
> +static struct stat new_cache_file_time;
>  
>  #ifdef SHARED
>  /* This is used to cache the priorities of glibc-hwcaps
> @@ -57,6 +63,7 @@ glibc_hwcaps_priorities_free (void)
>      free (glibc_hwcaps_priorities);
>    glibc_hwcaps_priorities = NULL;
>    glibc_hwcaps_priorities_allocated = 0;
> +  glibc_hwcaps_priorities_length = 0;
>  }
>  
>  /* Ordered comparison of a hwcaps string from the cache on the left
> @@ -88,10 +95,6 @@ glibc_hwcaps_compare (uint32_t left_index, struct dl_hwcaps_priority *right)
>  static void
>  glibc_hwcaps_priorities_init (void)
>  {
> -  struct cache_extension_all_loaded ext;
> -  if (!cache_extension_load (cache_new, cache, cachesize, &ext))
> -    return;
> -
>    uint32_t length = (ext.sections[cache_extension_tag_glibc_hwcaps].size
>  		     / sizeof (uint32_t));
>    if (length > glibc_hwcaps_priorities_allocated)
> @@ -390,6 +393,159 @@ _dl_cache_libcmp (const char *p1, const char *p2)
>    return *p1 - *p2;
>  }
>  
> +/* Set the cache back to the "no cache" state, which may include
> +   cleaning up a loaded cache.  */
> +static void
> +_dl_maybe_unload_ldsocache (void)
> +{
> +  if (cache != NULL)
> +    __munmap (cache, cachesize);
> +
> +  cache = NULL;
> +  cache_new = NULL;
> +  cachesize = 0;
> +
> +#ifdef SHARED
> +  glibc_hwcaps_priorities_free ();
> +#endif
> +}
> +
> +/* Returns TRUE if for any reason the cache needs to be reloaded
> +   (including, the first time, loaded).  */
> +static bool
> +_dl_check_ldsocache_needs_loading (void)
> +{
> +  int rv;
> +  static int copy_old_time = 0;
> +
> +  /* Save the previous stat every time.  We only care when this
> +     changes, and we only stat it here, so we can get away with doing
> +     the copy now instead of at every single return statement in this
> +     function.  However, we only need to copy it if the previous stat
> +     succeeded.  The only way this could be subverted is if the admin
> +     moves the file aside, then moves it back, but CACHE would be set
> +     to NULL in the interim so that would be detected.  */
> +  if (copy_old_time)
> +    cache_file_time = new_cache_file_time;
> +  rv = __stat (LD_SO_CACHE, &new_cache_file_time);
> +  copy_old_time = (rv >= 0);
> +
> +  /* No file to load, but there used to be.  Assume user intentionally
> +     deleted the cache and act accordingly.  */
> +  if (rv < 0 && cache != NULL)
> +    {
> +      _dl_maybe_unload_ldsocache ();
> +      return false;
> +    }
> +
> +  /* No file to load and no loaded cache, so nothing to do.  */
> +  if (rv < 0)
> +    return false;
> +
> +  /* Any file is better than no file (likely the first time
> +     through).  */
> +  if (cache == NULL)
> +    return true;
> +
> +  /* At this point, NEW_CACHE_FILE_TIME is valid as well as
> +     CACHE_FILE_TIME, so we compare them.  We list fields in the order
> +     they're most likely to be different in.  */
> +  return ((new_cache_file_time.st_mtime != cache_file_time.st_mtime)
> +	  || (new_cache_file_time.st_ino != cache_file_time.st_ino)
> +	  || (new_cache_file_time.st_size != cache_file_time.st_size)
> +	  || (new_cache_file_time.st_dev != cache_file_time.st_dev)
> +	   );
> +}
> +
> +/* Attemps to load and validate the cache.  On return, CACHE is either
> +   unchanged (still loaded or still not loaded) or valid.  */
> +static void
> +_dl_maybe_load_ldsocache (void)
> +{
> +  struct cache_file *tmp_cache = NULL;
> +  struct cache_file_new *tmp_cache_new = NULL;
> +  size_t tmp_cachesize = 0;
> +  
> +  /* Read the contents of the file.  */
> +  void *file = _dl_sysdep_read_whole_file (LD_SO_CACHE, &tmp_cachesize,
> +					   PROT_READ);
> +
> +  /* We can handle three different cache file formats here:
> +     - only the new format
> +     - the old libc5/glibc2.0/2.1 format
> +     - the old format with the new format in it
> +     The following checks if the cache contains any of these formats.  */
> +  if (file != MAP_FAILED && tmp_cachesize > sizeof *cache_new
> +      && memcmp (file, CACHEMAGIC_VERSION_NEW,
> +		 sizeof CACHEMAGIC_VERSION_NEW - 1) == 0
> +      /* Check for corruption, avoiding overflow.  */
> +      && ((tmp_cachesize - sizeof *cache_new) / sizeof (struct file_entry_new)
> +	  >= ((struct cache_file_new *) file)->nlibs))
> +    {
> +      if (! cache_file_new_matches_endian (file))
> +	{
> +	  __munmap (file, tmp_cachesize);
> +	  return;
> +	}
> +
> +      tmp_cache_new = file;
> +      tmp_cache = file;
> +    }
> +  else if (file != MAP_FAILED && cachesize > sizeof *cache
> +	   && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0
> +	   /* Check for corruption, avoiding overflow.  */
> +	   && ((tmp_cachesize - sizeof *cache) / sizeof (struct file_entry)
> +	       >= ((struct cache_file *) file)->nlibs))
> +    {
> +      size_t offset;
> +      /* Looks ok.  */
> +      tmp_cache = file;
> +
> +      /* Check for new version.  */
> +      offset = ALIGN_CACHE (sizeof (struct cache_file)
> +			    + cache->nlibs * sizeof (struct file_entry));
> +
> +      tmp_cache_new = (struct cache_file_new *) ((void *) tmp_cache + offset);
> +      if (tmp_cachesize < (offset + sizeof (struct cache_file_new))
> +	  || memcmp (tmp_cache_new->magic, CACHEMAGIC_VERSION_NEW,
> +		     sizeof CACHEMAGIC_VERSION_NEW - 1) != 0)
> +	tmp_cache_new = NULL;
> +      else
> +	{
> +	  if (! cache_file_new_matches_endian (tmp_cache_new))
> +	    /* The old-format part of the cache is bogus as well
> +	       if the endianness does not match.  (But it is
> +	       unclear how the new header can be located if the
> +	       endianness does not match.)  */
> +	    {
> +	      __munmap (file, tmp_cachesize);
> +	      return;
> +	    }
> +	}
> +    }
> +  else
> +    {
> +      if (file != MAP_FAILED)
> +	__munmap (file, tmp_cachesize);
> +      return;
> +    }
> +
> +  struct cache_extension_all_loaded tmp_ext;
> +  if (!cache_extension_load (tmp_cache_new, tmp_cache, tmp_cachesize, &tmp_ext))
> +    /* The extension is corrupt, so the cache is corrupt.  */
> +    return;
> +
> +  /* If we've gotten here, the loaded cache is good and we need to
> +     save it.  */
> +  _dl_maybe_unload_ldsocache ();
> +  cache = tmp_cache;
> +  cache_new = tmp_cache_new;
> +  cachesize = tmp_cachesize;
> +  ext = tmp_ext;
> +
> +  assert (cache != NULL);
> +}
> +
>  
>  /* Look up NAME in ld.so.cache and return the file name stored there, or null
>     if none is found.  The cache is loaded if it was not already.  If loading
> @@ -405,81 +561,14 @@ _dl_load_cache_lookup (const char *name)
>    if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_LIBS))
>      _dl_debug_printf (" search cache=%s\n", LD_SO_CACHE);
>  
> -  if (cache == NULL)
> -    {
> -      /* Read the contents of the file.  */
> -      void *file = _dl_sysdep_read_whole_file (LD_SO_CACHE, &cachesize,
> -					       PROT_READ);
> -
> -      /* We can handle three different cache file formats here:
> -	 - only the new format
> -	 - the old libc5/glibc2.0/2.1 format
> -	 - the old format with the new format in it
> -	 The following checks if the cache contains any of these formats.  */
> -      if (file != MAP_FAILED && cachesize > sizeof *cache_new
> -	  && memcmp (file, CACHEMAGIC_VERSION_NEW,
> -		     sizeof CACHEMAGIC_VERSION_NEW - 1) == 0
> -	  /* Check for corruption, avoiding overflow.  */
> -	  && ((cachesize - sizeof *cache_new) / sizeof (struct file_entry_new)
> -	      >= ((struct cache_file_new *) file)->nlibs))
> -	{
> -	  if (! cache_file_new_matches_endian (file))
> -	    {
> -	      __munmap (file, cachesize);
> -	      file = (void *) -1;
> -	    }
> -	  cache_new = file;
> -	  cache = file;
> -	}
> -      else if (file != MAP_FAILED && cachesize > sizeof *cache
> -	       && memcmp (file, CACHEMAGIC, sizeof CACHEMAGIC - 1) == 0
> -	       /* Check for corruption, avoiding overflow.  */
> -	       && ((cachesize - sizeof *cache) / sizeof (struct file_entry)
> -		   >= ((struct cache_file *) file)->nlibs))
> -	{
> -	  size_t offset;
> -	  /* Looks ok.  */
> -	  cache = file;
> -
> -	  /* Check for new version.  */
> -	  offset = ALIGN_CACHE (sizeof (struct cache_file)
> -				+ cache->nlibs * sizeof (struct file_entry));
> -
> -	  cache_new = (struct cache_file_new *) ((void *) cache + offset);
> -	  if (cachesize < (offset + sizeof (struct cache_file_new))
> -	      || memcmp (cache_new->magic, CACHEMAGIC_VERSION_NEW,
> -			 sizeof CACHEMAGIC_VERSION_NEW - 1) != 0)
> -	      cache_new = (void *) -1;
> -	  else
> -	    {
> -	      if (! cache_file_new_matches_endian (cache_new))
> -		{
> -		  /* The old-format part of the cache is bogus as well
> -		     if the endianness does not match.  (But it is
> -		     unclear how the new header can be located if the
> -		     endianness does not match.)  */
> -		  cache = (void *) -1;
> -		  cache_new = (void *) -1;
> -		  __munmap (file, cachesize);
> -		}
> -	    }
> -	}
> -      else
> -	{
> -	  if (file != MAP_FAILED)
> -	    __munmap (file, cachesize);
> -	  cache = (void *) -1;
> -	}
> +  if (_dl_check_ldsocache_needs_loading ())
> +    _dl_maybe_load_ldsocache ();
>  
> -      assert (cache != NULL);
> -    }
> -
> -  if (cache == (void *) -1)
> -    /* Previously looked for the cache file and didn't find it.  */
> +  if (cache == NULL)
>      return NULL;
>  
>    const char *best;
> -  if (cache_new != (void *) -1)
> +  if (cache_new != NULL)
>      {
>        const char *string_table = (const char *) cache_new;
>        best = search_cache (string_table, cachesize,
> @@ -505,7 +594,7 @@ _dl_load_cache_lookup (const char *name)
>      return NULL;
>  
>    /* The double copy is *required* since malloc may be interposed
> -     and call dlopen itself whose completion would unmap the data
> +     and call dlopen itself whose completion may unmap the data
>       we are accessing. Therefore we must make the copy of the
>       mapping data without using malloc.  */
>    char *temp;
> @@ -523,14 +612,7 @@ _dl_load_cache_lookup (const char *name)
>  void
>  _dl_unload_cache (void)
>  {
> -  if (cache != NULL && cache != (struct cache_file *) -1)
> -    {
> -      __munmap (cache, cachesize);
> -      cache = NULL;
> -    }
> -#ifdef SHARED
> -  /* This marks the glibc_hwcaps_priorities array as out-of-date.  */
> -  glibc_hwcaps_priorities_length = 0;
> -#endif
> +  /* Functionality is no longer needed, but kept for internal ABI for
> +     now.  */
>  }
>  #endif

Re: [PATCH 2/3] Add system-wide tunables: cache ld.so.cache

[DJ Delorie]

Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:
> It is not clear to me why we need the load/reload support for ld.so.cache
> to support system-wide tunables.

It's kinda independent.  Carlos requested it to benefit long-running
processes that call dlopen() regularly, to get updates to search paths.
The tunables code takes advantage of the refactoring to make sure the
cache is loaded when needed, since tunables happens before DSOs.

Re: [PATCH 2/3] Add system-wide tunables: cache ld.so.cache

[Adhemerval Zanella Netto]

On 29/05/24 14:15, DJ Delorie wrote:
> Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:
>> It is not clear to me why we need the load/reload support for ld.so.cache
>> to support system-wide tunables.
> 
> It's kinda independent.  Carlos requested it to benefit long-running
> processes that call dlopen() regularly, to get updates to search paths.
> The tunables code takes advantage of the refactoring to make sure the
> cache is loaded when needed, since tunables happens before DSOs.

But currently we do not catch ld.so.cache, it is mmap'ed on process start
and unmmap'ed at the end of DT_NEEDED process (elf/rtld.c:2402).  The same
process is done on dlopen (elf/dl-open.c:909), so any update done by ldconfig
will be seem on any subsequent dlopen.

There is still a small window where either a new process is starting of dlopen
is accessing the ld.so.cache contents and ldconfig is doing the final rename 
of a new cache, but trying to synchronize it would require a more complex
way (most likely either by making the ld.so.cache private or mapping is a
shared memory and doing some synchronization).

Re: [PATCH 2/3] Add system-wide tunables: cache ld.so.cache

[Carlos O'Donell]

On 6/19/24 8:52 AM, Adhemerval Zanella Netto wrote:
> 
> 
> On 29/05/24 14:15, DJ Delorie wrote:
>> Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:
>>> It is not clear to me why we need the load/reload support for ld.so.cache
>>> to support system-wide tunables.
>>
>> It's kinda independent.  Carlos requested it to benefit long-running
>> processes that call dlopen() regularly, to get updates to search paths.
>> The tunables code takes advantage of the refactoring to make sure the
>> cache is loaded when needed, since tunables happens before DSOs.
> 
> But currently we do not catch ld.so.cache, it is mmap'ed on process start
> and unmmap'ed at the end of DT_NEEDED process (elf/rtld.c:2402).  The same
> process is done on dlopen (elf/dl-open.c:909), so any update done by ldconfig
> will be seem on any subsequent dlopen.
The cache, while continuously mapped, is only updated by ldconfig with a rename:

 733   /* Move temporary to its final location.  */
 734   if (rename (temp_name, cache_name))
 735     error (EXIT_FAILURE, errno, _("Renaming of %s to %s failed"), temp_name,
 736            cache_name);

Therefore the old file remains, unlinked from the filesystem, and backing the mapping.

An atomic rename is the only sensible thing to do in this case, we have no guarantee that any
process on the system is in any state to traverse the file mapping while it is being updated.

> There is still a small window where either a new process is starting of dlopen
> is accessing the ld.so.cache contents and ldconfig is doing the final rename 
> of a new cache, but trying to synchronize it would require a more complex
> way (most likely either by making the ld.so.cache private or mapping is a
> shared memory and doing some synchronization).
 
May you please explain the race in more detail?

In summary: We need to detect the file change and reload the cache.

-- 
Cheers,
Carlos.

Re: [PATCH 2/3] Add system-wide tunables: cache ld.so.cache

[Adhemerval Zanella Netto]

On 20/06/24 16:08, Carlos O'Donell wrote:
> On 6/19/24 8:52 AM, Adhemerval Zanella Netto wrote:
>>
>>
>> On 29/05/24 14:15, DJ Delorie wrote:
>>> Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:
>>>> It is not clear to me why we need the load/reload support for ld.so.cache
>>>> to support system-wide tunables.
>>>
>>> It's kinda independent.  Carlos requested it to benefit long-running
>>> processes that call dlopen() regularly, to get updates to search paths.
>>> The tunables code takes advantage of the refactoring to make sure the
>>> cache is loaded when needed, since tunables happens before DSOs.
>>
>> But currently we do not catch ld.so.cache, it is mmap'ed on process start
>> and unmmap'ed at the end of DT_NEEDED process (elf/rtld.c:2402).  The same
>> process is done on dlopen (elf/dl-open.c:909), so any update done by ldconfig
>> will be seem on any subsequent dlopen.
> The cache, while continuously mapped, is only updated by ldconfig with a rename:
> 
>  733   /* Move temporary to its final location.  */
>  734   if (rename (temp_name, cache_name))
>  735     error (EXIT_FAILURE, errno, _("Renaming of %s to %s failed"), temp_name,
>  736            cache_name);
> 
> Therefore the old file remains, unlinked from the filesystem, and backing the mapping.
> 
> An atomic rename is the only sensible thing to do in this case, we have no guarantee that any
> process on the system is in any state to traverse the file mapping while it is being updated.
> 
>> There is still a small window where either a new process is starting of dlopen
>> is accessing the ld.so.cache contents and ldconfig is doing the final rename 
>> of a new cache, but trying to synchronize it would require a more complex
>> way (most likely either by making the ld.so.cache private or mapping is a
>> shared memory and doing some synchronization).
>  
> May you please explain the race in more detail?

Nevermind, for some reason I have a mental model that open and mmap could
race during concurrent ldconfig/symbol resolution; but kernel should not 
allow it.

> 
> In summary: We need to detect the file change and reload the cache.
> 

Right, but I am still not really sure if this is really an improvement.
On program startup and dlopen, it would make DT_NEEDED libraries 
potentially see different cache state (since _dl_map_object will be 
called for each dependency) which I am not really sure how safe and
robust it would be.  

I would expect that for both program loading and each ldopen, the
cache state used for libraries resolution to be the same.