From patchwork Tue Oct 17 23:24:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mathieu Desnoyers X-Patchwork-Id: 827360 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-85988-incoming=patchwork.ozlabs.org@sourceware.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="Szerc2c9"; 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 3yGrsR0wZGz9ryv for ; Wed, 18 Oct 2017 10:25:46 +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:cc:subject:date:message-id; q=dns; s= default; b=tzkAKlPJWLaAQvnBTCV2QRgLveIrA5FwI1cnOtvgyejTMQjyU2c2I MT/kPzNpcX/hvTWrciaIV2SDJB909wyBupvtqWBXnhbNUMax7I7LbTzUui1wWSz+ jyeZvzYnbXl9kIHyqBqyPXNe0fOQbhcMSMqJiT7PndXhGQ9m3b95+I= 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:cc:subject:date:message-id; s=default; bh=bD+STHY2WACiMMh415tRkeIR/oQ=; b=Szerc2c9kVmdIliKuGkgd1yD6jDx IxdhEG6Zh4rbZCi7mUYGumOt77TnXO9cENwcJBBAIyf71Qxdl5KsxFRcSpx5CJ21 FTEZywCc2wZa1Ok6AFA4UDk1NirukmmEpUHIGYFrZFpsal+oFkxdygLK+BTeiN64 XeUV3/Wz58snPp4= Received: (qmail 3410 invoked by alias); 17 Oct 2017 23:25:40 -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 3386 invoked by uid 89); 17 Oct 2017 23:25:40 -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 autolearn=ham version=3.3.2 spammy= X-HELO: mail.efficios.com From: Mathieu Desnoyers To: "Carlos O'Donell" , Florian Weimer Cc: Mathieu Desnoyers , "Ben Maurer" , libc-alpha@sourceware.org Subject: [RFC PATCH glibc v2] pthread_setspecific: Provide signal-safety across keys Date: Tue, 17 Oct 2017 19:24:40 -0400 Message-Id: <20171017232440.5134-1-mathieu.desnoyers@efficios.com> The intent here is to provide signal-safety against callers to pthread_{get/set}specific that work on different pthread keys, without hurting performance of the normal use-cases that do not care about signal-safety. One thing to keep in mind is that callers of pthread_{get/set}specific on a given key can disable signals if they require signal-safety against operations on their key. The real problem in my situation (lttng-ust tracer) is having pthread_setspecific (invoked by the application) do memory allocation and update pointers without disabling signals when it needs to allocate "level2". This only happens the first time a key >= PTHREAD_KEY_2NDLEVEL_SIZE is encountered by a thread. If lttng-ust tracing inserted into a signal handler nests over this allocation, the application pthread key specific may be lost, and memory leaked. Also, use directly mmap rather than calloc to ensure signal-safety. Make just the memory allocation part of pthread_setspecific signal-safe. Given this is not required very often, it should not cause a significant overhead. Document those new non-POSIX async-signal-safety guarantees in pthread_getspecific and pthread_setspecific user documentation. Signed-off-by: Mathieu Desnoyers CC: "Carlos O'Donell" CC: Florian Weimer CC: "Ben Maurer" CC: libc-alpha@sourceware.org --- manual/threads.texi | 18 ++++++++++-------- nptl/pthread_create.c | 3 ++- nptl/pthread_setspecific.c | 24 +++++++++++++++++++++--- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/manual/threads.texi b/manual/threads.texi index 769d974d50..62504ecfa9 100644 --- a/manual/threads.texi +++ b/manual/threads.texi @@ -53,20 +53,22 @@ is it called during thread exit. @c pthread_getspecific ok Return the thread-specific data associated with @var{key} in the calling thread. +As a non-POSIX extension, @code{pthread_getspecific} is async-signal safe. @end deftypefun @deftypefun int pthread_setspecific (pthread_key_t @var{key}, const void *@var{value}) @standards{POSIX, pthread.h} -@safety{@prelim{}@mtsafe{}@asunsafe{@asucorrupt{} @ascuheap{}}@acunsafe{@acucorrupt{} @acsmem{}}} -@c pthread_setspecific @asucorrupt @ascuheap @acucorrupt @acsmem -@c a level2 block may be allocated by a signal handler after -@c another call already made a decision to allocate it, thus losing -@c the allocated value. the seq number is updated before the -@c value, which might cause an earlier-generation value to seem -@c current if setspecific is cancelled or interrupted by a signal +@safety{@prelim{}@mtsafe{}@asunsafe{@asucorrupt{}}@acunsafe{@acucorrupt{} @acsmem{}}} +@c pthread_setspecific @asucorrupt @acucorrupt @acsmem +@c the seq number is updated before the value, which might cause +@c an earlier-generation value to seem current if setspecific is +@c cancelled or interrupted by a signal @c KEY_UNUSED ok -@c calloc dup @ascuheap @acsmem +@c dup @acsmem Associate the thread-specific @var{value} with @var{key} in the calling thread. +As a non-POSIX extension, @code{pthread_setspecific} is async-signal safe for +concurrent invocations against different @var{key}, but not async-signal +safe for concurrent invocations against the same @var{key}. @end deftypefun diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c index 992331e280..78abc07f91 100644 --- a/nptl/pthread_create.c +++ b/nptl/pthread_create.c @@ -33,6 +33,7 @@ #include #include #include "libioP.h" +#include #include @@ -327,7 +328,7 @@ __nptl_deallocate_tsd (void) { /* The first block is allocated as part of the thread descriptor. */ - free (level2); + (void) munmap (level2, PTHREAD_KEY_2NDLEVEL_SIZE * sizeof (*level2)); THREAD_SETMEM_NC (self, specific, cnt, NULL); } } diff --git a/nptl/pthread_setspecific.c b/nptl/pthread_setspecific.c index 214af3b661..2beb7f97fc 100644 --- a/nptl/pthread_setspecific.c +++ b/nptl/pthread_setspecific.c @@ -18,6 +18,7 @@ #include #include +#include #include "pthreadP.h" @@ -61,18 +62,35 @@ __pthread_setspecific (pthread_key_t key, const void *value) level2 = THREAD_GETMEM_NC (self, specific, idx1st); if (level2 == NULL) { + sigset_t ss, old_ss; + if (value == NULL) /* We don't have to do anything. The value would in any case be NULL. We can save the memory allocation. */ return 0; + /* Ensure that pthread_setspecific and pthread_getspecific are + signal-safe when used on different keys. */ + sigfillset (&ss); + pthread_sigmask (SIG_BLOCK, &ss, &old_ss); + /* Read level2 again with signals disabled. */ + level2 = THREAD_GETMEM_NC (self, specific, idx1st); + if (level2 != NULL) + goto skip_resize; + level2 - = (struct pthread_key_data *) calloc (PTHREAD_KEY_2NDLEVEL_SIZE, - sizeof (*level2)); - if (level2 == NULL) + = (struct pthread_key_data *) mmap (NULL, + PTHREAD_KEY_2NDLEVEL_SIZE * sizeof (*level2), + PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, + -1, 0); + if (level2 == MAP_FAILED) { + pthread_sigmask (SIG_SETMASK, &old_ss, NULL); return ENOMEM; + } THREAD_SETMEM_NC (self, specific, idx1st, level2); +skip_resize: + pthread_sigmask (SIG_SETMASK, &old_ss, NULL); } /* Pointer to the right array element. */