From patchwork Mon Sep 7 22:24:29 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella X-Patchwork-Id: 53251 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-wi0-f200.google.com (mail-wi0-f200.google.com [209.85.212.200]) by patches.linaro.org (Postfix) with ESMTPS id 52B7E22B05 for ; Mon, 7 Sep 2015 22:24:47 +0000 (UTC) Received: by wicmn1 with SMTP id mn1sf30119136wic.1 for ; Mon, 07 Sep 2015 15:24:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mailing-list:precedence:list-id :list-unsubscribe:list-subscribe:list-archive:list-post:list-help :sender:delivered-to:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-type :content-transfer-encoding:x-original-sender :x-original-authentication-results; bh=b5iz3Qx9SBPjq9UwrJscprhT6TNw36VCaSg/1LwN734=; b=OZc0YWLb8iQRvyasKBi5PPGp5ZPBfwAQBmgu/SCuEcD8mMp/W6KTgOfxp6shcdlXF8 b8JtUByCBsG512S1cscIGGrbslKnPTmwuqRWWrGTtFZoRMFKk6r0eU/4FSPFUW07wgRJ E4p7HpBkh2+tU5BUdYRMDzI2OPj+AJHY1PM/JVDZIxkRwxn3PtdG8MfbTo+KKiMEL4um jGSA0URS6f+vMPHe2ELx2bj8co1hxMzUZAEFWeNHaxOrLd54B/XKSbHu0Pu6A7ofu6QK 3Hk61omSEKmqXWUMrGyOERd99h2Vv4QnmkUu7sRTZ1q9Brk9oSGpqA6Hklg4LvhcMza3 9DKg== X-Gm-Message-State: ALoCoQkE1cpjg7obdzfKhcr+52AF5qUrUM3ldg/FGHBhqUMJ+uQUNlHNE1v6v/6LjvsE6X7x403w X-Received: by 10.112.78.101 with SMTP id a5mr5717496lbx.9.1441664686387; Mon, 07 Sep 2015 15:24:46 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.9.230 with SMTP id d6ls104208lab.13.gmail; Mon, 07 Sep 2015 15:24:46 -0700 (PDT) X-Received: by 10.152.204.108 with SMTP id kx12mr11921038lac.20.1441664686161; Mon, 07 Sep 2015 15:24:46 -0700 (PDT) Received: from mail-la0-x233.google.com (mail-la0-x233.google.com. [2a00:1450:4010:c03::233]) by mx.google.com with ESMTPS id f5si1289048lbd.110.2015.09.07.15.24.46 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 07 Sep 2015 15:24:46 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 2a00:1450:4010:c03::233 as permitted sender) client-ip=2a00:1450:4010:c03::233; Received: by lanb10 with SMTP id b10so57728229lan.3 for ; Mon, 07 Sep 2015 15:24:46 -0700 (PDT) X-Received: by 10.112.168.100 with SMTP id zv4mr18967981lbb.117.1441664685961; Mon, 07 Sep 2015 15:24:45 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.59.35 with SMTP id w3csp609109lbq; Mon, 7 Sep 2015 15:24:44 -0700 (PDT) X-Received: by 10.68.133.167 with SMTP id pd7mr51562139pbb.23.1441664684777; Mon, 07 Sep 2015 15:24:44 -0700 (PDT) Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id qp7si1950638pbc.93.2015.09.07.15.24.43 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 07 Sep 2015 15:24:44 -0700 (PDT) Received-SPF: pass (google.com: domain of libc-alpha-return-63026-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Received: (qmail 94227 invoked by alias); 7 Sep 2015 22:24:34 -0000 Mailing-List: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org Precedence: list 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 94214 invoked by uid 89); 7 Sep 2015 22:24:34 -0000 X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=AWL, BAYES_50, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-yk0-f173.google.com X-Received: by 10.129.159.129 with SMTP id w123mr24090052ywg.117.1441664670400; Mon, 07 Sep 2015 15:24:30 -0700 (PDT) Subject: Re: [PATCH 08/08] nptl: arm: Fix Race conditions in pthread cancellation (BZ#12683) To: Phil Blundell References: <55E4C300.9080800@linaro.org> <1441646253.22688.58.camel@pbcl.net> Cc: GNU C Library From: Adhemerval Zanella Message-ID: <55EE0E9D.10702@linaro.org> Date: Mon, 7 Sep 2015 19:24:29 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <1441646253.22688.58.camel@pbcl.net> X-Original-Sender: adhemerval.zanella@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 2a00:1450:4010:c03::233 as permitted sender) smtp.mailfrom=patch+caf_=patchwork-forward=linaro.org@linaro.org; dkim=pass header.i=@sourceware.org X-Google-Group-Id: 836684582541 On 07-09-2015 14:17, Phil Blundell wrote: > On Mon, 2015-08-31 at 18:11 -0300, Adhemerval Zanella wrote: >> This patch adds the ARM modifications required for the BZ#12683 fix. >> It basically removes the enable_asynccancel/disable_asynccancel function >> usage on code, provide a arch-specific symbol that contains global >> markers to be used in SIGCANCEL handler. > > I looked at this in a bit more detail. Here are some further comments: > >> .fnstart; /* matched by the .fnend in UNDOARGS below. */ \ >> - DOCARGS_##args; /* save syscall args etc. around CENABLE. */ \ >> - CENABLE; \ >> - mov ip, r0; /* put mask in safe place. */ \ >> - UNDOCARGS_##args; /* restore syscall args. */ \ >> - ldr r7, =SYS_ify (syscall_name); \ >> - swi 0x0; /* do the call. */ \ >> - mov r7, r0; /* save syscall return value. */ \ >> - mov r0, ip; /* get mask back. */ \ >> - CDISABLE; \ >> - mov r0, r7; /* retrieve return value. */ \ >> - RESTORE_LR_##args; \ > > After this change, DOCARGS(), UNDOCARGS() and RESTORE_LR() are all dead > code. Please delete them. I noted it on your first reply and I already removed them in my branch. > >> - UNDOARGS_##args; \ >> + push {r4, r5, lr}; \ >> + .save {r4, r5, lr}; \ >> + PSEUDO_CANCEL_BEFORE; \ >> + movw r0, SYS_ify (syscall_name); \ > > This fails when compiling for non-thumb2. Please use "ldr r0, =..." > instead. > I will change it >> + PSEUDO_CANCEL_AFTER; \ >> + pop {r4, r5, pc}; \ > Indeed 'pc' seems wrong, I think it should be 'lr' instead. I will change it. > Popping {pc} here causes an immediate return, which means that the errno > handling code which follows is never executed. Somewhat embarrassingly > it seems that there is no existing glibc test which catches this > failure. I've attached a proof of concept which demonstrates it, but I > rather wonder whether we should extend the test harness so that > some/most of the existing glibc tests are run both single-threaded (as > now) and with an additional dummy thread created at startup in order to > force this code down the multi-threaded path. > > Also note that the two testcases in the attached patch give slightly > different results and I think they would continue to do so (in a > different way) if the bug above was fixed. It's not entirely clear to > me that this part of __syscall_cancel() from your other patch is > correct: > > + /* If cancellation is not enabled, call the syscall directly. */ > + if (pd->cancelhandling & CANCELSTATE_BITMASK) > + { > + INTERNAL_SYSCALL_DECL (err); > + result = INTERNAL_SYSCALL_NCS (nr, err, 6, a1, a2, a3, a4, a5, a6); > + return INTERNAL_SYSCALL_ERROR_P (result, err) ? -result : result; > + } > > p. > I will add your testcases on my patch to fix the NPLT tests for this mechanism [1]. Also the inline cancellation call is fact wrong: it should only negate the value depending if the architecture set the syscall return to be negate in case of a error (powerpc for instance). With these changes: I saw no regression on arm with both NPTL current tests and the ones you have added. I also checked on other ports (x86, i386, powerpc64, and aarch64). Thanks for the review. [1] https://sourceware.org/ml/libc-alpha/2015-08/msg01287.html diff --git a/nptl/libc-cancellation.c b/nptl/libc-cancellation.c index 5c7d357..9bb8716 100644 --- a/nptl/libc-cancellation.c +++ b/nptl/libc-cancellation.c @@ -50,7 +50,9 @@ __syscall_cancel (__syscall_arg_t nr, __syscall_arg_t a1, { INTERNAL_SYSCALL_DECL (err); result = INTERNAL_SYSCALL_NCS (nr, err, 6, a1, a2, a3, a4, a5, a6); - return INTERNAL_SYSCALL_ERROR_P (result, err) ? -result : result; + if (INTERNAL_SYSCALL_ERROR_P (result, err)) + return -INTERNAL_SYSCALL_ERRNO (result, err); + return result; } /* Call the arch-specific entry points that contains the globals markers diff --git a/sysdeps/unix/sysv/linux/arm/sysdep-cancel.h b/sysdeps/unix/sysv/linux/arm/sysdep-cancel.h index 9f03bb5..6ed3feb 100644 --- a/sysdeps/unix/sysv/linux/arm/sysdep-cancel.h +++ b/sysdeps/unix/sysv/linux/arm/sysdep-cancel.h @@ -54,9 +54,9 @@ push {r4, r5, lr}; \ .save {r4, r5, lr}; \ PSEUDO_CANCEL_BEFORE; \ - movw r0, SYS_ify (syscall_name); \ + ldr r0, =SYS_ify (syscall_name); \ PSEUDO_CANCEL_AFTER; \ - pop {r4, r5, pc}; \ + pop {r4, r5, lr}; \ .fnend; \ cmn r0, $4096