From patchwork Thu Oct 11 02:57:54 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tulio Magno Quites Machado Filho X-Patchwork-Id: 982193 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=sourceware.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=libc-alpha-return-96349-incoming=patchwork.ozlabs.org@sourceware.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=linux.ibm.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="i8AfnR5u"; 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 42Vwf81Mwwz9s8r for ; Thu, 11 Oct 2018 13:58:55 +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:date:in-reply-to:references :message-id; q=dns; s=default; b=ABz0p3iKrHzLt/tfRGFGnHMcEOyo6yj RYN9/rEXwWWt+F2Aqij1ThzEd5qpdlTYfKBIHfnElglys9QjNSwuuehcCz4ZRfPz kVqQzaF1rhHhrLewHM5HHc1XLLskk3GqNiVtl0Ad95yzJ9b2JR+iVnu2CU/ecTwO jFnceOAhvq58= 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:date:in-reply-to:references :message-id; s=default; bh=a7djn6FSicWN89bS0mppIeXHjUY=; b=i8Afn R5unSLloiia4y0v1Hu3rJNi/yLQn0brDO4iDp9iqQHiSPXqXXhoyBI8zQ5U0geY8 V0tWO8dNsTS6K167Lw5lF/wHZkpfg5gGF56e6cAlIgde6qk9Hz83Z7ttUi/wvTOb +Fk7uj1/R1uoIJu6hJl/7HsfdwPFIOgwpunkuo= Received: (qmail 76934 invoked by alias); 11 Oct 2018 02:58:49 -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 76920 invoked by uid 89); 11 Oct 2018 02:58:48 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_MANYTO, KAM_SHORT, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=H*Ad:U*fw, os X-HELO: mx0a-001b2d01.pphosted.com From: Tulio Magno Quites Machado Filho To: Florian Weimer , "Carlos O'Donell" , libc-alpha@sourceware.org, John David Anglin , Adhemerval Zanella , Joseph Myers , Florian Weimer Subject: [PATCHv3] Protect _dl_profile_fixup data-dependency order [BZ #23690] Date: Wed, 10 Oct 2018 23:57:54 -0300 In-Reply-To: <87tvlwxmpi.fsf@mid.deneb.enyo.de> References: <87tvlwxmpi.fsf@mid.deneb.enyo.de> x-cbid: 18101102-2213-0000-0000-000003018475 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00009857; HX=3.00000242; KW=3.00000007; PH=3.00000004; SC=3.00000268; SDB=6.01100901; UDB=6.00569630; IPR=6.00880949; MB=3.00023701; MTD=3.00000008; XFM=3.00000015; UTC=2018-10-11 02:58:41 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 18101102-2214-0000-0000-00005BDA767C Message-Id: <20181011025754.23862-1-tuliom@linux.ibm.com> Florian Weimer writes: > * Tulio Magno Quites Machado Filho: > >> I suspect this patch doesn't address all the comments from v1. >> However, I believe some of the open questions/comments may not be >> necessary anymore after the latest changes. >> >> I've decided to not add the new test to xtests, because it executes in >> less than 3s in most of my tests. There is just a single case that >> takes up to 30s. >> >> Changes since v1: >> >> - Fixed the coding style issues. >> - Replaced atomic loads/store with memory fences. >> - Added a test. > > I don't think the fences are correct, they still need to be combined > with relaxed MO loads and stores. > > Does the issue that Carlos mentioned really show up in cross-builds? Yes, it does fail on hppa and ia64. But v3 (using thread fences) pass on build-many-glibcs. Changes since v2: - Fixed coding style in nptl/tst-audit-threads-mod1.c. - Replaced pthreads.h functions with respective support/xthread.h ones. - Replaced malloc() with xcalloc() in nptl/tst-audit-threads.c. - Removed bzero(). - Reduced the amount of functions to 7k in order to fit the relocation limit of some architectures, e.g. m68k, mips. - Fixed issues in nptl/Makefile. Changes since v1: - Fixed the coding style issues. - Replaced atomic loads/store with memory fences. - Added a test. ---- 8< ---- The field reloc_result->addr is used to indicate if the rest of the fields of reloc_result have already been written, creating a data-dependency order. Reading reloc_result->addr to the variable value requires to complete before reading the rest of the fields of reloc_result. Likewise, the writes to the other fields of the reloc_result must complete before reloc_result-addr is updated. Tested with build-many-glibcs. 2018-10-10 Tulio Magno Quites Machado Filho [BZ #23690] * elf/dl-runtime.c (_dl_profile_fixup): Guarantee memory modification order when accessing reloc_result->addr. * nptl/Makefile (tests): Add tst-audit-threads. (modules-names): Add tst-audit-threads-mod1 and tst-audit-threads-mod2. Add rules to build tst-audit-threads. * nptl/tst-audit-threads-mod1.c: New file. * nptl/tst-audit-threads-mod2.c: Likewise. * nptl/tst-audit-threads.c: Likewise. * nptl/tst-audit-threads.h: Likewise. Signed-off-by: Tulio Magno Quites Machado Filho --- elf/dl-runtime.c | 19 ++++++++- nptl/Makefile | 10 ++++- nptl/tst-audit-threads-mod1.c | 38 ++++++++++++++++++ nptl/tst-audit-threads-mod2.c | 22 +++++++++++ nptl/tst-audit-threads.c | 91 +++++++++++++++++++++++++++++++++++++++++++ nptl/tst-audit-threads.h | 84 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 260 insertions(+), 4 deletions(-) create mode 100644 nptl/tst-audit-threads-mod1.c create mode 100644 nptl/tst-audit-threads-mod2.c create mode 100644 nptl/tst-audit-threads.c create mode 100644 nptl/tst-audit-threads.h diff --git a/elf/dl-runtime.c b/elf/dl-runtime.c index 63bbc89776..c1ba372bd7 100644 --- a/elf/dl-runtime.c +++ b/elf/dl-runtime.c @@ -183,9 +183,18 @@ _dl_profile_fixup ( /* This is the address in the array where we store the result of previous relocations. */ struct reloc_result *reloc_result = &l->l_reloc_result[reloc_index]; - DL_FIXUP_VALUE_TYPE *resultp = &reloc_result->addr; + /* CONCURRENCY NOTES: + + The following code uses DL_FIXUP_VALUE_CODE_ADDR to access a potential + member of reloc_result->addr to indicate if it is the first time this + object is being relocated. + Reading/Writing from/to reloc_result->addr must not happen before previous + writes to reloc_result complete as they could end-up with an incomplete + struct. */ + DL_FIXUP_VALUE_TYPE *resultp = &reloc_result->addr; DL_FIXUP_VALUE_TYPE value = *resultp; + atomic_thread_fence_acquire (); if (DL_FIXUP_VALUE_CODE_ADDR (value) == 0) { /* This is the first time we have to relocate this object. */ @@ -346,7 +355,13 @@ _dl_profile_fixup ( /* Store the result for later runs. */ if (__glibc_likely (! GLRO(dl_bind_not))) - *resultp = value; + { + /* Guarantee all previous writes complete before + resultp (aka. reloc_result->addr) is updated. See CONCURRENCY + NOTES earlier */ + atomic_thread_fence_release (); + *resultp = value; + } } /* By default we do not call the pltexit function. */ diff --git a/nptl/Makefile b/nptl/Makefile index be8066524c..48aba579c0 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -382,7 +382,8 @@ tests += tst-cancelx2 tst-cancelx3 tst-cancelx4 tst-cancelx5 \ tst-cleanupx0 tst-cleanupx1 tst-cleanupx2 tst-cleanupx3 tst-cleanupx4 \ tst-oncex3 tst-oncex4 ifeq ($(build-shared),yes) -tests += tst-atfork2 tst-tls4 tst-_res1 tst-fini1 tst-compat-forwarder +tests += tst-atfork2 tst-tls4 tst-_res1 tst-fini1 tst-compat-forwarder \ + tst-audit-threads tests-internal += tst-tls3 tst-tls3-malloc tst-tls5 tst-stackguard1 tests-nolibpthread += tst-fini1 ifeq ($(have-z-execstack),yes) @@ -394,7 +395,8 @@ modules-names = tst-atfork2mod tst-tls3mod tst-tls4moda tst-tls4modb \ tst-tls5mod tst-tls5moda tst-tls5modb tst-tls5modc \ tst-tls5modd tst-tls5mode tst-tls5modf tst-stack4mod \ tst-_res1mod1 tst-_res1mod2 tst-execstack-mod tst-fini1mod \ - tst-join7mod tst-compat-forwarder-mod + tst-join7mod tst-compat-forwarder-mod tst-audit-threads-mod1 \ + tst-audit-threads-mod2 extra-test-objs += $(addsuffix .os,$(strip $(modules-names))) \ tst-cleanup4aux.o tst-cleanupx4aux.o test-extras += tst-cleanup4aux tst-cleanupx4aux @@ -709,6 +711,10 @@ endif $(objpfx)tst-compat-forwarder: $(objpfx)tst-compat-forwarder-mod.so +$(objpfx)tst-audit-threads: $(objpfx)tst-audit-threads-mod2.so +$(objpfx)tst-audit-threads.out: $(objpfx)tst-audit-threads-mod1.so +tst-audit-threads-ENV = LD_AUDIT=$(objpfx)tst-audit-threads-mod1.so + # The tests here better do not run in parallel ifneq ($(filter %tests,$(MAKECMDGOALS)),) .NOTPARALLEL: diff --git a/nptl/tst-audit-threads-mod1.c b/nptl/tst-audit-threads-mod1.c new file mode 100644 index 0000000000..194c65a6bb --- /dev/null +++ b/nptl/tst-audit-threads-mod1.c @@ -0,0 +1,38 @@ +/* Dummy audit library for test-audit-threads. + + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#include +#include +#include +#include +#include + +volatile int count = 0; + +unsigned int +la_version (unsigned int ver) +{ + return 1; +} + +unsigned int +la_objopen (struct link_map *map, Lmid_t lmid, uintptr_t *cookie) +{ + return LA_FLG_BINDTO | LA_FLG_BINDFROM; +} diff --git a/nptl/tst-audit-threads-mod2.c b/nptl/tst-audit-threads-mod2.c new file mode 100644 index 0000000000..6ceedb0196 --- /dev/null +++ b/nptl/tst-audit-threads-mod2.c @@ -0,0 +1,22 @@ +/* Shared object with a huge number of functions for test-audit-threads. + + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +/* Define all the retNumN functions. */ +#define definenum +#include "tst-audit-threads.h" diff --git a/nptl/tst-audit-threads.c b/nptl/tst-audit-threads.c new file mode 100644 index 0000000000..0c81edc762 --- /dev/null +++ b/nptl/tst-audit-threads.c @@ -0,0 +1,91 @@ +/* Test multi-threading using LD_AUDIT. + + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +/* This test uses a dummy LD_AUDIT library (test-audit-threads-mod1) and a + library with a huge number of functions in order to validate lazy symbol + binding with an audit library. */ + +#include +#include +#include +#include + +static int do_test (void); + +/* This test usually takes less than 3s to run. However, there are cases that + take up to 30s. */ +#define TIMEOUT 60 +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" + +#define externnum +#include "tst-audit-threads.h" +#undef externnum + +int num_threads; +pthread_barrier_t barrier; + +void +sync_all (int num) +{ + pthread_barrier_wait (&barrier); +} + +void +call_all_ret_nums (void) +{ +#define callnum +#include "tst-audit-threads.h" +#undef callnum +} + +void * +thread_main (void *unused) +{ + call_all_ret_nums (); + return NULL; +} + +#define STR2(X) #X +#define STR(X) STR2(X) + +static int +do_test (void) +{ + int i; + pthread_t *threads; + + num_threads = get_nprocs (); + if (num_threads <= 1) + num_threads = 2; + + /* Used to synchronize all the threads after calling each retNumN. */ + xpthread_barrier_init (&barrier, NULL, num_threads); + + threads = (pthread_t *) xcalloc (num_threads, sizeof(pthread_t)); + for (i = 0; i < num_threads; i++) + threads[i] = xpthread_create(NULL, thread_main, NULL); + + for (i = 0; i < num_threads; i++) + xpthread_join(threads[i]); + + free (threads); + + return 0; +} diff --git a/nptl/tst-audit-threads.h b/nptl/tst-audit-threads.h new file mode 100644 index 0000000000..cb17645f4b --- /dev/null +++ b/nptl/tst-audit-threads.h @@ -0,0 +1,84 @@ +/* Helper header for test-audit-threads. + + Copyright (C) 2018 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + . */ + +#define CONCAT(a, b) a ## b +#define NUM(x, y) CONCAT (x, y) + +#define FUNC10(x) \ + FUNC (NUM (x, 0)); \ + FUNC (NUM (x, 1)); \ + FUNC (NUM (x, 2)); \ + FUNC (NUM (x, 3)); \ + FUNC (NUM (x, 4)); \ + FUNC (NUM (x, 5)); \ + FUNC (NUM (x, 6)); \ + FUNC (NUM (x, 7)); \ + FUNC (NUM (x, 8)); \ + FUNC (NUM (x, 9)) + +#define FUNC100(x) \ + FUNC10 (NUM (x, 0)); \ + FUNC10 (NUM (x, 1)); \ + FUNC10 (NUM (x, 2)); \ + FUNC10 (NUM (x, 3)); \ + FUNC10 (NUM (x, 4)); \ + FUNC10 (NUM (x, 5)); \ + FUNC10 (NUM (x, 6)); \ + FUNC10 (NUM (x, 7)); \ + FUNC10 (NUM (x, 8)); \ + FUNC10 (NUM (x, 9)) + +#define FUNC1000(x) \ + FUNC100 (NUM (x, 0)); \ + FUNC100 (NUM (x, 1)); \ + FUNC100 (NUM (x, 2)); \ + FUNC100 (NUM (x, 3)); \ + FUNC100 (NUM (x, 4)); \ + FUNC100 (NUM (x, 5)); \ + FUNC100 (NUM (x, 6)); \ + FUNC100 (NUM (x, 7)); \ + FUNC100 (NUM (x, 8)); \ + FUNC100 (NUM (x, 9)) + +#define FUNC7000() \ + FUNC1000 (1); \ + FUNC1000 (2); \ + FUNC1000 (3); \ + FUNC1000 (4); \ + FUNC1000 (5); \ + FUNC1000 (6); \ + FUNC1000 (7); + +#ifdef FUNC +# undef FUNC +#endif + +#ifdef externnum +# define FUNC(x) extern int CONCAT (retNum, x) (void) +#endif + +#ifdef definenum +# define FUNC(x) int CONCAT (retNum, x) (void) { return x; } +#endif + +#ifdef callnum +# define FUNC(x) CONCAT (retNum, x) (); sync_all (x) +#endif + +FUNC7000 ();