From patchwork Thu Dec 5 15:19:34 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Florian Weimer X-Patchwork-Id: 1204643 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=sourceware.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=libc-alpha-return-107744-incoming=patchwork.ozlabs.org@sourceware.org; receiver=) Authentication-Results: ozlabs.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="T9OmEYXL"; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="MTs88zkx"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 47TKCG5Z9gz9sP3 for ; Fri, 6 Dec 2019 02:19:54 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:in-reply-to:references :message-id:date:mime-version:content-type :content-transfer-encoding; q=dns; s=default; b=pcEjfmVlMsaCoKnh hlQUyjHR+7AK/907GQnsYZezfCjsmQaEey2wenLaZ7CkepnzVoysHsTauOIh/NW5 fYJhs5zR53OGJ9k83yyIy/hBwzQObalsQC4lnfa9CfZFEC5rWby4mAA1H8+hxh5I GnyatQyqwFvD8hZVsn6mqx00n4U= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:in-reply-to:references :message-id:date:mime-version:content-type :content-transfer-encoding; s=default; bh=s0VDQI1oxfmQ94OUAIEgCR /Kjec=; b=T9OmEYXLzG7pS3yo/R8Oj5XuESmY49Bue2DNmdy/pxSPTbhLJyfstO jvfcUgWFZVtI+WJyFQm98z7kA6rSV3dCe7qZFr7xAttPmQfy8UY71AFSsB8ceZcR 6mxg9486z/EYCxgGVh3lO/zuw8GCV/iOJ+5jGQTBGo8y0dif9RMeM= Received: (qmail 29970 invoked by alias); 5 Dec 2019 15:19:47 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 29885 invoked by uid 89); 5 Dec 2019 15:19:46 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=activating, H*i:sk:cover.1 X-HELO: us-smtp-delivery-1.mimecast.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575559181; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=PzsuToySAKpL1JaeTpil8aJaR4F3mwEvCmAurPTC0oI=; b=MTs88zkxuLX86MKA/6Ku7lDYZtWqnCEu8WfNSJQe/J0yIFBuiu2xbqZzHXWU4pWqK9uw+X 9XcWtWIaKnc2Wr3Ogvez3ckRhVUGU2qYr8PPLs5BKijdvgfA5120+C0/SX6XhKpfG+ZyUI FWdJpCTmErozJyrhcrsrCjAIvNM4VYU= From: Florian Weimer To: libc-alpha@sourceware.org Subject: [PATCH 1/2] dlopen: Rework handling of pending NODELETE status In-Reply-To: References: Message-Id: <9a3ad6519fdcc6986cadb9ad169c00eb25f5d515.1575558989.git.fweimer@redhat.com> Date: Thu, 05 Dec 2019 16:19:34 +0100 User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 To avoid a read-modify-write cycle on the l_nodelete field, this commit introduces two flags for active NODELETE status (irrevocable) and pending NODELETE status (revocable until activate_nodelete) is invoked. As a result, NODELETE processing in dlopen does not introduce further reasons why lazy binding from signal handlers is unsafe during dlopen, and a subsequent commit can remove signal blocking from dlopen. Reviewed-by: Adhemerval Zanella Reviewed-by: Carlos O'Donell --- elf/dl-close.c | 7 +++-- elf/dl-lookup.c | 58 +++++++++++++++++++++++++----------------- elf/dl-open.c | 22 +++++++++------- elf/get-dynamic-info.h | 2 +- include/link.h | 31 ++++++++-------------- 5 files changed, 62 insertions(+), 58 deletions(-) diff --git a/elf/dl-close.c b/elf/dl-close.c index e35a62daf6..df1df6fb29 100644 --- a/elf/dl-close.c +++ b/elf/dl-close.c @@ -197,7 +197,7 @@ _dl_close_worker (struct link_map *map, bool force) /* Check whether this object is still used. */ if (l->l_type == lt_loaded && l->l_direct_opencount == 0 - && l->l_nodelete != link_map_nodelete_active + && !l->l_nodelete_active /* See CONCURRENCY NOTES in cxa_thread_atexit_impl.c to know why acquire is sufficient and correct. */ && atomic_load_acquire (&l->l_tls_dtor_count) == 0 @@ -279,8 +279,7 @@ _dl_close_worker (struct link_map *map, bool force) if (!used[i]) { - assert (imap->l_type == lt_loaded - && imap->l_nodelete != link_map_nodelete_active); + assert (imap->l_type == lt_loaded && !imap->l_nodelete_active); /* Call its termination function. Do not do it for half-cooked objects. Temporarily disable exception @@ -830,7 +829,7 @@ _dl_close (void *_map) before we took the lock. There is no way to detect this (see below) so we proceed assuming this isn't the case. First see whether we can remove the object at all. */ - if (__glibc_unlikely (map->l_nodelete == link_map_nodelete_active)) + if (__glibc_unlikely (map->l_nodelete_active)) { /* Nope. Do nothing. */ __rtld_lock_unlock_recursive (GL(dl_load_lock)); diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c index 99846918c3..759b45a2c9 100644 --- a/elf/dl-lookup.c +++ b/elf/dl-lookup.c @@ -187,6 +187,28 @@ enter_unique_sym (struct unique_sym *table, size_t size, table[idx].map = map; } +/* Mark MAP as NODELETE according to the lookup mode in FLAGS. During + initial relocation, NODELETE state is pending only. */ +static void +mark_nodelete (struct link_map *map, int flags) +{ + if (flags & DL_LOOKUP_FOR_RELOCATE) + map->l_nodelete_pending = true; + else + map->l_nodelete_active = true; +} + +/* Return true if MAP is marked as NODELETE according to the lookup + mode in FLAGS> */ +static bool +is_nodelete (struct link_map *map, int flags) +{ + /* Non-pending NODELETE always counts. Pending NODELETE only counts + during initial relocation processing. */ + return map->l_nodelete_active + || ((flags & DL_LOOKUP_FOR_RELOCATE) && map->l_nodelete_pending); +} + /* Utility function for do_lookup_x. Lookup an STB_GNU_UNIQUE symbol in the unique symbol table, creating a new entry if necessary. Return the matching symbol in RESULT. */ @@ -311,8 +333,7 @@ do_lookup_unique (const char *undef_name, uint_fast32_t new_hash, enter_unique_sym (entries, size, new_hash, strtab + sym->st_name, sym, map); - if (map->l_type == lt_loaded - && map->l_nodelete == link_map_nodelete_inactive) + if (map->l_type == lt_loaded && !is_nodelete (map, flags)) { /* Make sure we don't unload this object by setting the appropriate flag. */ @@ -320,10 +341,7 @@ do_lookup_unique (const char *undef_name, uint_fast32_t new_hash, _dl_debug_printf ("\ marking %s [%lu] as NODELETE due to unique symbol\n", map->l_name, map->l_ns); - if (flags & DL_LOOKUP_FOR_RELOCATE) - map->l_nodelete = link_map_nodelete_pending; - else - map->l_nodelete = link_map_nodelete_active; + mark_nodelete (map, flags); } } ++tab->n_elements; @@ -586,7 +604,7 @@ add_dependency (struct link_map *undef_map, struct link_map *map, int flags) dependencies may pick an dependency which can be dlclose'd, but such IFUNC resolvers are undefined anyway. */ assert (map->l_type == lt_loaded); - if (map->l_nodelete != link_map_nodelete_inactive) + if (is_nodelete (map, flags)) return 0; struct link_map_reldeps *l_reldeps @@ -694,17 +712,16 @@ add_dependency (struct link_map *undef_map, struct link_map *map, int flags) /* Redo the NODELETE check, as when dl_load_lock wasn't held yet this could have changed. */ - if (map->l_nodelete != link_map_nodelete_inactive) + if (is_nodelete (map, flags)) 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_nodelete != link_map_nodelete_inactive)) + if (undef_map->l_type != lt_loaded || is_nodelete (map, flags)) { if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS) - && map->l_nodelete == link_map_nodelete_inactive) + && !is_nodelete (map, flags)) { if (undef_map->l_name[0] == '\0') _dl_debug_printf ("\ @@ -716,11 +733,7 @@ marking %s [%lu] as NODELETE due to reference to %s [%lu]\n", map->l_name, map->l_ns, undef_map->l_name, undef_map->l_ns); } - - if (flags & DL_LOOKUP_FOR_RELOCATE) - map->l_nodelete = link_map_nodelete_pending; - else - map->l_nodelete = link_map_nodelete_active; + mark_nodelete (map, flags); goto out; } @@ -746,17 +759,14 @@ marking %s [%lu] as NODELETE due to reference to %s [%lu]\n", cannot be unloaded. This is semantically the correct behavior. */ if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_BINDINGS) - && map->l_nodelete == link_map_nodelete_inactive) + && !is_nodelete (map, flags)) _dl_debug_printf ("\ marking %s [%lu] as NODELETE due to memory allocation failure\n", map->l_name, map->l_ns); - if (flags & DL_LOOKUP_FOR_RELOCATE) - /* In case of non-lazy binding, we could actually - report the memory allocation error, but for now, we - use the conservative approximation as well. */ - map->l_nodelete = link_map_nodelete_pending; - else - map->l_nodelete = link_map_nodelete_active; + /* In case of non-lazy binding, we could actually report + the memory allocation error, but for now, we use the + conservative approximation as well. */ + mark_nodelete (map, flags); goto out; } else diff --git a/elf/dl-open.c b/elf/dl-open.c index 56f213323c..c23341be58 100644 --- a/elf/dl-open.c +++ b/elf/dl-open.c @@ -440,13 +440,17 @@ activate_nodelete (struct link_map *new) NODELETE status for objects outside the local scope. */ for (struct link_map *l = GL (dl_ns)[new->l_ns]._ns_loaded; l != NULL; l = l->l_next) - if (l->l_nodelete == link_map_nodelete_pending) + if (l->l_nodelete_pending) { if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES)) _dl_debug_printf ("activating NODELETE for %s [%lu]\n", l->l_name, l->l_ns); - l->l_nodelete = link_map_nodelete_active; + l->l_nodelete_active = true; + + /* This is just a debugging aid, to indicate that + activate_nodelete has run for this map. */ + l->l_nodelete_pending = false; } } @@ -549,10 +553,10 @@ dl_open_worker (void *a) if (__glibc_unlikely (mode & RTLD_NODELETE)) { if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_FILES) - && new->l_nodelete == link_map_nodelete_inactive) + && !new->l_nodelete_active) _dl_debug_printf ("marking %s [%lu] as NODELETE\n", new->l_name, new->l_ns); - new->l_nodelete = link_map_nodelete_active; + new->l_nodelete_active = true; } /* Finalize the addition to the global scope. */ @@ -568,7 +572,7 @@ dl_open_worker (void *a) /* Schedule NODELETE marking for the directly loaded object if requested. */ if (__glibc_unlikely (mode & RTLD_NODELETE)) - new->l_nodelete = link_map_nodelete_pending; + new->l_nodelete_pending = true; /* Load that object's dependencies. */ _dl_map_object_deps (new, NULL, 0, 0, @@ -683,7 +687,7 @@ dl_open_worker (void *a) _dl_start_profile (); /* Prevent unloading the object. */ - GL(dl_profile_map)->l_nodelete = link_map_nodelete_active; + GL(dl_profile_map)->l_nodelete_active = true; } } else @@ -882,9 +886,9 @@ no more namespaces available for dlmopen()")); happens inside dl_open_worker. */ __libc_signal_restore_set (&args.original_signal_mask); - /* All link_map_nodelete_pending objects should have been - deleted at this point, which is why it is not necessary - to reset the flag here. */ + /* All l_nodelete_pending objects should have been deleted + at this point, which is why it is not necessary to reset + the flag here. */ } else __libc_signal_restore_set (&args.original_signal_mask); diff --git a/elf/get-dynamic-info.h b/elf/get-dynamic-info.h index 075683d729..ea4e304bef 100644 --- a/elf/get-dynamic-info.h +++ b/elf/get-dynamic-info.h @@ -164,7 +164,7 @@ elf_get_dynamic_info (struct link_map *l, ElfW(Dyn) *temp) { l->l_flags_1 = info[VERSYMIDX (DT_FLAGS_1)]->d_un.d_val; if (l->l_flags_1 & DF_1_NODELETE) - l->l_nodelete = link_map_nodelete_pending; + l->l_nodelete_pending = true; /* Only DT_1_SUPPORTED_MASK bits are supported, and we would like to assert this, but we can't. Users have been setting diff --git a/include/link.h b/include/link.h index 2e771e433a..2acc836380 100644 --- a/include/link.h +++ b/include/link.h @@ -79,22 +79,6 @@ struct r_search_path_struct int malloced; }; -/* Type used by the l_nodelete member. */ -enum link_map_nodelete -{ - /* This link map can be deallocated. */ - link_map_nodelete_inactive = 0, /* Zero-initialized in _dl_new_object. */ - - /* This link map cannot be deallocated. */ - link_map_nodelete_active, - - /* This link map cannot be deallocated after dlopen has succeded. - dlopen turns this into link_map_nodelete_active. dlclose treats - this intermediate state as link_map_nodelete_active. */ - link_map_nodelete_pending, -}; - - /* Structure describing a loaded shared object. The `l_next' and `l_prev' members form a chain of all the shared objects loaded at startup. @@ -218,10 +202,17 @@ struct link_map freed, ie. not allocated with the dummy malloc in ld.so. */ - /* Actually of type enum link_map_nodelete. Separate byte due to - a read in add_dependency in elf/dl-lookup.c outside the loader - lock. Only valid for l_type == lt_loaded. */ - unsigned char l_nodelete; + /* NODELETE status of the map. Only valid for maps of type + lt_loaded. Lazy binding sets l_nodelete_active directly, + potentially from signal handlers. Initial loading of an + DF_1_NODELETE object set l_nodelete_pending. Relocation may + set l_nodelete_pending as well. l_nodelete_pending maps are + promoted to l_nodelete_active status in the final stages of + dlopen, prior to calling ELF constructors. dlclose only + refuses to unload l_nodelete_active maps, the pending status is + ignored. */ + bool l_nodelete_active; + bool l_nodelete_pending; #include