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: AS17314 8.43.84.0/22 X-Spam-Status: No, score=-3.7 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RDNS_DYNAMIC,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (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 017661F8C6 for ; Thu, 26 Aug 2021 10:41:33 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 3AA9B385843E for ; Thu, 26 Aug 2021 10:41:31 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3AA9B385843E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1629974491; bh=8LCqSQ/uku2NqfFiQNh5AHCTQ/hIpm+8T74oxivNCRc=; h=To:Subject:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=h52Nr7qzDv5G084CmY1oioXtGWmYlEwcrck6FD7MSJOm0Qpy1qjCZIzXgc6mGxlpp EddgxxUe+6fw1daO2FVoYLTRaFGych0EZtmXlwAGxqnpAtFHm4AvdakC6m37B5y+NF D5RMvl6kAdFTeuPThnTw8WuxdBB6jKtbCQThusvs= Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id A1FCC3858C27 for ; Thu, 26 Aug 2021 10:41:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A1FCC3858C27 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-466-z5otRz9sPh2jFuAShIwolg-1; Thu, 26 Aug 2021 06:41:09 -0400 X-MC-Unique: z5otRz9sPh2jFuAShIwolg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D186B100CEC0; Thu, 26 Aug 2021 10:41:08 +0000 (UTC) Received: from oldenburg.str.redhat.com (unknown [10.39.194.140]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8E78B60C0F; Thu, 26 Aug 2021 10:41:07 +0000 (UTC) To: Adhemerval Zanella Subject: Re: [PATCH v2 04/19] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951) References: <20210823195047.543237-1-adhemerval.zanella@linaro.org> <20210823195047.543237-5-adhemerval.zanella@linaro.org> Date: Thu, 26 Aug 2021 12:41:05 +0200 In-Reply-To: <20210823195047.543237-5-adhemerval.zanella@linaro.org> (Adhemerval Zanella's message of "Mon, 23 Aug 2021 16:50:32 -0300") Message-ID: <87y28o7ake.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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: Florian Weimer via Libc-alpha Reply-To: Florian Weimer Cc: libc-alpha@sourceware.org Errors-To: libc-alpha-bounces+e=80x24.org@sourceware.org Sender: "Libc-alpha" * 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: =E2=80=9CIn the exit phase of the wrapper function for the thread = start routine (reached =E2=80=A6=E2=80=9D > 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: =E2=80=9Cthe thread itself is responsible for arranging the deallocation of any resource=E2=80=9D (detached threads cannot immediately deallocate themselves) > resource, otherwise the threads needs to be joined. =E2=80=9Cthe thread[] needs to be joined=E2=80=9D > 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. > 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. > 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. > 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_hidd= en; > static inline bool > __nptl_stack_in_use (struct pthread *pd) > { > - return pd->tid <=3D 0; > + return atomic_load_relaxed (&pd->joinstate) =3D=3D THREAD_STATE_EXITED= ; > } Maybe this should be an acquire load, to synchronize with the thread exit as communicated by the kernel? > 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 =3D (volatile struct pthread *) th; > =20 > /* Make sure the descriptor is valid. */ > - if (INVALID_TD_P (pd)) > + int state =3D atomic_load_acquire (&pd->joinstate); > + if (state =3D=3D THREAD_STATE_EXITED || state =3D=3D THREAD_STATE_EXIT= ING) > /* 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 =3D pd->setup_failed =3D=3D 1; > if (setup_failed) > -=09pd->joinid =3D NULL; > +=09pd->joinstate =3D THREAD_STATE_JOINABLE; > =20 > /* And give it up right away. */ > lll_unlock (pd->lock, LLL_PRIVATE); > =20 > if (setup_failed) > -=09goto out; > +=09{ > +=09 pd->tid =3D 0; > +=09 goto out; > +=09} > } What's the advantage of setting pd->tid here and below in start_thread? 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); > =20 > - 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 =E2=80=9Cwait [for] the thread to terminate=E2=80=9D > + set here is set so a pthread_join() wait until all the required cle= anup > + steps are done. > + > + The 'joinstate' field will be used to determine who is responsible = to > + call __nptl_free_tcb below. */ > + > + unsigned int joinstate =3D THREAD_STATE_JOINABLE; > + atomic_compare_exchange_weak_acquire (&pd->joinstate, &joinstate, > +=09=09=09=09=09THREAD_STATE_EXITING); I think you need a strong CAS here. We don't have, so you'll have to add a loop. > 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 =3D THREAD_STATE_JOINABLE; > + if (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &curstate, > +=09=09=09=09=09 THREAD_STATE_DETACHED)) This should be a strong CAS, so this needs a loop, I think. > 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 > =20 > -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% s= ure 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 i= n this > + situation they will deadlock. It is the programmer's problem to figu= re > + 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 =3D THREAD_SELF; > - atomic_compare_exchange_weak_acquire (&arg, &self, NULL); > + return ((pd =3D=3D self > +=09 || (atomic_load_acquire (&self->joinstate) =3D=3D THREAD_STATE_DET= ACHED > +=09 && (pd->cancelhandling > +=09=09 & (CANCELED_BITMASK | EXITING_BITMASK > +=09=09 | TERMINATED_BITMASK)) =3D=3D 0)) > +=09 && !(self->cancelstate =3D=3D PTHREAD_CANCEL_ENABLE > +=09=09&& (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK > +=09=09=09=09=09 | TERMINATED_BITMASK)) > +=09=09=3D=3D CANCELED_BITMASK)); > } The state argument to check_for_deadlock appears to be unused. > + /* pthread_join() is a cancellation entrypoint and we use the same > + rationale for pthread_timedjoin_np(). > + > +=09 The kernel notifies a process which uses CLONE_CHILD_CLEARTID via > +=09 a memory zerouing and futex wake up when the process terminates. =E2=80=9Cmemory zero[]ing and futex wake[-]up=E2=80=9D > #if __TIMESIZE =3D=3D 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 =3D (struct pthread *) threadid; > - if (pd->tid !=3D 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_EXI= TING) > + might also result in a possible blocking call: a detached thread mi= ght > + 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. > + struct pthread *pd =3D (struct pthread *) threadid; > + return atomic_load_acquire (&pd->joinstate) !=3D THREAD_STATE_EXITED > +=09 ? EBUSY > +=09 : __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) =3D=3D 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? Thanks, Florian