From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22d.google.com (mail-oi1-x22d.google.com [IPv6:2607:f8b0:4864:20::22d]) by sourceware.org (Postfix) with ESMTPS id 4E7CA3856DE8 for ; Thu, 14 Apr 2022 15:50:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4E7CA3856DE8 Received: by mail-oi1-x22d.google.com with SMTP id k10so5872889oia.0 for ; Thu, 14 Apr 2022 08:50:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=iGIGJyPE6evYgLYO8xncFsrAYU6BHzNl3eRmyOLyDF8=; b=XiTWvGCU8Z/ieKTpZMsrtrwXR9nqJ1qm07+wSjvWrD+wOMK0f04oJhC8ixI5SPdKD1 g8kV1Lc6c71yhDFxcFRDUWRSdx6seL3x2DTAbuVkki3aYFoslqkbKRfEYoT0fuL5fUf7 /jNCemAhDEU1cFFcYFJ0sWvEDH9L0Y7/QwwsPff9dLeDU/BQh0Dy2B3/eY+y2D2GB/ui HzhI2d3IHAlvM/g5hHY2SBaT/LEuGRQ1EZoSaoHWyYCo7wMig5/ItcsBqWaVFotVdhs4 SGODi0AU5NPiGdPjSomRNHvk9Bg3dQqZkmuQEn3Dgw4cEhtGkdFKFTvvDjyNBBY2L8Rq 7DQw== X-Gm-Message-State: AOAM531TG4EQU+jJo47dThMG3FwwRqlVChCPxzPASvXeVtp6/QqV1mnT LV7TJzhb71PaCI0BGDxQv4wdDsbYDmc3Ww== X-Google-Smtp-Source: ABdhPJzjk5CtYDWqtvO8fm2D3JiWk7Xx8OMIOynJ+wGDldTaIldvhRpgWoS9j/GDuA2lRVf3nUNGOQ== X-Received: by 2002:a05:6808:14c1:b0:2fa:72d6:5dfa with SMTP id f1-20020a05680814c100b002fa72d65dfamr1622936oiw.182.1649951401583; Thu, 14 Apr 2022 08:50:01 -0700 (PDT) Received: from ?IPV6:2804:431:c7ca:431f:3dc9:7133:8dac:5273? ([2804:431:c7ca:431f:3dc9:7133:8dac:5273]) by smtp.gmail.com with ESMTPSA id p16-20020a05680811d000b002d72ec3a921sm146992oiv.21.2022.04.14.08.50.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 14 Apr 2022 08:50:01 -0700 (PDT) Message-ID: <44695f1d-0a88-9557-9ffc-673b49b0c3b3@linaro.org> Date: Thu, 14 Apr 2022 12:49:58 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029) Content-Language: en-US To: Florian Weimer Cc: libc-alpha@sourceware.org, Aurelien Jarno References: <20220407132602.1689442-1-adhemerval.zanella@linaro.org> <87y20bk6vo.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella In-Reply-To: <87y20bk6vo.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: , X-List-Received-Date: Thu, 14 Apr 2022 15:50:03 -0000 On 11/04/2022 11:54, Florian Weimer wrote: > * Adhemerval Zanella: > >> @@ -56,10 +63,29 @@ __pthread_disable_asynccancel (int oldtype) >> { > >> + /* We cannot return when we are being canceled. Upon return the >> + thread might be things which would have to be undone. The >> + following loop should loop until the cancellation signal is >> + delivered. */ >> + while (__glibc_unlikely ((newval & (CANCELING_BITMASK | CANCELED_BITMASK)) >> + == CANCELING_BITMASK)) >> + { >> + futex_wait_simple ((unsigned int *) &self->cancelhandling, newval, >> + FUTEX_PRIVATE); >> + newval = atomic_load_relaxed (&self->cancelhandling); >> + } > > Is there no FUTEX_WAKE in the patch because futex_wait_simple already > fails with EINTR once the signal finally arrives? My understanding is it tries to handle interrupted syscall with side effects on thread with async cancellation enabled where pthread_cancel was issued, but SIGCANCEL has not yet arrived. In this case the cancellation handler will handle thread cancellation. This restores the previous semantic, which I think it not fully correct (the old BZ#12683). > >> diff --git a/nptl/descr.h b/nptl/descr.h >> index ea8aca08e6..58e3849a63 100644 >> --- a/nptl/descr.h >> +++ b/nptl/descr.h >> @@ -279,6 +279,15 @@ struct pthread >> >> /* Flags determining processing of cancellation. */ >> int cancelhandling; >> + /* Bit set if cancellation is disabled. */ >> +#define CANCELSTATE_BIT 0 >> +#define CANCELSTATE_BITMASK (0x01 << CANCELSTATE_BIT) >> + /* Bit set if asynchronous cancellation mode is selected. */ >> +#define CANCELTYPE_BIT 1 >> +#define CANCELTYPE_BITMASK (0x01 << CANCELTYPE_BIT) >> + /* Bit set if canceling has been initiated. */ >> +#define CANCELING_BIT 2 >> +#define CANCELING_BITMASK (0x01 << CANCELING_BIT) >> /* Bit set if canceled. */ >> #define CANCELED_BIT 3 >> #define CANCELED_BITMASK (0x01 << CANCELED_BIT) > > 0x01 looks like a shifted bit pattern. Maybe a plain 1 is enough? Ack. > > Maybe add a reference to the pthread_cancel comment explaining the need > for this? Right, I will extend the comment below to add why we need both the cancellation type and mode to be modified atomically along cancellation status. > >> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c >> index 7524c7ce4d..5aad1bab78 100644 >> --- a/nptl/pthread_cancel.c >> +++ b/nptl/pthread_cancel.c > >> @@ -92,29 +103,66 @@ __pthread_cancel (pthread_t th) > >> + /* Some syscalls are never restarted after being interrupted by a signal >> + handler, regardless of the use of SA_RESTART (they always fail with >> + EINTR). So pthread_cancel can not send SIGCANCEL unless the cancellation > > typo: can[]not > >> + is enabled and set as asynchronous (in this case the cancellation will >> + be acted in the cancellation handler instead by the syscall wrapper). >> + Otherwise the target thread is set as 'cancelling' (CANCELING_BITMASK) >> + by atomically set 'cancelhandling' and the cancelation will be acted > > typo: atomically set[ting] > >> + uppon on next cancellation entrypoing in the target thread. */ > > typos: up[]on, cancellation [point] Ack. > > Test looks okay to me. > > Reviewed-by: Florian Weimer > > Thanks, > Florian >