* [PATCH] Fix an ABBA deadlock in ld.so
@ 2007-09-18 14:49 Jakub Jelinek
0 siblings, 0 replies; only message in thread
From: Jakub Jelinek @ 2007-09-18 14:49 UTC (permalink / raw)
To: Ulrich Drepper; +Cc: Glibc hackers
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
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2007-09-18 14:49 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-09-18 14:49 [PATCH] Fix an ABBA deadlock in ld.so Jakub Jelinek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).