This is the mail archive of the libc-hacker@sourceware.org mailing list for the glibc project.

Note that libc-hacker is a closed list. You may look at the archives of this list, but subscription and posting are not open.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Fix an ABBA deadlock in ld.so


Hi!

If one thread is in the middle of lazy binding, in between
THREAD_GSCOPE_SET_FLAG () and THREAD_GSCOPE_RESET_FLAG () and needs
to add_dependency, while another thread holds dl_load_lock and
in the middle of dlopen or dlclose needs to THREAD_GSCOPE_WAIT (),
we deadlock.  The first thread is waiting for the second to release
dl_load_lock, so it can acquire it in add_dependency,
while the second waits for the first one to THREAD_GSCOPE_RESET_FLAG ().

The following patch should fix it, by temporarily releasing the
THREAD_GSCOPE_* locking around acquiring dl_load_lock.  As in the mean
time map could have been e.g. dlclosed and removed, add_dependency
then needs to avoid dereferencing the MAP pointer until it has found
it through some list (so it is surely live link_map).  Even then,
it could have been dlclosed and a different dlopened library could
be given by calloc the earlier freed address, so I have added a 64-bit
serial counter to detect that (GL(dl_load_adds) increments monotonically
with each new link_map, so it can just be saved into the link_map).

2007-09-18  Jakub Jelinek  <jakub@redhat.com>

	* sysdeps/generic/ldsodefs.h (DL_LOOKUP_GSCOPE_LOCK): New.
	* elf/dl-runtime.c (_dl_fixup, _dl_profile_fixup): Or in
	DL_LOOKUP_GSCOPE_LOCK into flags after THREAD_GSCOPE_SET_FLAG ().
	* elf/dl-sym.c (do_sym): Likewise.
	* include/link.h (struct link_map): Add l_serial field.
	* elf/dl-object.c (_dl_new_object): Initialize l_serial.
	* elf/dl-lookup.c (add_dependency): Add flags argument.
	Remember map->l_serial, if DL_LOOKUP_GSCOPE_LOCK is among
	flags, use THREAD_GSCOPE_RESET_FLAG before and
	THREAD_GSCOPE_SET_FLAG after
	__rtld_lock_lock_recursive (GL(dl_load_lock)) to avoid deadlock.
	Don't dereference map until it has been found on some list.
	If map->l_serial changed, return -1.

--- libc/sysdeps/generic/ldsodefs.h.jj	2007-06-29 10:19:58.000000000 +0200
+++ libc/sysdeps/generic/ldsodefs.h	2007-09-18 14:59:18.000000000 +0200
@@ -845,7 +845,9 @@ enum
     DL_LOOKUP_ADD_DEPENDENCY = 1,
     /* Return most recent version instead of default version for
        unversioned lookup.  */
-    DL_LOOKUP_RETURN_NEWEST = 2
+    DL_LOOKUP_RETURN_NEWEST = 2,
+    /* Set if dl_lookup* called with GSCOPE lock held.  */
+    DL_LOOKUP_GSCOPE_LOCK = 4,
   };
 
 /* Lookup versioned symbol.  */
--- libc/elf/dl-runtime.c.jj	2007-06-29 10:19:55.000000000 +0200
+++ libc/elf/dl-runtime.c	2007-09-18 14:59:18.000000000 +0200
@@ -100,7 +100,10 @@ _dl_fixup (
 	 we are not using any threads (yet).  */
       int flags = DL_LOOKUP_ADD_DEPENDENCY;
       if (!RTLD_SINGLE_THREAD_P)
-	THREAD_GSCOPE_SET_FLAG ();
+	{
+	  THREAD_GSCOPE_SET_FLAG ();
+	  flags |= DL_LOOKUP_GSCOPE_LOCK;
+	}
 
       result = _dl_lookup_symbol_x (strtab + sym->st_name, l, &sym, l->l_scope,
 				    version, ELF_RTYPE_CLASS_PLT, flags, NULL);
@@ -192,7 +195,10 @@ _dl_profile_fixup (
 	     we are not using any threads (yet).  */
 	  int flags = DL_LOOKUP_ADD_DEPENDENCY;
 	  if (!RTLD_SINGLE_THREAD_P)
-	    THREAD_GSCOPE_SET_FLAG ();
+	    {
+	      THREAD_GSCOPE_SET_FLAG ();
+	      flags |= DL_LOOKUP_GSCOPE_LOCK;
+	    }
 
 	  result = _dl_lookup_symbol_x (strtab + refsym->st_name, l,
 					&defsym, l->l_scope, version,
--- libc/elf/dl-sym.c.jj	2007-06-29 10:19:55.000000000 +0200
+++ libc/elf/dl-sym.c	2007-09-18 14:59:18.000000000 +0200
@@ -123,7 +123,8 @@ do_sym (void *handle, const char *name, 
 	  args.name = name;
 	  args.map = match;
 	  args.vers = vers;
-	  args.flags = flags | DL_LOOKUP_ADD_DEPENDENCY;
+	  args.flags
+	    = flags | DL_LOOKUP_ADD_DEPENDENCY | DL_LOOKUP_GSCOPE_LOCK;
 	  args.refp = &ref;
 
 	  THREAD_GSCOPE_SET_FLAG ();
--- libc/include/link.h.jj	2007-06-29 10:19:55.000000000 +0200
+++ libc/include/link.h	2007-09-18 15:18:36.000000000 +0200
@@ -286,6 +286,8 @@ struct link_map
     ElfW(Addr) l_relro_addr;
     size_t l_relro_size;
 
+    unsigned long long l_serial;
+
     /* Audit information.  This array apparent must be the last in the
        structure.  Never add something after it.  */
     struct auditstate
--- libc/elf/dl-object.c.jj	2007-06-29 10:19:55.000000000 +0200
+++ libc/elf/dl-object.c	2007-09-18 15:20:29.000000000 +0200
@@ -103,6 +103,7 @@ _dl_new_object (char *realname, const ch
   else
     GL(dl_ns)[nsid]._ns_loaded = new;
   ++GL(dl_ns)[nsid]._ns_nloaded;
+  new->l_serial = GL(dl_load_adds);
   ++GL(dl_load_adds);
 
   /* If we have no loader the new object acts as it.  */
--- libc/elf/dl-lookup.c.jj	2007-06-29 10:19:55.000000000 +0200
+++ libc/elf/dl-lookup.c	2007-09-18 15:58:26.000000000 +0200
@@ -86,36 +86,42 @@ dl_new_hash (const char *s)
 /* Add extra dependency on MAP to UNDEF_MAP.  */
 static int
 internal_function
-add_dependency (struct link_map *undef_map, struct link_map *map)
+add_dependency (struct link_map *undef_map, struct link_map *map, int flags)
 {
   struct link_map **list;
   struct link_map *runp;
   unsigned int act;
   unsigned int i;
   int result = 0;
+  unsigned long long serial;
 
   /* Avoid self-references and references to objects which cannot be
      unloaded anyway.  */
   if (undef_map == map)
     return 0;
 
-  /* Make sure nobody can unload the object while we are at it.  */
-  __rtld_lock_lock_recursive (GL(dl_load_lock));
-
-  /* Avoid references to objects which cannot be unloaded anyway.  */
-  if (map->l_type != lt_loaded
-      || (map->l_flags_1 & DF_1_NODELETE) != 0)
-    goto out;
+  /* Save serial number of the target MAP.  */
+  serial = map->l_serial;
 
-  /* If the object with the undefined reference cannot be removed ever
-     just make sure the same is true for the object which contains the
-     definition.  */
-  if (undef_map->l_type != lt_loaded
-      || (undef_map->l_flags_1 & DF_1_NODELETE) != 0)
+  /* Make sure nobody can unload the object while we are at it.  */
+  if (__builtin_expect (flags & DL_LOOKUP_GSCOPE_LOCK, 0))
     {
-      map->l_flags_1 |= DF_1_NODELETE;
-      goto out;
+      /* We can't just call __rtld_lock_lock_recursive (GL(dl_load_lock))
+	 here, that can result in ABBA deadlock.  */
+      THREAD_GSCOPE_RESET_FLAG ();
+      __rtld_lock_lock_recursive (GL(dl_load_lock));
+      THREAD_GSCOPE_SET_FLAG ();
+      /* While MAP value won't change, after THREAD_GSCOPE_RESET_FLAG ()
+	 it can e.g. point to unallocated memory.  So avoid the optimizer
+	 treating the above read from MAP->l_serial as ensurance it
+	 can safely dereference it.  */
+      map = atomic_forced_read (map);
     }
+  else
+    __rtld_lock_lock_recursive (GL(dl_load_lock));
+
+  /* From this point on it is unsafe to dereference MAP, until it
+     has been found in one of the lists.  */
 
   /* Determine whether UNDEF_MAP already has a reference to MAP.  First
      look in the normal dependencies.  */
@@ -125,7 +131,7 @@ add_dependency (struct link_map *undef_m
 
       for (i = 0; list[i] != NULL; ++i)
 	if (list[i] == map)
-	  goto out;
+	  goto out_check;
     }
 
   /* No normal dependency.  See whether we already had to add it
@@ -135,7 +141,7 @@ add_dependency (struct link_map *undef_m
 
   for (i = 0; i < act; ++i)
     if (list[i] == map)
-      goto out;
+      goto out_check;
 
   /* The object is not yet in the dependency list.  Before we add
      it make sure just one more time the object we are about to
@@ -148,7 +154,29 @@ add_dependency (struct link_map *undef_m
 
   if (runp != NULL)
     {
-      /* The object is still available.  Add the reference now.  */
+      /* The object is still available.  */
+
+      /* MAP could have been dlclosed, freed and then some other dlopened
+	 library could have the same link_map pointer.  */
+      if (map->l_serial != serial)
+	goto out_check;
+
+      /* Avoid references to objects which cannot be unloaded anyway.  */
+      if (map->l_type != lt_loaded
+	  || (map->l_flags_1 & DF_1_NODELETE) != 0)
+	goto out;
+
+      /* If the object with the undefined reference cannot be removed ever
+	 just make sure the same is true for the object which contains the
+	 definition.  */
+      if (undef_map->l_type != lt_loaded
+	  || (undef_map->l_flags_1 & DF_1_NODELETE) != 0)
+	{
+	  map->l_flags_1 |= DF_1_NODELETE;
+	  goto out;
+	}
+
+      /* Add the reference now.  */
       if (__builtin_expect (act >= undef_map->l_reldepsmax, 0))
 	{
 	  /* Allocate more memory for the dependency list.  Since this
@@ -196,6 +224,11 @@ add_dependency (struct link_map *undef_m
   __rtld_lock_unlock_recursive (GL(dl_load_lock));
 
   return result;
+
+ out_check:
+  if (map->l_serial != serial)
+    result = -1;
+  goto out;
 }
 
 static void
@@ -227,9 +260,11 @@ _dl_lookup_symbol_x (const char *undef_n
 
   bump_num_relocations ();
 
-  /* No other flag than DL_LOOKUP_ADD_DEPENDENCY is allowed if we look
-     up a versioned symbol.  */
-  assert (version == NULL || (flags & ~(DL_LOOKUP_ADD_DEPENDENCY)) == 0);
+  /* No other flag than DL_LOOKUP_ADD_DEPENDENCY or DL_LOOKUP_GSCOPE_LOCK
+     is allowed if we look up a versioned symbol.  */
+  assert (version == NULL
+	  || (flags & ~(DL_LOOKUP_ADD_DEPENDENCY | DL_LOOKUP_GSCOPE_LOCK))
+	     == 0);
 
   size_t i = 0;
   if (__builtin_expect (skip_map != NULL, 0))
@@ -335,10 +370,12 @@ _dl_lookup_symbol_x (const char *undef_n
 	 runtime lookups.  */
       && (flags & DL_LOOKUP_ADD_DEPENDENCY) != 0
       /* Add UNDEF_MAP to the dependencies.  */
-      && add_dependency (undef_map, current_value.m) < 0)
+      && add_dependency (undef_map, current_value.m, flags) < 0)
       /* Something went wrong.  Perhaps the object we tried to reference
 	 was just removed.  Try finding another definition.  */
-      return _dl_lookup_symbol_x (undef_name, undef_map, ref, symbol_scope,
+      return _dl_lookup_symbol_x (undef_name, undef_map, ref,
+				  (flags & DL_LOOKUP_GSCOPE_LOCK)
+				  ? undef_map->l_scope : symbol_scope,
 				  version, type_class, flags, skip_map);
 
   /* The object is used.  */

	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]