From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x736.google.com (mail-qk1-x736.google.com [IPv6:2607:f8b0:4864:20::736]) by sourceware.org (Postfix) with ESMTPS id AC10D3950C77 for ; Tue, 22 Jun 2021 18:30:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AC10D3950C77 Received: by mail-qk1-x736.google.com with SMTP id o6so6673998qkh.4 for ; Tue, 22 Jun 2021 11:30:23 -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:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=M74JhZOyQIGBtlXSkqU+ehkWgWK4FNscmuymUbiePs4=; b=FLQ8ZIFALjZgWMkOXvLrjkHUpDDuH2p3MwaVHW/QSMjr+2EuxrXCP13C5q9BMkhUM/ wpCKfgO9hucqh4E9MFNDUnD+qJj3oG10juYE3ix2cXqSoQqZpgSwIawRGzkVV9f7Nvgm 5BeiwQtvweHSyVhxRSOFwi++B1b82DMGIk+Tp0Y8Ph/xcX6F0ddH6qY3DsvRyyEVYT5u 4KhVmd3wyNzYWMszQRVRxhKZlDUMajn9AcU0EGo8r+FE64FEVtHcF+o130Sz46QNlQ20 ybwS6zz0esTmUBHuDb7S52FsBU+cmNiOeAdO0Fe43AsN5yr9GHg61GCTWLcYfjZ9/DLl OvxA== X-Gm-Message-State: AOAM532Bq9vFZl75lWrKEm0spnGr613RbzuR7HEUbSbpA+z1hcNVtOQw 0G5ReheZy1XRJiS1sLt1rBBLtiDW4CT1Hw== X-Google-Smtp-Source: ABdhPJw8AB9/kq51bPfRlgZbhtOwLK/K1QNatMQdWuqWtJ7Edu+PuzY8VuG5vqeUz1hbPrwQFPzzUg== X-Received: by 2002:a05:620a:307:: with SMTP id s7mr5596382qkm.213.1624386623117; Tue, 22 Jun 2021 11:30:23 -0700 (PDT) Received: from [192.168.1.108] ([177.194.59.218]) by smtp.gmail.com with ESMTPSA id s81sm13161623qka.82.2021.06.22.11.30.21 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 22 Jun 2021 11:30:22 -0700 (PDT) Subject: Re: [PATCH] nptl: Use SA_RESTART for SIGCANCEL handler To: Florian Weimer , Adhemerval Zanella via Libc-alpha References: <20210617125241.1415287-1-adhemerval.zanella@linaro.org> <878s37qu6l.fsf@oldenburg.str.redhat.com> From: Adhemerval Zanella Message-ID: <0ed7e3e6-845b-0609-db0e-82f9898874dc@linaro.org> Date: Tue, 22 Jun 2021 15:30:20 -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: <878s37qu6l.fsf@oldenburg.str.redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.1 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 autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Tue, 22 Jun 2021 18:30:25 -0000 On 18/06/2021 08:38, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> The usage of signals to implementation pthread cancellation is an >> implementation detail and should not be visible through cancellation >> entrypoints. >> >> However now that pthread_cancel always send the SIGCANCEL, some >> entrypoint might be interruptable and return EINTR to the caller >> (for instance on sem_wait). >> >> Using SA_RESTART hides this, since the cancellation handler should >> either act uppon cancellation (if asynchronous cancellation is enable) >> or ignore the cancellation internal signal. > > I think this still needs a NEWS entry because there have been kernel > bugs in this area (e.g. in CIFS). Ok, I have added the following on "Deprecated and removed features, and other changes affecting compatibility" * The pthread cancellation handler is now setup with SA_RESTART. It should not be visible to application since the cancellation handler should either act uppon cancellation (if asynchronous cancellation is enabled) or ignore the cancellation internal signal. > >> Checked on x86_64-linux-gnu and i686-linux-gnu. >> --- >> nptl/pthread_cancel.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c >> index 0698cd2046..cc25ff21f3 100644 >> --- a/nptl/pthread_cancel.c >> +++ b/nptl/pthread_cancel.c >> @@ -72,7 +72,11 @@ __pthread_cancel (pthread_t th) >> { >> struct sigaction sa; >> sa.sa_sigaction = sigcancel_handler; >> - sa.sa_flags = SA_SIGINFO; >> + /* The signal handle should be non-interruptible to avoid the risk of >> + spurious EINTR caused by SIGCANCEL sent to process or if >> + pthread_cancel() is called while cancellation is disabled in the >> + target thread. */ >> + sa.sa_flags = SA_SIGINFO | SA_RESTART; >> __sigemptyset (&sa.sa_mask); >> __libc_sigaction (SIGCANCEL, &sa, NULL); >> atomic_store_relaxed (&init_sigcancel, 1); > > I really don't feel comfortable reviewing this. However I think it is > still consistent with the (buggy) SYSCALL_CANCEL implementation: > > int sc_cancel_oldtype = LIBC_CANCEL_ASYNC (); \ > sc_ret = INLINE_SYSCALL_CALL (__VA_ARGS__); \ > LIBC_CANCEL_RESET (sc_cancel_oldtype); \ > > We temporary enable async cancellation, in which case we unwind through > the signal handler if canceled. We do not rely on a EINTR error return > from the system call and a cancellation check outside of the signal > handler. So adding SA_RESTART should really be okay. Yes, we still cancel the thread even for partial results (BZ#12683).