From patchwork Tue Dec 29 10:38:53 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Palachev X-Patchwork-Id: 10159 Received: (qmail 66523 invoked by alias); 29 Dec 2015 10:40:35 -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 66492 invoked by uid 89); 29 Dec 2015 10:40:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.1 required=5.0 tests=AWL, BAYES_20, KAM_ASCII_DIVIDERS, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=no version=3.3.2 spammy=dl_main, Statement, H*r:Tue, 9435 X-HELO: mailout3.w1.samsung.com From: Ilya Palachev To: libc-alpha@sourceware.org Cc: triegel@redhat.com, siddhesh.poyarekar@linaro.org, szabolcs.nagy@arm.com, v.barinov@samsung.com, Ilya Palachev Subject: [PATCH] [BZ #19329] Fix race during concurrent dlopen and pthread_create Date: Tue, 29 Dec 2015 13:38:53 +0300 Message-id: <1451385533-9485-1-git-send-email-i.palachev@samsung.com> This patch fixes the bug https://sourceware.org/bugzilla/show_bug.cgi?id=19329 that happens when pthread_create and dlopen are run concurrently in two parallel threads. In this case the race between two threads can happen as follows: thread #1 thread #2 || || .. .. || || vv vv dlopen pthread_create || || .. .. || || vv vv dl_open_worker----------------------+ _dl_allocate_tls_init--------+ | | | | | ... | | ... | | _dl_add_to_slotinfo----------+ | | | | | | | | | | | ... | | | | | | | | | | | | /* Statement #1. */ | | | | | | listp->slotinfo[idx].gen =| | | | | | GL(dl_tls_generation) + 1;| | | | | | | | | | | | | | | | | +----------------------------+ | | | | | | | | | | /* Statement #3. */ | | /* The race window is here. */ | | assert ( | | | | listp->slotinfo[cnt].gen | | | | <= GL(dl_tls_generation)); | | | | | | /* Statement #2. */ | | | | ++GL(dl_tls_generation); | | | | | | | | | | | +-----------------------------------+ +----------------------------+ The patch changes the code so that to unify statements #1 and #2 in one atomic block so that to prevent the race window in which the statement #3 can happen. The changes have no side effects because the changed check if (any_tls && __builtin_expect (GL(dl_tls_generation) == 0, 0)) succeeds iff any_tls is true and hence iff _dl_add_to_slotinfo was called previously. So it can't happen at startup when GL(dl_tls_generation) == 0. The function _dl_add_to_slotinfo is called only from dl_open_worker and dl_main in elf/rtld.c where the dependencies of the currently executed program are loaded. Built and regtested for {x86_64,aarh64}-linux-gnu. Signed-off-by: Ilya Palachev [BZ #19329] * elf/dl-open.c (dl_open_worker): Remove the incrementation of global generation counter from here, since it should happen immediately after the addition of new TLS module to the slot info list. * elf/dl-tls.c (_dl_add_to_slotinfo): Add the atomic incrementation of global generation counter just when the new TLS module is added. This prevents the race that existed before. --- ChangeLog | 10 ++++++++++ elf/dl-open.c | 14 +++++++++++--- elf/dl-tls.c | 7 ++++++- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index 16e605a..3d7b007 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2015-12-29 Ilya Palachev + + [BZ #19329] + * elf/dl-open.c (dl_open_worker): Remove the incrementation of global + generation counter from here, since it should happen immediately after + the addition of new TLS module to the slot info list. + * elf/dl-tls.c (_dl_add_to_slotinfo): Add the atomic incrementation of + global generation counter just when the new TLS module is added. This + prevents the race that existed before. + 2015-12-21 Florian Weimer [BZ #19182] diff --git a/elf/dl-open.c b/elf/dl-open.c index 5429d18..a0eff3b 100644 --- a/elf/dl-open.c +++ b/elf/dl-open.c @@ -514,7 +514,9 @@ dl_open_worker (void *a) && first_static_tls == new->l_searchlist.r_nlist) first_static_tls = i; - /* We have to bump the generation counter. */ + /* At least one new module with TLS has been loaded. This is the + only place where the value of any_tls becomes true, and it + happen when and only when the _dl_add_to_slotinfo was called. */ any_tls = true; } @@ -523,8 +525,14 @@ dl_open_worker (void *a) _dl_show_scope (imap, from_scope); } - /* Bump the generation number if necessary. */ - if (any_tls && __builtin_expect (++GL(dl_tls_generation) == 0, 0)) + /* Check the TLS generation counter integer overflow if at least one new + module with TLS has been loaded. The condition holds iff any_tls is + true. It can happen only iff _dl_add_to_slotinfo was called (see above + code). And _dl_add_to_slotinfo always increments the value of + GL(dl_tls_generation) atomically (it's done to prevent the race). + That's why the fatal error does not happen in case when + GL(dl_tls_generation) == 0 before any TLS module load. */ + if (any_tls && __builtin_expect (GL(dl_tls_generation) == 0, 0)) _dl_fatal_printf (N_("\ TLS generation counter wrapped! Please report this.")); diff --git a/elf/dl-tls.c b/elf/dl-tls.c index 20c7e33..3cde943 100644 --- a/elf/dl-tls.c +++ b/elf/dl-tls.c @@ -943,5 +943,10 @@ cannot create TLS data structures")); /* Add the information into the slotinfo data structure. */ listp->slotinfo[idx].map = l; - listp->slotinfo[idx].gen = GL(dl_tls_generation) + 1; + /* Atomically increment the global TLS generation counter and set the + generation counter of new TLS module to this updated value. This + helps to prevent the race that happens if somebody tries to read + the dl_tls_generation when listp->slotinfo[idx].gen is already + incremented while GL(dl_tls_generation) was not. */ + listp->slotinfo[idx].gen = atomic_increment_val(&GL(dl_tls_generation)); }