From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS3215 2.6.0.0/16 X-Spam-Status: No, score=-5.5 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI,NICE_REPLY_A, RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 8143B1F8C6 for ; Thu, 26 Aug 2021 14:58:49 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 82A1D3858007 for ; Thu, 26 Aug 2021 14:58:48 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 82A1D3858007 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1629989928; bh=QZR1Wywwyn8QNSdcOszFii6Ty0kU93aK3BVPDwRoKq0=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=m9Gj2g+Nqj0mD2a2Uz/vUnzeErf39x2rMUOPl5Tx2uq4Ws+Ws6HBzK23cAkZhioSR za/vS89kMFeblKBmMy0HhX+TVOz350BA7aK7/1Dv80nUcjS8++UO18bsxf0+2u6AXI 0OeZskftpgvLfUAyPHZHMmI6J8tDKsAx+kRLgCYY= Received: from mail-qt1-x831.google.com (mail-qt1-x831.google.com [IPv6:2607:f8b0:4864:20::831]) by sourceware.org (Postfix) with ESMTPS id 3A8963858402 for ; Thu, 26 Aug 2021 14:58:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 3A8963858402 Received: by mail-qt1-x831.google.com with SMTP id c19so2668472qte.7 for ; Thu, 26 Aug 2021 07:58:19 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=QZR1Wywwyn8QNSdcOszFii6Ty0kU93aK3BVPDwRoKq0=; b=hetTIx19UhKCPtHKXmxj7VKWoqYCRSUEtRKJQHojweWFCUYFWsTEbbjBqbGwaUPWCS hlAkE4RyPB+iXS5t4MpAH+dwrII8gwsDD0vradXAknDDinm8rsGu4EPC3vxDSz3nWwBO XGBkqq5CG6jZehrYExBF679I4i9JNYn1TGK2xEAb9pJ8GxA9v5aVmzRtlm/MM2kT8gEH QHn1+/R+qEimNPwxhyIhctzSDepeRHPUSTPpkHtgW1scSnxUoPP5MSPmjaFr0wKphklP 3kHBTvFoQJWR0yDwEwJdIHi73zgkkJbpkrFPVp7muJo2FMIHp/rwnP0i6ecoUdDzWBgn Q3Yg== X-Gm-Message-State: AOAM533R0YDOMXvvc2R7p+xMVPWVupeXC4LkDp/IWamSWMRWgCJrIrwk VY9AZij3yBwDTeylOVJ+q9k8meCwAc3jMQ== X-Google-Smtp-Source: ABdhPJyCh8sDeQOVaCEiCxWq6K7mjSXJ71PL7xKCaC9IxZNuTZR5AUQaM9Ba9C1XlrXWVc09mVMa7A== X-Received: by 2002:a05:622a:254:: with SMTP id c20mr3668496qtx.213.1629989897090; Thu, 26 Aug 2021 07:58:17 -0700 (PDT) Received: from ?IPv6:2804:431:c7ca:1a68:4e82:9e7c:b7e4:7f3? ([2804:431:c7ca:1a68:4e82:9e7c:b7e4:7f3]) by smtp.gmail.com with ESMTPSA id s28sm2740327qkm.43.2021.08.26.07.58.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 26 Aug 2021 07:58:16 -0700 (PDT) Subject: Re: [PATCH v2 04/19] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951) To: Florian Weimer References: <20210823195047.543237-1-adhemerval.zanella@linaro.org> <20210823195047.543237-5-adhemerval.zanella@linaro.org> <87y28o7ake.fsf@oldenburg.str.redhat.com> Message-ID: <1f026797-7be8-bf9d-323f-6ac7539752a1@linaro.org> Date: Thu, 26 Aug 2021 11:58:14 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <87y28o7ake.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Adhemerval Zanella via Libc-alpha Reply-To: Adhemerval Zanella Cc: libc-alpha@sourceware.org Errors-To: libc-alpha-bounces+e=80x24.org@sourceware.org Sender: "Libc-alpha" On 26/08/2021 07:41, Florian Weimer wrote: > * Adhemerval Zanella: > >> The use after free described in BZ#19951 is due the use of two different >> PD fields, 'joinid' and 'cancelhandling', to describe the thread state >> to synchronize the calls of pthread_join(), pthread_detach(), >> pthread_exit(), and normal thread exit. >> >> And any state change potentially requires to check for both fields >> atomically to handle partial state (such as pthread_join() with a >> cancellation handler to issue a 'joinstate' field rollback). >> >> This patch uses a different PD member with 4 possible states (JOINABLE, >> DETACHED, EXITING, and EXITED) instead of pthread 'tid' field: >> >> 1. On pthread_create() the inital state is set either to JOINABLE or >> DETACHED depending of the pthread attribute used. >> >> 2. On pthread_detach(), a CAS is issued on the state. If the CAS >> fails it means that thread is already detached (DETACHED) or is >> being terminated (EXITING). For former an EINVAL is returned, >> while for latter pthread_detach() should be reponsible to join the >> thread (and deallocate any internal resources). >> >> 3. On pthread_create() exit phase (reached either if the thread > > Suggest: “In the exit phase of the wrapper function for the thread start > routine (reached …” Ack. > >> function has returned, pthread_exit() has being called, or >> cancellation handled has been acted upon) we issue a CAS on state >> to set to EXITING mode. If the thread is previously on DETACHED >> mode it will be the thread itself responsible to deallocate any > > Suggest: “the thread itself is responsible for arranging the > deallocation of any resource” > > (detached threads cannot immediately deallocate themselves) Ack. > >> resource, otherwise the threads needs to be joined. > > “the thread[] needs to be joined” Ack. > > >> 4. The clear_tid_field on 'clone' call is changed to set the new >> 'state' field on thread exit (EXITED). This state ins only >> reached at thread termination. >> >> 4. The pthread_join() implementation is now simpler: the futex wait >> is done directly on thread state and there is no need to reset it >> in case of timeout (since the state is now set either by >> pthread_detach() or by the kernel on process termination). > > Duplicate 4. Ack. > >> The race condition on pthread_detach is avoided with only one atomic >> operation on PD state: once the mode is set to THREAD_STATE_DETACHED >> it is up to thread itself to deallocate its memory (done on the exit >> phase at pthread_create()). > > See above regarding thread self-deallocation. > > The design as described above looks sound to me, those are just nits. Right, should I change this paragraph as well (it is not clear the suggestion here). > >> This change trigger an invalid C11 thread tests: it crates a thread, >> which detaches itself, and after a timeout the creating thread checks >> if the join fails. The issue is once thrd_join() is called the thread >> lifetime has already expired. The test is changed so the sleep is done >> by the thread itself, so the creating thread will try to join a valid >> thread. > > This could be a separate commit. Ack. > >> diff --git a/nptl/nptl-stack.h b/nptl/nptl-stack.h >> index 8bcfde7ec5..c6b2b3078f 100644 >> --- a/nptl/nptl-stack.h >> +++ b/nptl/nptl-stack.h >> @@ -32,7 +32,7 @@ extern size_t __nptl_stack_cache_maxsize attribute_hidden; >> static inline bool >> __nptl_stack_in_use (struct pthread *pd) >> { >> - return pd->tid <= 0; >> + return atomic_load_relaxed (&pd->joinstate) == THREAD_STATE_EXITED; >> } > > Maybe this should be an acquire load, to synchronize with the thread > exit as communicated by the kernel? It seems ok, I also added the comment: /* It synchronize with the thread exit as communicated by the kernel with set_tid_address set at clone(). */ > >> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c >> index 38f637c5cf..67e00ef007 100644 >> --- a/nptl/pthread_cancel.c >> +++ b/nptl/pthread_cancel.c >> @@ -63,7 +63,8 @@ __pthread_cancel (pthread_t th) >> volatile struct pthread *pd = (volatile struct pthread *) th; >> >> /* Make sure the descriptor is valid. */ >> - if (INVALID_TD_P (pd)) >> + int state = atomic_load_acquire (&pd->joinstate); >> + if (state == THREAD_STATE_EXITED || state == THREAD_STATE_EXITING) >> /* Not a valid thread handle. */ >> return ESRCH; > > This is fixed in patch 08/19, so it's okay. > >> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c >> index 08e5189ad6..763e32bc3e 100644 >> --- a/nptl/pthread_create.c >> +++ b/nptl/pthread_create.c >> @@ -286,7 +286,7 @@ static int create_thread (struct pthread *pd, >> const struct >> @@ -351,13 +351,16 @@ start_thread (void *arg) >> and free any resource prior return to the pthread_create caller. */ >> setup_failed = pd->setup_failed == 1; >> if (setup_failed) >> - pd->joinid = NULL; >> + pd->joinstate = THREAD_STATE_JOINABLE; >> >> /* And give it up right away. */ >> lll_unlock (pd->lock, LLL_PRIVATE); >> >> if (setup_failed) >> - goto out; >> + { >> + pd->tid = 0; >> + goto out; >> + } >> } > > What's the advantage of setting pd->tid here and below in start_thread? We don't really need to clear the tid on setup_failed case in fact, since in this case no pthread_t will be returned to caller. I remove it. > > It destroys information that could be useful for debugging purposes, > answering the question what the TID was before the thread exited. > >> @@ -481,9 +484,20 @@ start_thread (void *arg) >> the breakpoint reports TD_THR_RUN state rather than TD_THR_ZOMBIE. */ >> atomic_bit_set (&pd->cancelhandling, EXITING_BIT); >> >> - if (__glibc_unlikely (atomic_decrement_and_test (&__nptl_nthreads))) >> - /* This was the last thread. */ >> - exit (0); >> + >> + /* CONCURRENCY NOTES: >> + >> + Concurrent pthread_detach() will either set state to >> + THREAD_STATE_DETACHED or wait the thread to terminate. The exiting state > > “wait [for] the thread to terminate” Ack. > >> + set here is set so a pthread_join() wait until all the required cleanup >> + steps are done. >> + >> + The 'joinstate' field will be used to determine who is responsible to >> + call __nptl_free_tcb below. */ >> + >> + unsigned int joinstate = THREAD_STATE_JOINABLE; >> + atomic_compare_exchange_weak_acquire (&pd->joinstate, &joinstate, >> + THREAD_STATE_EXITING); > > I think you need a strong CAS here. We don't have, so you'll have to > add a loop. Yeah, it seems right. I changed to: unsigned int prevstate; while (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &prevstate, THREAD_STATE_EXITING)) prevstate = atomic_load_relaxed (&pd->joinstate); > >> diff --git a/nptl/pthread_detach.c b/nptl/pthread_detach.c >> index ac50db9b0e..97d292cab8 100644 >> --- a/nptl/pthread_detach.c >> +++ b/nptl/pthread_detach.c >> @@ -26,32 +26,24 @@ ___pthread_detach (pthread_t th) > >> + For the case the thread is being terminated (THREAD_STATE_EXITING), >> + pthread_detach() will responsible to clean up the stack. */ >> + >> + int curstate = THREAD_STATE_JOINABLE; >> + if (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &curstate, >> + THREAD_STATE_DETACHED)) > > This should be a strong CAS, so this needs a loop, I think. I changed to: unsigned int prevstate = atomic_load_relaxed (&pd->joinstate); do { if (prevstate != THREAD_STATE_JOINABLE) return EINVAL; return __pthread_join (th, 0); } while (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &prevstate, THREAD_STATE_DETACHED)); > >> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c >> index 7303069316..428ed35101 100644 >> --- a/nptl/pthread_join_common.c >> +++ b/nptl/pthread_join_common.c >> @@ -22,113 +22,73 @@ >> #include >> #include >> >> -static void >> -cleanup (void *arg) >> +/* Check for a possible deadlock situation where the threads are waiting for >> + each other to finish. Note that this is a "may" error. To be 100% sure we >> + catch this error we would have to lock the data structures but it is not >> + necessary. In the unlikely case that two threads are really caught in this >> + situation they will deadlock. It is the programmer's problem to figure >> + this out. */ >> +static inline bool >> +check_for_deadlock (int state, struct pthread *pd) >> { >> - /* If we already changed the waiter ID, reset it. The call cannot >> - fail for any reason but the thread not having done that yet so >> - there is no reason for a loop. */ >> struct pthread *self = THREAD_SELF; >> - atomic_compare_exchange_weak_acquire (&arg, &self, NULL); >> + return ((pd == self >> + || (atomic_load_acquire (&self->joinstate) == THREAD_STATE_DETACHED >> + && (pd->cancelhandling >> + & (CANCELED_BITMASK | EXITING_BITMASK >> + | TERMINATED_BITMASK)) == 0)) >> + && !(self->cancelstate == PTHREAD_CANCEL_ENABLE >> + && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK >> + | TERMINATED_BITMASK)) >> + == CANCELED_BITMASK)); >> } > > The state argument to check_for_deadlock appears to be unused. I removed it. > >> + /* pthread_join() is a cancellation entrypoint and we use the same >> + rationale for pthread_timedjoin_np(). >> + >> + The kernel notifies a process which uses CLONE_CHILD_CLEARTID via >> + a memory zerouing and futex wake up when the process terminates. > > “memory zero[]ing and futex wake[-]up” Ack. > >> #if __TIMESIZE == 64 >> diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c >> index fd938e8780..726f2abc68 100644 >> --- a/nptl/pthread_tryjoin.c >> +++ b/nptl/pthread_tryjoin.c >> @@ -22,15 +22,17 @@ >> int >> __pthread_tryjoin_np (pthread_t threadid, void **thread_return) >> { >> - /* Return right away if the thread hasn't terminated yet. */ >> - struct pthread *pd = (struct pthread *) threadid; >> - if (pd->tid != 0) >> - return EBUSY; >> + /* The joinable state (THREAD_STATE_JOINABLE) is straigthforward since the >> + thread hasn't finished yet and trying to join might block. >> + Both detached (THREAD_STATE_DETACHED) and exiting (THREAD_STATE_EXITING) >> + might also result in a possible blocking call: a detached thread might >> + change its state to exiting and a exiting thread my take some time to >> + exit (and thus let the kernel set the state to THREAD_STATE_EXITED). */ > > pthread_tryjoin_np on a thread which is THREAD_STATE_DETACHED is > invalid, so that case doesn't matter, I think. I changed the comment to: /* The joinable state (THREAD_STATE_JOINABLE) is straigthforward since the thread hasn't finished yet and trying to join might block. The exiting thread (THREAD_STATE_EXITING) also mgith result in ablocking call: a detached thread might change its state to exiting and a exiting thread my take some time to exit (and thus let the kernel set the state to THREAD_STATE_EXITED). */ > >> + struct pthread *pd = (struct pthread *) threadid; >> + return atomic_load_acquire (&pd->joinstate) != THREAD_STATE_EXITED >> + ? EBUSY >> + : __pthread_clockjoin_ex (threadid, thread_return, 0, NULL); >> } > > But the code is correct, I believe. > >> diff --git a/sysdeps/pthread/tst-thrd-detach.c b/sysdeps/pthread/tst-thrd-detach.c >> index c844767748..e1906a0e10 100644 >> --- a/sysdeps/pthread/tst-thrd-detach.c >> +++ b/sysdeps/pthread/tst-thrd-detach.c > >> - if (thrd_join (id, NULL) == thrd_success) >> - FAIL_EXIT1 ("thrd_join succeed where it should fail"); >> + TEST_COMPARE (thrd_join (id, NULL), thrd_error); > > This is still a user-after-free bug, right? Indeed, I think it would be better to just remove this test.