Message ID | 1490770778-11050-1-git-send-email-maninder1.s@samsung.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 85040 invoked by alias); 29 Mar 2017 07:00:41 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 82625 invoked by uid 89); 29 Mar 2017 07:00:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Singh, singh, H*r:Symantec, barriers X-HELO: mailout4.samsung.com From: Maninder Singh <maninder1.s@samsung.com> To: libc-alpha@sourceware.org, triegel@redhat.com, szabolcs.nagy@arm.com Cc: pankaj.m@samsung.com, ajeet.y@samsung.com, a.sahrawat@samsung.com, lalit.mohan@samsung.com, akhilesh.k@samsung.com, hakbong5.lee@samsung.com, Maninder Singh <maninder1.s@samsung.com>, Vaneet Narang <v.narang@samsung.com> Subject: [PATCH v2] dl-load: add memory barrier before updating the next Date: Wed, 29 Mar 2017 12:29:38 +0530 Message-id: <1490770778-11050-1-git-send-email-maninder1.s@samsung.com> X-CMS-MailID: 20170329070025epcas5p4644c528bf730c87d3c69e2ac3557d643 X-Msg-Generator: CA X-Sender-IP: 182.195.40.13 X-Local-Sender: =?UTF-8?B?TWFuaW5kZXIgU2luZ2gbU1JJLURlbGhpLVBsYXRmb3JtIFMv?= =?UTF-8?B?VyAxIFRlYW0b7IK87ISx7KCE7J6QG0xlYWQgRW5naW5lZXI=?= X-Global-Sender: =?UTF-8?B?TWFuaW5kZXIgU2luZ2gbU1JJLURlbGhpLVBsYXRmb3JtIFMv?= =?UTF-8?B?VyAxIFRlYW0bU2Ftc3VuZ8KgRWxlY3Ryb25pY3PCoBtMZWFkIEVuZ2luZWVy?= X-Sender-Code: =?UTF-8?B?QzEwG1NXQUhRG0MxMElEMDJJRDAyODExNQ==?= Content-type: text/plain; charset=utf-8 X-MTR: 20170329070025epcas5p4644c528bf730c87d3c69e2ac3557d643 X-EPHeader: CA CMS-TYPE: 105P DLP-Filter: Pass X-CFilter-Loop: Reflected X-Auth-Email: maninder1.s@samsung.com X-HopCount: 7 X-CMS-RootMailID: 20170329070025epcas5p4644c528bf730c87d3c69e2ac3557d643 X-RootMTR: 20170329070025epcas5p4644c528bf730c87d3c69e2ac3557d643 References: <CGME20170329070025epcas5p4644c528bf730c87d3c69e2ac3557d643@epcas5p4.samsung.com> |
Commit Message
Maninder Singh
March 29, 2017, 6:59 a.m. UTC
This patch adds C11 memory barrier before updating the liblist next. Issue Fix: race condition between add_name_to_object & _dl_name_match_p. One threads calling dlopen which further calls add_name_to_object & other thread trying to resolve RTLD_LAZY symbols through _dl_runtime_resolve which further calls. _dl_name_match_p checks if libname->next is valid, then it assumes libname->next->name to be valid. Also add_name_to_object initialized name first and then sets valid next pointer. This patch avoids any reorder of instruction when next is set before name to avoid any race. Signed-off-by: Maninder Singh <maninder1.s@samsung.com> Signed-off-by: Vaneet Narang <v.narang@samsung.com> --- v1 -> v2 use C11 atomics rather than direct memory barriers elf/dl-load.c | 2 +- elf/dl-misc.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Comments
On Wed, 2017-03-29 at 12:29 +0530, Maninder Singh wrote: > This patch adds C11 memory barrier before updating the liblist next. > > Issue Fix: race condition between add_name_to_object & _dl_name_match_p. > One threads calling dlopen which further calls add_name_to_object & > other thread trying to resolve RTLD_LAZY symbols through > _dl_runtime_resolve which further calls. > > _dl_name_match_p checks if libname->next is valid, then it assumes > libname->next->name to be valid. Also add_name_to_object initialized > name first and then sets valid next pointer. This patch is better, but it is still incomplete: We want to document concurrent code in sufficient detail so that other developers can understand the synchronization scheme without having to review all potentially interacting code first. (I'm aware that this isn't the case in several parts of glibc code, but we're trying to improve that.) This means documenting, in comments in the code, the synchronization scheme that is intended. In this particular case, this is this libname_list: * What concurrent operations can run on it? Apparently, search in _dl_name_match_p can be concurrent with insertion. Can insertion be concurrent with other insertion or not (if so, why)? * What are the important happens-before relationships we need to ensure? (For example, in this case, we need to ensure that if we see a list element in search, then all stores to the fields of list element happen before the search operation looks at the fields.) * What release operations and acquire operations are supposed to synchronize with each other. In your patch, it's these two. If we don't document this, it will be much harder for other developers to figure out what the necessary pairings are. You analyzed that already, so tell others about it. Documenting the code in this way is a good test to see if you have really analyzed the synchronization problem completely. (If one can't clearly explain how something is supposed to work, perhaps one doesn't yet fully understand it.) As noted on the Concurrency wiki page, we don't use atomic types, but use the atomic_* functions consistently as if they'd be required for atomic types. That is, the next field would be an atomic type, conceptually, and you should use atomic_load_relaxed for accesses to the next field in the list search of the insertion operation, for example. The only exception we make is for initializing stores. > This patch avoids any reorder of instruction when next is set before > name to avoid any race. > Signed-off-by: Maninder Singh <maninder1.s@samsung.com> > Signed-off-by: Vaneet Narang <v.narang@samsung.com> > --- > v1 -> v2 use C11 atomics rather than direct memory barriers > > elf/dl-load.c | 2 +- > elf/dl-misc.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/elf/dl-load.c b/elf/dl-load.c > index a5318f9..52df931 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -418,7 +418,7 @@ add_name_to_object (struct link_map *l, const char *name) > newname->name = memcpy (newname + 1, name, name_len); > newname->next = NULL; > newname->dont_free = 0; > - lastp->next = newname; > + atomic_store_release (&(lastp->next), newname); You can drop the inner parentheses. Space between name of the function that you call and its arguments (see the memcpy above). > } > > /* Standard search directories. */ > diff --git a/elf/dl-misc.c b/elf/dl-misc.c > index 1e9a6ee..8f48751 100644 > --- a/elf/dl-misc.c > +++ b/elf/dl-misc.c > @@ -295,7 +295,7 @@ _dl_name_match_p (const char *name, const struct link_map *map) > if (strcmp (name, runp->name) == 0) > return 1; > else > - runp = runp->next; > + runp = atomic_load_acquire(&(runp->next)); Likewise. > > return 0; > }
On 29/03/17 07:59, Maninder Singh wrote: > This patch adds C11 memory barrier before updating the liblist next. > > Issue Fix: race condition between add_name_to_object & _dl_name_match_p. > One threads calling dlopen which further calls add_name_to_object & > other thread trying to resolve RTLD_LAZY symbols through > _dl_runtime_resolve which further calls. > > _dl_name_match_p checks if libname->next is valid, then it assumes > libname->next->name to be valid. Also add_name_to_object initialized > name first and then sets valid next pointer. > > This patch avoids any reorder of instruction when next is set before > name to avoid any race. > > Signed-off-by: Maninder Singh <maninder1.s@samsung.com> > Signed-off-by: Vaneet Narang <v.narang@samsung.com> > --- > v1 -> v2 use C11 atomics rather than direct memory barriers > > elf/dl-load.c | 2 +- > elf/dl-misc.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) please report this bug in bugzilla and add the bug number reference to the changelog according to https://sourceware.org/glibc/wiki/Contribution%20checklist#Properly_Formatted_GNU_ChangeLog
diff --git a/elf/dl-load.c b/elf/dl-load.c index a5318f9..52df931 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -418,7 +418,7 @@ add_name_to_object (struct link_map *l, const char *name) newname->name = memcpy (newname + 1, name, name_len); newname->next = NULL; newname->dont_free = 0; - lastp->next = newname; + atomic_store_release (&(lastp->next), newname); } /* Standard search directories. */ diff --git a/elf/dl-misc.c b/elf/dl-misc.c index 1e9a6ee..8f48751 100644 --- a/elf/dl-misc.c +++ b/elf/dl-misc.c @@ -295,7 +295,7 @@ _dl_name_match_p (const char *name, const struct link_map *map) if (strcmp (name, runp->name) == 0) return 1; else - runp = runp->next; + runp = atomic_load_acquire(&(runp->next)); return 0; }