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.6 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_HI,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 8EFAF1F8C6 for ; Thu, 26 Aug 2021 16:16:37 +0000 (UTC) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 6B85C3857800 for ; Thu, 26 Aug 2021 16:16:36 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6B85C3857800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1629994596; bh=X53K1HrQiANwSpe66/L3jyYPImFdlNBSJOVbvo23q0U=; 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=fZ+gcu731sPBJXv8wdlKY9jXlTXUfNC7altL3HA53FtmdvAs6m8GY23Pkv737dnI6 adhppn9dWBYO2GzjEG2/zIblnhDmRwuqMOWj02j7DuQp/XWgDUkBJ3GuwVoYKB1bhM 4lK4dD84yEbXQ6rzQ4fyk/HaLs0LZDuDsabSqyFU= Received: from mail-qt1-x836.google.com (mail-qt1-x836.google.com [IPv6:2607:f8b0:4864:20::836]) by sourceware.org (Postfix) with ESMTPS id 000D23858D3C for ; Thu, 26 Aug 2021 16:16:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 000D23858D3C Received: by mail-qt1-x836.google.com with SMTP id e3so2897595qth.9 for ; Thu, 26 Aug 2021 09:16:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=X53K1HrQiANwSpe66/L3jyYPImFdlNBSJOVbvo23q0U=; b=qkQ8ZaQXlcB4Sfrv2dIcoJZLUrg3zIoDmLHpxS/IxPeUDEbfubTzJRsXJIHV6KZw7o NfET0vVmkVtY8YJfFe8Bp3HuITg+1B8p9Iq/pznjkxiwvE3HjfhBiyUkl+YXRfkjhACZ VEmMlE2iUmyH58TeV7ULrIrUqO/OSHue+rm9yVBneRWOqSYErFdXl1xFVYuyut/gbQHe XNSZyvvvhsyPd+dlqOPeUgoRz7PttokZSvdgJ+BoUwHa/MIInM866FZAV5GhuDr0HMbj Xv9+/GNkiC+s0dTZ0iFDaE53aaKh+3bFb0/CJXRlrMdgt7OzThCJpzaG8JJ+zNmYt3qi +btw== X-Gm-Message-State: AOAM532Mjxkk5XAY4gtWaSXsFZaBaTginJo53n5HvfmpjlkTqc3Yo4JH snhcqWzBPRPa3IE/U305nNJEidYd/Zm/Lw== X-Google-Smtp-Source: ABdhPJyTj/d/6175QKoo6XhvmhOTVuN0OUnRo3VL1Dxwc1OLfwnT8y2Gb/SpKR5XIe7EildixUoEdQ== X-Received: by 2002:ac8:734c:: with SMTP id q12mr4104487qtp.192.1629994567098; Thu, 26 Aug 2021 09:16:07 -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 j26sm2871803qki.26.2021.08.26.09.16.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 26 Aug 2021 09:16:06 -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> <1f026797-7be8-bf9d-323f-6ac7539752a1@linaro.org> <87zgt4454x.fsf@oldenburg.str.redhat.com> Message-ID: <8719cf82-e68a-402a-f019-174f5602251c@linaro.org> Date: Thu, 26 Aug 2021 13:16:04 -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: <87zgt4454x.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 12:06, Florian Weimer wrote: > * Adhemerval Zanella: > >>>> 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). > > Maybe “up to [the] thread itself to [trigger deallocation of] its memory”? Ack. > >>>> 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. > > What about the change in start_thread? > > The subsequent changes look at the tid member, but they could equally > well look at joinstate, I think. My understanding it we still need to clear the tid member to avoid the unintentional usage, since kernel will clear it anymore. For instance on pthread_kill() (or any other usage), there is still an windows where the joinstate test and the tid read might result in an invalid value (either a tid reuse or an invalid value). > > >>> 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); > > Isn't prevstate uninitialized? Why no do-while loop? It is, I have changed to: unsigned int prevstate; do prevstate = atomic_load_relaxed (&pd->joinstate); while (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &prevstate, THREAD_STATE_EXITING)); > >>> 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). */ > > Typo: mgith Ack. > > Rest looks okay to me. > >>>> 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. > > Agreed. > > Thanks, > Florian >