From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20568 invoked by alias); 2 Feb 2010 16:57:02 -0000 Received: (qmail 20551 invoked by uid 22791); 2 Feb 2010 16:57:01 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 02 Feb 2010 16:56:57 +0000 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o12Guuhp022831 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 2 Feb 2010 11:56:56 -0500 Received: from hase.home (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx08.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o12Gus96026368 for ; Tue, 2 Feb 2010 11:56:55 -0500 From: Andreas Schwab To: libc-hacker@sourceware.org Subject: [PATCH] Don't deadlock in __dl_iterate_phdr while (un)loading objects X-Yow: I'm pretending I'm pulling in a TROUT! Am I doing it correctly?? Date: Tue, 02 Feb 2010 16:57:00 -0000 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact libc-hacker-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-hacker-owner@sourceware.org X-SW-Source: 2010-02/txt/msg00000.txt.bz2 A destructor may call pthread_cancel which triggers __dl_iterate_phdr in another thread, causing a deadlock because dl_load_hook is already locked. Andreas. 2010-02-02 Andreas Schwab * sysdeps/generic/ldsodefs.h (struct rtld_global): Add _dl_load_write_lock. * elf/rtld.c (_rtld_global): Initialize it. * elf/dl-support.c (_dl_load_write_lock): Define . * elf/dl-close.c (_dl_close_worker): Lock GL(dl_load_write_lock) when modifying the list of loaded objects. * elf/dl-load.c (lose): Likewise. * elf/dl-object.c (_dl_new_object): Likewise. * elf/dl-iteratephdr.c (__dl_iterate_phdr): Lock GL(dl_load_write_lock) instead of GL(dl_load_lock). diff --git a/elf/dl-close.c b/elf/dl-close.c index b73a7ad..700e765 100644 --- a/elf/dl-close.c +++ b/elf/dl-close.c @@ -507,6 +507,9 @@ _dl_close_worker (struct link_map *map) size_t tls_free_end; tls_free_start = tls_free_end = NO_TLS_OFFSET; + /* We modify the list of loaded objects. */ + __rtld_lock_lock_recursive (GL(dl_load_write_lock)); + /* Check each element of the search list to see if all references to it are gone. */ for (unsigned int i = first_loaded; i < nloaded; ++i) @@ -665,6 +668,8 @@ _dl_close_worker (struct link_map *map) } } + __rtld_lock_unlock_recursive (GL(dl_load_write_lock)); + /* If we removed any object which uses TLS bump the generation counter. */ if (any_tls) { diff --git a/elf/dl-iteratephdr.c b/elf/dl-iteratephdr.c index fee19f3..6e9eddb 100644 --- a/elf/dl-iteratephdr.c +++ b/elf/dl-iteratephdr.c @@ -26,7 +26,7 @@ static void cancel_handler (void *arg __attribute__((unused))) { - __rtld_lock_unlock_recursive (GL(dl_load_lock)); + __rtld_lock_unlock_recursive (GL(dl_load_write_lock)); } hidden_proto (__dl_iterate_phdr) @@ -38,8 +38,8 @@ __dl_iterate_phdr (int (*callback) (struct dl_phdr_info *info, struct dl_phdr_info info; int ret = 0; - /* Make sure we are alone. */ - __rtld_lock_lock_recursive (GL(dl_load_lock)); + /* Make sure nobody modifies the list of loaded objects. */ + __rtld_lock_lock_recursive (GL(dl_load_write_lock)); __libc_cleanup_push (cancel_handler, 0); /* We have to determine the namespace of the caller since this determines @@ -80,7 +80,7 @@ __dl_iterate_phdr (int (*callback) (struct dl_phdr_info *info, /* Release the lock. */ __libc_cleanup_pop (0); - __rtld_lock_unlock_recursive (GL(dl_load_lock)); + __rtld_lock_unlock_recursive (GL(dl_load_write_lock)); return ret; } diff --git a/elf/dl-load.c b/elf/dl-load.c index 597193c..68b5758 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -803,6 +803,8 @@ lose (int code, int fd, const char *name, char *realname, struct link_map *l, (void) __close (fd); if (l != NULL) { + /* We modify the list of loaded objects. */ + __rtld_lock_lock_recursive (GL(dl_load_write_lock)); /* Remove the stillborn object from the list and free it. */ assert (l->l_next == NULL); if (l->l_prev == NULL) @@ -813,6 +815,7 @@ lose (int code, int fd, const char *name, char *realname, struct link_map *l, l->l_prev->l_next = NULL; --GL(dl_ns)[l->l_ns]._ns_nloaded; free (l); + __rtld_lock_unlock_recursive (GL(dl_load_write_lock)); } free (realname); diff --git a/elf/dl-object.c b/elf/dl-object.c index 788e2c0..22a1635 100644 --- a/elf/dl-object.c +++ b/elf/dl-object.c @@ -93,6 +93,9 @@ _dl_new_object (char *realname, const char *libname, int type, new->l_scope = new->l_scope_mem; new->l_scope_max = sizeof (new->l_scope_mem) / sizeof (new->l_scope_mem[0]); + /* We modify the list of loaded objects. */ + __rtld_lock_lock_recursive (GL(dl_load_write_lock)); + /* Counter for the scopes we have to handle. */ idx = 0; @@ -114,6 +117,8 @@ _dl_new_object (char *realname, const char *libname, int type, new->l_serial = GL(dl_load_adds); ++GL(dl_load_adds); + __rtld_lock_unlock_recursive (GL(dl_load_write_lock)); + /* If we have no loader the new object acts as it. */ if (loader == NULL) loader = new; diff --git a/elf/dl-support.c b/elf/dl-support.c index bcf0e2a..6649981 100644 --- a/elf/dl-support.c +++ b/elf/dl-support.c @@ -158,6 +158,10 @@ const ElfW(Ehdr) *_dl_sysinfo_dso; the loaded object might as well require a call to this function. At this time it is not anymore a problem to modify the tables. */ __rtld_lock_define_initialized_recursive (, _dl_load_lock) +/* This lock is used to keep __dl_iterate_phdr from inspecting the + list of loaded objects while an object is added to or removed from + that list. */ +__rtld_lock_define_initialized_recursive (, _dl_load_write_lock) #ifdef HAVE_AUX_VECTOR diff --git a/elf/rtld.c b/elf/rtld.c index 3afb997..5702b45 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -126,6 +126,7 @@ struct rtld_global _rtld_global = ._dl_stack_flags = PF_R|PF_W|PF_X, #ifdef _LIBC_REENTRANT ._dl_load_lock = _RTLD_LOCK_RECURSIVE_INITIALIZER, + ._dl_load_write_lock = _RTLD_LOCK_RECURSIVE_INITIALIZER, #endif ._dl_nns = 1, ._dl_ns = diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h index 230c39a..1ba4608 100644 --- a/sysdeps/generic/ldsodefs.h +++ b/sysdeps/generic/ldsodefs.h @@ -416,6 +416,10 @@ struct rtld_global the loaded object might as well require a call to this function. At this time it is not anymore a problem to modify the tables. */ __rtld_lock_define_recursive (EXTERN, _dl_load_lock) + /* This lock is used to keep __dl_iterate_phdr from inspecting the + list of loaded objects while an object is added to or removed + from that list. */ + __rtld_lock_define_recursive (EXTERN, _dl_load_write_lock) /* Incremented whenever something may have been added to dl_loaded. */ EXTERN unsigned long long _dl_load_adds; -- 1.6.6.1 -- Andreas Schwab, schwab@redhat.com GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84 5EC7 45C6 250E 6F00 984E "And now for something completely different."