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 Netto X-Patchwork-Id: 8596 Received: (qmail 94227 invoked by alias); 7 Sep 2015 22:24:34 -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 94214 invoked by uid 89); 7 Sep 2015 22:24:34 -0000 Authentication-Results: sourceware.org; auth=none 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-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-type :content-transfer-encoding; bh=b5iz3Qx9SBPjq9UwrJscprhT6TNw36VCaSg/1LwN734=; b=dCF1RAWYyeRI6Qdn38heHQgEqBWO+yQ4eUPZtzr+wl3xLl9kJ+owtUFt57R9TAMDR7 kDvf6oeV7xhzSZMfPU4m+nNteqVcV32fwipVAp72JkIkEn814Nx4OJJ8YEJw6dp7nyZF srfE5HUvT6fvXTiizIlGrS3TVoO3SC+L8+slh3wYC1IFbEj3zfXzXzBQAutwkKx6Tgi5 KYodjiT7CbapUIkTtEmTMmTaGeHz5d0jR2PHJ+WOvSubJdapzxaumdl9sa4kE7+cMEf0 Y2NxufWfPFnKIKAJtlBwmpKcy4tNSwppRmD2TqFZ/11NqRbiyZkZH2VR+HLDd4BOuTw7 rq6Q== X-Gm-Message-State: ALoCoQkDOS597XIie/xVwVBM16HsRVNnuKOsMKYsVFTs9mEGkLWk2SmGuEVIxjPu5gXgTPJ1Iogf 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> 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