From patchwork Fri Apr 15 20:07:39 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 65971 Delivered-To: patch@linaro.org Received: by 10.140.93.198 with SMTP id d64csp122418qge; Fri, 15 Apr 2016 13:08:17 -0700 (PDT) X-Received: by 10.66.144.4 with SMTP id si4mr32147862pab.0.1460750897733; Fri, 15 Apr 2016 13:08:17 -0700 (PDT) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id z75si4060495pfi.48.2016.04.15.13.08.17 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 15 Apr 2016 13:08:17 -0700 (PDT) Received-SPF: pass (google.com: domain of libc-alpha-return-68928-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@sourceware.org; spf=pass (google.com: domain of libc-alpha-return-68928-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=libc-alpha-return-68928-patch=linaro.org@sourceware.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org 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:message-id; q=dns; s= default; b=kvDqM7Q0d08dXxCukbFTeRSHWgkZrlItZH0a65EbU89te8Pbutbte SlTiNRICJUtbPGiHUMSGK8P3wEux3THc7Wa7HsaMkCXiQG1ck27UfHro8oiVGkuC 6peKkmF/kRL2u33mJ2qVkGERTKQwIYeYq0wAA1vYvYnabnGw2r9GpA= 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:message-id; s=default; bh=t2DmGZnuO4ki0EtZJi0EAR47GEc=; b=R972jk9rZodACVHEKwxCcM16yr+A /IkwLA7EiYvHviiqqPtPDJIYY7AoqxH3AuaS7YsrlVWBdBLcxiycAPsj7mXRyYd3 1ed/pe6XNhPMXCORknmYYy7Z0UrNe0kGfen96IguIizifeVrF+m19uSdHGP0q4ZL KcJtkTf53ILBz4Y= Received: (qmail 3872 invoked by alias); 15 Apr 2016 20:08:00 -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 3447 invoked by uid 89); 15 Apr 2016 20:07:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=ite, zmw, ZMW, zmk X-HELO: mail-yw0-f175.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:subject:date:message-id; bh=YymLqzuxm+Bph9UhkG1M1OUeVwaDHqIBCE3kSyqD07U=; b=IRxhGQ1tvvtDXTbBdddNPnGluZT76dR3Oh/j6BPlifgof6x5E9X1nj31HlEW6ShJXn Y8YfbomTwPUw09zrrmno2EJFf2np7Kwwqb63J7OC6bx5SU5W8HagCXLMr2Ks5G+wQOzW 7EQ+mWcJ8xiqkpqB0ZSgwvZc1lBD3ggQAakCCMMTlRyAsNz6vhnwARnJNjOW3PM8lmbu 1MKKE8pvTmG3gqfXTleuw4gvw192FRShUcGGlbt/FAEUPFRtD2oWFtUsOzDKOnA7jF17 gvB3WZ1gVWONrhwIoMyDAdqsOuDSGKLVgtyVpKnHQt6T8VjcFWyibpM1TWpBObNStHS0 K1CA== X-Gm-Message-State: AOPr4FXtTegKIt9A56f7oz/fhiBMeaSH6gQXtWpp073/ZXVSyKdkr+nyM0gyk2noxjZ6L/bN X-Received: by 10.13.204.11 with SMTP id o11mr13380605ywd.52.1460750867067; Fri, 15 Apr 2016 13:07:47 -0700 (PDT) From: Adhemerval Zanella To: libc-alpha@sourceware.org Subject: [PATCH] Fix clone (CLONE_VM) pid/tid reset (BZ#19957) Date: Fri, 15 Apr 2016 17:07:39 -0300 Message-Id: <1460750859-5593-1-git-send-email-adhemerval.zanella@linaro.org> As discussed in libc-alpha [1] current clone with CLONE_VM (without CLONE_THREAD set) will reset the pthread pid/tid fields to -1. The issue is since memory is shared between the parent and chield it will clobber parent's cached pid/tid leading to internal inconsistencies if the value is not restore. And even it is restore it may lead to racy condition when between set/restore a thread might invoke pthread function that validate the pthread with INVALID_TD_P/INVALID_NOT_TERMINATED_TD_P and thus get wrong results due inconsistent internal pthread state. As stated in BZ19957, previously reports of this behaviour was close with EWONTFIX due the fact usage of clone outside glibc is tricky since glibc requires consistent internal pthread, while using clone directly may not provide it. However since now posix_spawn uses clone(CLONE_VM) to fixes various issues related to previous vfork usage this issue requires fixing. The vfork implementation also does something similar, but instead it negates and restores only the *pid* field and functions that might access its value know to handle such case (getpid, raise and pthread ones that uses INVALID_TD_P/INVALID_NOT_TERMINATED_TD_P macros that check only *tid* field). Also vfork does not call __clone directly, instead calling either __NR_vfork or __NR_clone directly. So this patch removes this clone behavior by avoiding setting the pthread pid/tid field for CLONE_VM as well for CLONE_THREAD. Instead of current approach of: int clone(int (*fn)(void *), void *child_stack, int flags, ...) [...] if (flags & CLONE_THREAD) goto do_syscall; pid_t new_value; if (flags & CLONE_VM) new_value = -1; else new_value = getpid (); THREAD_SETMEM (THREAD_SELF, pid, new_value); THREAD_SETMEM (THREAD_SELF, tid, new_value); do_syscall: [...] The new approach uses: int clone(int (*fn)(void *), void *child_stack, int flags, ...) [...] if (flags & CLONE_THREAD || flags & CLONE_VM) goto do_syscall; pid_t new_value = getpid (); THREAD_SETMEM (THREAD_SELF, pid, new_value); THREAD_SETMEM (THREAD_SELF, tid, new_value); do_syscall: [...] It also removes the linux tst-getpid2.c test which expects the previous behavior. I only fixed the clone.S implementations for i386, x86_64, powerpc, aarch64, and arm (basically the ones I have a way to actually test it). If would like to ask for some help on fixing the remaning ones: sparc (32 and 64-bit), alpha, s390 (32 and 64-bit), microblaze, sh, ia64, tile, hppa, m68k, nio2, and mips. Tested on x86_64, i686, x32, powerpc64le, aarch64, and armhf. [1] https://sourceware.org/ml/libc-alpha/2016-04/msg00307.html --- ChangeLog | 13 +++++++++++++ sysdeps/unix/sysv/linux/Makefile | 2 +- sysdeps/unix/sysv/linux/aarch64/clone.S | 8 +++++--- sysdeps/unix/sysv/linux/arm/clone.S | 7 +++---- sysdeps/unix/sysv/linux/i386/clone.S | 5 +---- sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S | 12 +++++++----- sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S | 10 ++++++---- sysdeps/unix/sysv/linux/tst-getpid2.c | 2 -- sysdeps/unix/sysv/linux/x86_64/clone.S | 7 ++----- 9 files changed, 38 insertions(+), 28 deletions(-) delete mode 100644 sysdeps/unix/sysv/linux/tst-getpid2.c -- 1.9.1 diff --git a/ChangeLog b/ChangeLog index 9eafe63..9d094a1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,16 @@ +2016-04-15 Adhemerval Zanella + + * sysdeps/unix/sysv/linux/Makefile [$(subdir) == nptl] (test): Remove + tst-getpid2. + * sysdeps/unix/sysv/linux/aarch64/clone.S (__clone): Do not change + pid/tid fields for CLONE_VM. + * sysdeps/unix/sysv/linux/arm/clone.S: Likewise. + * sysdeps/unix/sysv/linux/i386/clone.S: Likewise. + * sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S: Likewise. + * sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S: Likewise. + * sysdeps/unix/sysv/linux/x86_64/clone.S: Likewise. + * sysdeps/unix/sysv/linux/tst-getpid2.c: Remove file. + 2016-04-15 Mike Frysinger * locale/iso-4217.def: Add SSP and change ZMK to ZMW. diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index 9999600..6d52b97 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -194,7 +194,7 @@ CFLAGS-gai.c += -DNEED_NETLINK endif ifeq ($(subdir),nptl) -tests += tst-setgetname tst-align-clone tst-getpid1 tst-getpid2 \ +tests += tst-setgetname tst-align-clone tst-getpid1 \ tst-thread-affinity-pthread tst-thread-affinity-pthread2 \ tst-thread-affinity-sched endif diff --git a/sysdeps/unix/sysv/linux/aarch64/clone.S b/sysdeps/unix/sysv/linux/aarch64/clone.S index 8f3ab40..d95c990 100644 --- a/sysdeps/unix/sysv/linux/aarch64/clone.S +++ b/sysdeps/unix/sysv/linux/aarch64/clone.S @@ -72,9 +72,11 @@ thread_start: cfi_undefined (x30) mov x29, 0 - tbnz x11, #CLONE_THREAD_BIT, 3f - mov x0, #-1 - tbnz x11, #CLONE_VM_BIT, 2f + mov x0, CLONE_VM + movk x0, CLONE_THREAD>>16, lsl 16 + tst x0, x11 + bne 3f + mov x8, #SYS_ify(getpid) svc 0x0 2: diff --git a/sysdeps/unix/sysv/linux/arm/clone.S b/sysdeps/unix/sysv/linux/arm/clone.S index c103123..b3a875f 100644 --- a/sysdeps/unix/sysv/linux/arm/clone.S +++ b/sysdeps/unix/sysv/linux/arm/clone.S @@ -72,13 +72,12 @@ PSEUDO_END (__clone) .cantunwind tst ip, #CLONE_THREAD bne 3f + tst ip, #CLONE_VM + bne 3f GET_TLS (lr) mov r1, r0 - tst ip, #CLONE_VM ldr r7, =SYS_ify(getpid) - ite ne - movne r0, #-1 - swieq 0x0 + swi 0x0 NEGOFF_ADJ_BASE (r1, TID_OFFSET) str r0, NEGOFF_OFF1 (r1, TID_OFFSET) str r0, NEGOFF_OFF2 (r1, PID_OFFSET, TID_OFFSET) diff --git a/sysdeps/unix/sysv/linux/i386/clone.S b/sysdeps/unix/sysv/linux/i386/clone.S index ef447d1..64f0c9b 100644 --- a/sysdeps/unix/sysv/linux/i386/clone.S +++ b/sysdeps/unix/sysv/linux/i386/clone.S @@ -108,7 +108,7 @@ L(thread_start): cfi_undefined (eip); /* Note: %esi is zero. */ movl %esi,%ebp /* terminate the stack frame */ - testl $CLONE_THREAD, %edi + testl $(CLONE_THREAD | CLONE_VM), %edi je L(newpid) L(haspid): call *%ebx @@ -124,9 +124,6 @@ L(here): .subsection 2 L(newpid): - testl $CLONE_VM, %edi - movl $-1, %eax - jne L(nomoregetpid) movl $SYS_ify(getpid), %eax ENTER_KERNEL L(nomoregetpid): diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S b/sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S index eb973db..ec82ed6 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/clone.S @@ -76,11 +76,13 @@ ENTRY (__clone) crandc cr1*4+eq,cr1*4+eq,cr0*4+so bne- cr1,L(parent) /* The '-' is to minimise the race. */ - andis. r0,r28,CLONE_THREAD>>16 - bne+ r0,L(oldpid) - andi. r0,r28,CLONE_VM - li r3,-1 - bne- r0,L(nomoregetpid) + /* If CLONE_THREAD of CLONE_VM does not update the pid/tid field. */ + lis r0,CLONE_THREAD>>16 + ori r0,r0,CLONE_VM + and r0,r0,r29 + cmpwi r0,0 + bne+ cr0,L(oldpid) + DO_CALL(SYS_ify(getpid)) L(nomoregetpid): stw r3,TID(r2) diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S index 959fdb7..a8dfce6 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S @@ -78,11 +78,13 @@ ENTRY (__clone) crandc cr1*4+eq,cr1*4+eq,cr0*4+so bne- cr1,L(parent) /* The '-' is to minimise the race. */ - andis. r0,r29,CLONE_THREAD>>16 + /* If CLONE_THREAD of CLONE_VM does not update the pid/tid field. */ + lis r0,CLONE_THREAD>>16 + ori r0,r0,CLONE_VM + and r0,r0,r29 + cmpwi r0,0 bne+ cr0,L(oldpid) - andi. r0,r29,CLONE_VM - li r3,-1 - bne- cr0,L(nomoregetpid) + DO_CALL(SYS_ify(getpid)) L(nomoregetpid): stw r3,TID(r13) diff --git a/sysdeps/unix/sysv/linux/tst-getpid2.c b/sysdeps/unix/sysv/linux/tst-getpid2.c deleted file mode 100644 index fc98cb6..0000000 --- a/sysdeps/unix/sysv/linux/tst-getpid2.c +++ /dev/null @@ -1,2 +0,0 @@ -#define TEST_CLONE_FLAGS CLONE_VM -#include "tst-getpid1.c" diff --git a/sysdeps/unix/sysv/linux/x86_64/clone.S b/sysdeps/unix/sysv/linux/x86_64/clone.S index 1a50957..d33e35e 100644 --- a/sysdeps/unix/sysv/linux/x86_64/clone.S +++ b/sysdeps/unix/sysv/linux/x86_64/clone.S @@ -92,14 +92,11 @@ L(thread_start): the outermost frame obviously. */ xorl %ebp, %ebp - testq $CLONE_THREAD, %rdi + andq $(CLONE_THREAD | CLONE_VM), %rdi jne 1f - testq $CLONE_VM, %rdi - movl $-1, %eax - jne 2f movl $SYS_ify(getpid), %eax syscall -2: movl %eax, %fs:PID + movl %eax, %fs:PID movl %eax, %fs:TID 1: