* Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)
2022-04-14 15:49 [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029) Adhemerval Zanella
@ 2022-04-14 18:26 ` Adhemerval Zanella
2022-04-19 10:44 ` Szabolcs Nagy
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Adhemerval Zanella @ 2022-04-14 18:26 UTC (permalink / raw)
To: libc-alpha
I will commit this patch shortly.
On 14/04/2022 12:49, Adhemerval Zanella wrote:
> Some Linux interfaces never restart after being interrupted by a signal
> handler, regardless of the use of SA_RESTART [1]. It means that for
> pthread cancellation, if the target thread disables cancellation with
> pthread_setcancelstate and calls such interfaces (like poll or select),
> it should not see spurious EINTR failures due the internal SIGCANCEL.
>
> However recent changes made pthread_cancel to always sent the internal
> signal, regardless of the target thread cancellation status or type.
> To fix it, the previous semantic is restored, where the cancel signal
> is only sent if the target thread has cancelation enabled in
> asynchronous mode.
>
> The cancel state and cancel type is moved back to cancelhandling
> and atomic operation are used to synchronize between threads. The
> patch essentially revert the following commits:
>
> 8c1c0aae20 nptl: Move cancel type out of cancelhandling
> 2b51742531 nptl: Move cancel state out of cancelhandling
> 26cfbb7162 nptl: Remove CANCELING_BITMASK
>
> However I changed the atomic operation to follow the internal C11
> semantic and removed the MACRO usage, it simplifies a bit the
> resulting code (and removes another usage of the old atomic macros).
>
> Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu,
> and powerpc64-linux-gnu.
>
> [1] https://man7.org/linux/man-pages/man7/signal.7.html
>
> Reviewed-by: Florian Weimer <fweimer@redhat.com>
> Tested-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
> v2: Fixed some typos and extended pthread_cancel comments.
> ---
> manual/process.texi | 3 +-
> nptl/allocatestack.c | 2 -
> nptl/cancellation.c | 50 ++++++--
> nptl/cleanup_defer.c | 42 ++++++-
> nptl/descr.h | 41 +++++--
> nptl/libc-cleanup.c | 39 ++++++-
> nptl/pthread_cancel.c | 110 +++++++++++++-----
> nptl/pthread_join_common.c | 7 +-
> nptl/pthread_setcancelstate.c | 26 ++++-
> nptl/pthread_setcanceltype.c | 31 ++++-
> nptl/pthread_testcancel.c | 9 +-
> sysdeps/nptl/dl-tls_init_tp.c | 3 -
> sysdeps/nptl/pthreadP.h | 2 +-
> sysdeps/pthread/Makefile | 1 +
> sysdeps/pthread/tst-cancel29.c | 207 +++++++++++++++++++++++++++++++++
> 15 files changed, 482 insertions(+), 91 deletions(-)
> create mode 100644 sysdeps/pthread/tst-cancel29.c
>
> diff --git a/manual/process.texi b/manual/process.texi
> index 28c9531f42..9307379194 100644
> --- a/manual/process.texi
> +++ b/manual/process.texi
> @@ -68,8 +68,7 @@ until the subprogram terminates before you can do anything else.
> @c CLEANUP_HANDLER @ascuplugin @ascuheap @acsmem
> @c libc_cleanup_region_start @ascuplugin @ascuheap @acsmem
> @c pthread_cleanup_push_defer @ascuplugin @ascuheap @acsmem
> -@c __pthread_testcancel @ascuplugin @ascuheap @acsmem
> -@c CANCEL_ENABLED_AND_CANCELED ok
> +@c cancel_enabled_and_canceled @ascuplugin @ascuheap @acsmem
> @c do_cancel @ascuplugin @ascuheap @acsmem
> @c cancel_handler ok
> @c kill syscall ok
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 34a33164ff..01a282f3f6 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -119,8 +119,6 @@ get_cached_stack (size_t *sizep, void **memp)
>
> /* Cancellation handling is back to the default. */
> result->cancelhandling = 0;
> - result->cancelstate = PTHREAD_CANCEL_ENABLE;
> - result->canceltype = PTHREAD_CANCEL_DEFERRED;
> result->cleanup = NULL;
> result->setup_failed = 0;
>
> diff --git a/nptl/cancellation.c b/nptl/cancellation.c
> index 8d54a6add1..f4b08902b2 100644
> --- a/nptl/cancellation.c
> +++ b/nptl/cancellation.c
> @@ -30,19 +30,26 @@ int
> __pthread_enable_asynccancel (void)
> {
> struct pthread *self = THREAD_SELF;
> + int oldval = atomic_load_relaxed (&self->cancelhandling);
>
> - int oldval = THREAD_GETMEM (self, canceltype);
> - THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_ASYNCHRONOUS);
> + while (1)
> + {
> + int newval = oldval | CANCELTYPE_BITMASK;
>
> - int ch = THREAD_GETMEM (self, cancelhandling);
> + if (newval == oldval)
> + break;
>
> - if (self->cancelstate == PTHREAD_CANCEL_ENABLE
> - && (ch & CANCELED_BITMASK)
> - && !(ch & EXITING_BITMASK)
> - && !(ch & TERMINATED_BITMASK))
> - {
> - THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> - __do_cancel ();
> + if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &oldval, newval))
> + {
> + if (cancel_enabled_and_canceled_and_async (newval))
> + {
> + self->result = PTHREAD_CANCELED;
> + __do_cancel ();
> + }
> +
> + break;
> + }
> }
>
> return oldval;
> @@ -56,10 +63,29 @@ __pthread_disable_asynccancel (int oldtype)
> {
> /* If asynchronous cancellation was enabled before we do not have
> anything to do. */
> - if (oldtype == PTHREAD_CANCEL_ASYNCHRONOUS)
> + if (oldtype & CANCELTYPE_BITMASK)
> return;
>
> struct pthread *self = THREAD_SELF;
> - self->canceltype = PTHREAD_CANCEL_DEFERRED;
> + int newval;
> + int oldval = atomic_load_relaxed (&self->cancelhandling);
> + do
> + {
> + newval = oldval & ~CANCELTYPE_BITMASK;
> + }
> + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &oldval, newval));
> +
> + /* 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);
> + }
> }
> libc_hidden_def (__pthread_disable_asynccancel)
> diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
> index f8181a40e8..eb0bc77740 100644
> --- a/nptl/cleanup_defer.c
> +++ b/nptl/cleanup_defer.c
> @@ -30,9 +30,22 @@ ___pthread_register_cancel_defer (__pthread_unwind_buf_t *buf)
> ibuf->priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
> ibuf->priv.data.cleanup = THREAD_GETMEM (self, cleanup);
>
> - /* Disable asynchronous cancellation for now. */
> - ibuf->priv.data.canceltype = THREAD_GETMEM (self, canceltype);
> - THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
> + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> + if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
> + {
> + int newval;
> + do
> + {
> + newval = cancelhandling & ~CANCELTYPE_BITMASK;
> + }
> + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &cancelhandling,
> + newval));
> + }
> +
> + ibuf->priv.data.canceltype = (cancelhandling & CANCELTYPE_BITMASK
> + ? PTHREAD_CANCEL_ASYNCHRONOUS
> + : PTHREAD_CANCEL_DEFERRED);
>
> /* Store the new cleanup handler info. */
> THREAD_SETMEM (self, cleanup_jmp_buf, (struct pthread_unwind_buf *) buf);
> @@ -54,9 +67,26 @@ ___pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
>
> THREAD_SETMEM (self, cleanup_jmp_buf, ibuf->priv.data.prev);
>
> - THREAD_SETMEM (self, canceltype, ibuf->priv.data.canceltype);
> - if (ibuf->priv.data.canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> - __pthread_testcancel ();
> + if (ibuf->priv.data.canceltype == PTHREAD_CANCEL_DEFERRED)
> + return;
> +
> + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> + if (cancelhandling & CANCELTYPE_BITMASK)
> + {
> + int newval;
> + do
> + {
> + newval = cancelhandling | CANCELTYPE_BITMASK;
> + }
> + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &cancelhandling, newval));
> +
> + if (cancel_enabled_and_canceled (cancelhandling))
> + {
> + self->result = PTHREAD_CANCELED;
> + __do_cancel ();
> + }
> + }
> }
> versioned_symbol (libc, ___pthread_unregister_cancel_restore,
> __pthread_unregister_cancel_restore, GLIBC_2_34);
> diff --git a/nptl/descr.h b/nptl/descr.h
> index ea8aca08e6..bb46b5958e 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -279,18 +279,27 @@ struct pthread
>
> /* Flags determining processing of cancellation. */
> int cancelhandling;
> + /* Bit set if cancellation is disabled. */
> +#define CANCELSTATE_BIT 0
> +#define CANCELSTATE_BITMASK (1 << CANCELSTATE_BIT)
> + /* Bit set if asynchronous cancellation mode is selected. */
> +#define CANCELTYPE_BIT 1
> +#define CANCELTYPE_BITMASK (1 << CANCELTYPE_BIT)
> + /* Bit set if canceling has been initiated. */
> +#define CANCELING_BIT 2
> +#define CANCELING_BITMASK (1 << CANCELING_BIT)
> /* Bit set if canceled. */
> #define CANCELED_BIT 3
> -#define CANCELED_BITMASK (0x01 << CANCELED_BIT)
> +#define CANCELED_BITMASK (1 << CANCELED_BIT)
> /* Bit set if thread is exiting. */
> #define EXITING_BIT 4
> -#define EXITING_BITMASK (0x01 << EXITING_BIT)
> +#define EXITING_BITMASK (1 << EXITING_BIT)
> /* Bit set if thread terminated and TCB is freed. */
> #define TERMINATED_BIT 5
> -#define TERMINATED_BITMASK (0x01 << TERMINATED_BIT)
> +#define TERMINATED_BITMASK (1 << TERMINATED_BIT)
> /* Bit set if thread is supposed to change XID. */
> #define SETXID_BIT 6
> -#define SETXID_BITMASK (0x01 << SETXID_BIT)
> +#define SETXID_BITMASK (1 << SETXID_BIT)
>
> /* Flags. Including those copied from the thread attribute. */
> int flags;
> @@ -390,14 +399,6 @@ struct pthread
> /* Indicates whether is a C11 thread created by thrd_creat. */
> bool c11;
>
> - /* Thread cancel state (PTHREAD_CANCEL_ENABLE or
> - PTHREAD_CANCEL_DISABLE). */
> - unsigned char cancelstate;
> -
> - /* Thread cancel type (PTHREAD_CANCEL_DEFERRED or
> - PTHREAD_CANCEL_ASYNCHRONOUS). */
> - unsigned char canceltype;
> -
> /* Used in __pthread_kill_internal to detected a thread that has
> exited or is about to exit. exit_lock must only be acquired
> after blocking signals. */
> @@ -417,6 +418,22 @@ struct pthread
> (sizeof (struct pthread) - offsetof (struct pthread, end_padding))
> } __attribute ((aligned (TCB_ALIGNMENT)));
>
> +static inline bool
> +cancel_enabled_and_canceled (int value)
> +{
> + return (value & (CANCELSTATE_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
> + | TERMINATED_BITMASK))
> + == CANCELED_BITMASK;
> +}
> +
> +static inline bool
> +cancel_enabled_and_canceled_and_async (int value)
> +{
> + return ((value) & (CANCELSTATE_BITMASK | CANCELTYPE_BITMASK | CANCELED_BITMASK
> + | EXITING_BITMASK | TERMINATED_BITMASK))
> + == (CANCELTYPE_BITMASK | CANCELED_BITMASK);
> +}
> +
> /* This yields the pointer that TLS support code calls the thread pointer. */
> #if TLS_TCB_AT_TP
> # define TLS_TPADJ(pd) (pd)
> diff --git a/nptl/libc-cleanup.c b/nptl/libc-cleanup.c
> index cb4c226281..c4a83591bf 100644
> --- a/nptl/libc-cleanup.c
> +++ b/nptl/libc-cleanup.c
> @@ -26,9 +26,24 @@ __libc_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer)
>
> buffer->__prev = THREAD_GETMEM (self, cleanup);
>
> + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> +
> /* Disable asynchronous cancellation for now. */
> - buffer->__canceltype = THREAD_GETMEM (self, canceltype);
> - THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
> + if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
> + {
> + int newval;
> + do
> + {
> + newval = cancelhandling & ~CANCELTYPE_BITMASK;
> + }
> + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &cancelhandling,
> + newval));
> + }
> +
> + buffer->__canceltype = (cancelhandling & CANCELTYPE_BITMASK
> + ? PTHREAD_CANCEL_ASYNCHRONOUS
> + : PTHREAD_CANCEL_DEFERRED);
>
> THREAD_SETMEM (self, cleanup, buffer);
> }
> @@ -41,8 +56,22 @@ __libc_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer)
>
> THREAD_SETMEM (self, cleanup, buffer->__prev);
>
> - THREAD_SETMEM (self, canceltype, buffer->__canceltype);
> - if (buffer->__canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> - __pthread_testcancel ();
> + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> + if (cancelhandling & CANCELTYPE_BITMASK)
> + {
> + int newval;
> + do
> + {
> + newval = cancelhandling | CANCELTYPE_BITMASK;
> + }
> + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &cancelhandling, newval));
> +
> + if (cancel_enabled_and_canceled (cancelhandling))
> + {
> + self->result = PTHREAD_CANCELED;
> + __do_cancel ();
> + }
> + }
> }
> libc_hidden_def (__libc_cleanup_pop_restore)
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index 7524c7ce4d..c76882e279 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -42,18 +42,29 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
>
> struct pthread *self = THREAD_SELF;
>
> - int ch = atomic_load_relaxed (&self->cancelhandling);
> - /* Cancelation not enabled, not cancelled, or already exitting. */
> - if (self->cancelstate == PTHREAD_CANCEL_DISABLE
> - || (ch & CANCELED_BITMASK) == 0
> - || (ch & EXITING_BITMASK) != 0)
> - return;
> -
> - /* Set the return value. */
> - THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> - /* Make sure asynchronous cancellation is still enabled. */
> - if (self->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> - __do_cancel ();
> + int oldval = atomic_load_relaxed (&self->cancelhandling);
> + while (1)
> + {
> + /* We are canceled now. When canceled by another thread this flag
> + is already set but if the signal is directly send (internally or
> + from another process) is has to be done here. */
> + int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
> +
> + if (oldval == newval || (oldval & EXITING_BITMASK) != 0)
> + /* Already canceled or exiting. */
> + break;
> +
> + if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &oldval, newval))
> + {
> + self->result = PTHREAD_CANCELED;
> +
> + /* Make sure asynchronous cancellation is still enabled. */
> + if ((oldval & CANCELTYPE_BITMASK) != 0)
> + /* Run the registered destructors and terminate the thread. */
> + __do_cancel ();
> + }
> + }
> }
>
> int
> @@ -92,29 +103,70 @@ __pthread_cancel (pthread_t th)
> }
> #endif
>
> - int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK);
> - if ((oldch & CANCELED_BITMASK) != 0)
> - return 0;
> -
> - if (pd == THREAD_SELF)
> + /* 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 cannot send SIGCANCEL unless the cancellation
> + 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 setting 'cancelhandling' and the cancelation will be acted
> + upon on next cancellation entrypoing in the target thread.
> +
> + It also requires to atomically check if cancellation is enabled and
> + asynchronous, so both cancellation state and type are tracked on
> + 'cancelhandling'. */
> +
> + int result = 0;
> + int oldval = atomic_load_relaxed (&pd->cancelhandling);
> + int newval;
> + do
> {
> - /* A single-threaded process should be able to kill itself, since there
> - is nothing in the POSIX specification that says that it cannot. So
> - we set multiple_threads to true so that cancellation points get
> - executed. */
> - THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
> + newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
> + if (oldval == newval)
> + break;
> +
> + /* If the cancellation is handled asynchronously just send a
> + signal. We avoid this if possible since it's more
> + expensive. */
> + if (cancel_enabled_and_canceled_and_async (newval))
> + {
> + /* Mark the cancellation as "in progress". */
> + int newval2 = oldval | CANCELING_BITMASK;
> + if (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling,
> + &oldval, newval2))
> + continue;
> +
> + if (pd == THREAD_SELF)
> + /* This is not merely an optimization: An application may
> + call pthread_cancel (pthread_self ()) without calling
> + pthread_create, so the signal handler may not have been
> + set up for a self-cancel. */
> + {
> + pd->result = PTHREAD_CANCELED;
> + if ((newval & CANCELTYPE_BITMASK) != 0)
> + __do_cancel ();
> + }
> + else
> + /* The cancellation handler will take care of marking the
> + thread as canceled. */
> + result = __pthread_kill_internal (th, SIGCANCEL);
> +
> + break;
> + }
> +
> + /* A single-threaded process should be able to kill itself, since
> + there is nothing in the POSIX specification that says that it
> + cannot. So we set multiple_threads to true so that cancellation
> + points get executed. */
> + THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
> #ifndef TLS_MULTIPLE_THREADS_IN_TCB
> __libc_multiple_threads = 1;
> #endif
> -
> - THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
> - if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
> - && pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> - __do_cancel ();
> - return 0;
> }
> + while (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling, &oldval,
> + newval));
>
> - return __pthread_kill_internal (th, SIGCANCEL);
> + return result;
> }
> versioned_symbol (libc, __pthread_cancel, pthread_cancel, GLIBC_2_34);
>
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index a8e884f341..ca3245b0af 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -57,12 +57,9 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
> if ((pd == self
> || (self->joinid == pd
> && (pd->cancelhandling
> - & (CANCELED_BITMASK | EXITING_BITMASK
> + & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
> | TERMINATED_BITMASK)) == 0))
> - && !(self->cancelstate == PTHREAD_CANCEL_ENABLE
> - && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
> - | TERMINATED_BITMASK))
> - == CANCELED_BITMASK))
> + && !cancel_enabled_and_canceled (self->cancelhandling))
> /* This is a deadlock situation. 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
> diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
> index 9905b12e4f..f8edf18fbe 100644
> --- a/nptl/pthread_setcancelstate.c
> +++ b/nptl/pthread_setcancelstate.c
> @@ -30,9 +30,29 @@ __pthread_setcancelstate (int state, int *oldstate)
>
> self = THREAD_SELF;
>
> - if (oldstate != NULL)
> - *oldstate = self->cancelstate;
> - self->cancelstate = state;
> + int oldval = atomic_load_relaxed (&self->cancelhandling);
> + while (1)
> + {
> + int newval = (state == PTHREAD_CANCEL_DISABLE
> + ? oldval | CANCELSTATE_BITMASK
> + : oldval & ~CANCELSTATE_BITMASK);
> +
> + if (oldstate != NULL)
> + *oldstate = ((oldval & CANCELSTATE_BITMASK)
> + ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
> +
> + if (oldval == newval)
> + break;
> +
> + if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &oldval, newval))
> + {
> + if (cancel_enabled_and_canceled_and_async (newval))
> + __do_cancel ();
> +
> + break;
> + }
> + }
>
> return 0;
> }
> diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
> index 94e56466de..1307d355c1 100644
> --- a/nptl/pthread_setcanceltype.c
> +++ b/nptl/pthread_setcanceltype.c
> @@ -28,11 +28,32 @@ __pthread_setcanceltype (int type, int *oldtype)
>
> volatile struct pthread *self = THREAD_SELF;
>
> - if (oldtype != NULL)
> - *oldtype = self->canceltype;
> - self->canceltype = type;
> - if (type == PTHREAD_CANCEL_ASYNCHRONOUS)
> - __pthread_testcancel ();
> + int oldval = atomic_load_relaxed (&self->cancelhandling);
> + while (1)
> + {
> + int newval = (type == PTHREAD_CANCEL_ASYNCHRONOUS
> + ? oldval | CANCELTYPE_BITMASK
> + : oldval & ~CANCELTYPE_BITMASK);
> +
> + if (oldtype != NULL)
> + *oldtype = ((oldval & CANCELTYPE_BITMASK)
> + ? PTHREAD_CANCEL_ASYNCHRONOUS : PTHREAD_CANCEL_DEFERRED);
> +
> + if (oldval == newval)
> + break;
> +
> + if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &oldval, newval))
> + {
> + if (cancel_enabled_and_canceled_and_async (newval))
> + {
> + THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> + __do_cancel ();
> + }
> +
> + break;
> + }
> + }
>
> return 0;
> }
> diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
> index 13123608e8..b81928c000 100644
> --- a/nptl/pthread_testcancel.c
> +++ b/nptl/pthread_testcancel.c
> @@ -23,13 +23,10 @@ void
> ___pthread_testcancel (void)
> {
> struct pthread *self = THREAD_SELF;
> - int cancelhandling = THREAD_GETMEM (self, cancelhandling);
> - if (self->cancelstate == PTHREAD_CANCEL_ENABLE
> - && (cancelhandling & CANCELED_BITMASK)
> - && !(cancelhandling & EXITING_BITMASK)
> - && !(cancelhandling & TERMINATED_BITMASK))
> + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> + if (cancel_enabled_and_canceled (cancelhandling))
> {
> - THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> + self->result = PTHREAD_CANCELED;
> __do_cancel ();
> }
> }
> diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
> index 1294c91816..53fba774a5 100644
> --- a/sysdeps/nptl/dl-tls_init_tp.c
> +++ b/sysdeps/nptl/dl-tls_init_tp.c
> @@ -128,7 +128,4 @@ __tls_init_tp (void)
> It will be bigger than it actually is, but for unwind.c/pt-longjmp.c
> purposes this is good enough. */
> THREAD_SETMEM (pd, stackblock_size, (size_t) __libc_stack_end);
> -
> - THREAD_SETMEM (pd, cancelstate, PTHREAD_CANCEL_ENABLE);
> - THREAD_SETMEM (pd, canceltype, PTHREAD_CANCEL_DEFERRED);
> }
> diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
> index 708bd92469..601db4ff2b 100644
> --- a/sysdeps/nptl/pthreadP.h
> +++ b/sysdeps/nptl/pthreadP.h
> @@ -275,7 +275,7 @@ __do_cancel (void)
> struct pthread *self = THREAD_SELF;
>
> /* Make sure we get no more cancellations. */
> - THREAD_ATOMIC_BIT_SET (self, cancelhandling, EXITING_BIT);
> + atomic_bit_set (&self->cancelhandling, EXITING_BIT);
>
> __pthread_unwind ((__pthread_unwind_buf_t *)
> THREAD_GETMEM (self, cleanup_jmp_buf));
> diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> index 318dfdc04a..e901c51df0 100644
> --- a/sysdeps/pthread/Makefile
> +++ b/sysdeps/pthread/Makefile
> @@ -69,6 +69,7 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
> tst-cancel12 tst-cancel13 tst-cancel14 tst-cancel15 tst-cancel16 \
> tst-cancel18 tst-cancel19 tst-cancel20 tst-cancel21 \
> tst-cancel22 tst-cancel23 tst-cancel26 tst-cancel27 tst-cancel28 \
> + tst-cancel29 \
> tst-cleanup0 tst-cleanup1 tst-cleanup2 tst-cleanup3 \
> tst-clock1 \
> tst-cond-except \
> diff --git a/sysdeps/pthread/tst-cancel29.c b/sysdeps/pthread/tst-cancel29.c
> new file mode 100644
> index 0000000000..4f0d99e002
> --- /dev/null
> +++ b/sysdeps/pthread/tst-cancel29.c
> @@ -0,0 +1,207 @@
> +/* Check if a thread that disables cancellation and which call functions
> + that might be interrupted by a signal do not see the internal SIGCANCEL.
> +
> + Copyright (C) 2022 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <array_length.h>
> +#include <errno.h>
> +#include <inttypes.h>
> +#include <poll.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/xthread.h>
> +#include <sys/socket.h>
> +#include <signal.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +
> +/* On Linux some interfaces are never restarted after being interrupted by
> + a signal handler, regardless of the use of SA_RESTART. It means that
> + if asynchronous cancellation is not enabled, the pthread_cancel can not
> + set the internal SIGCANCEL otherwise the interface might see a spurious
> + EINTR failure. */
> +
> +static pthread_barrier_t b;
> +
> +/* Cleanup handling test. */
> +static int cl_called;
> +static void
> +cl (void *arg)
> +{
> + ++cl_called;
> +}
> +
> +static void *
> +tf_sigtimedwait (void *arg)
> +{
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> + xpthread_barrier_wait (&b);
> +
> + int r;
> + pthread_cleanup_push (cl, NULL);
> +
> + sigset_t mask;
> + sigemptyset (&mask);
> + r = sigtimedwait (&mask, NULL, &(struct timespec) { 0, 250000000 });
> + if (r != -1)
> + return (void*) -1;
> + if (errno != EAGAIN)
> + return (void*) -2;
> +
> + pthread_cleanup_pop (0);
> + return NULL;
> +}
> +
> +static void *
> +tf_poll (void *arg)
> +{
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> + xpthread_barrier_wait (&b);
> +
> + int r;
> + pthread_cleanup_push (cl, NULL);
> +
> + r = poll (NULL, 0, 250);
> + if (r != 0)
> + return (void*) -1;
> +
> + pthread_cleanup_pop (0);
> + return NULL;
> +}
> +
> +static void *
> +tf_ppoll (void *arg)
> +{
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> +
> + xpthread_barrier_wait (&b);
> +
> + int r;
> + pthread_cleanup_push (cl, NULL);
> +
> + r = ppoll (NULL, 0, &(struct timespec) { 0, 250000000 }, NULL);
> + if (r != 0)
> + return (void*) -1;
> +
> + pthread_cleanup_pop (0);
> + return NULL;
> +}
> +
> +static void *
> +tf_select (void *arg)
> +{
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> + xpthread_barrier_wait (&b);
> +
> + int r;
> + pthread_cleanup_push (cl, NULL);
> +
> + r = select (0, NULL, NULL, NULL, &(struct timeval) { 0, 250000 });
> + if (r != 0)
> + return (void*) -1;
> +
> + pthread_cleanup_pop (0);
> + return NULL;
> +}
> +
> +static void *
> +tf_pselect (void *arg)
> +{
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> + xpthread_barrier_wait (&b);
> +
> + int r;
> + pthread_cleanup_push (cl, NULL);
> +
> + r = pselect (0, NULL, NULL, NULL, &(struct timespec) { 0, 250000000 }, NULL);
> + if (r != 0)
> + return (void*) -1;
> +
> + pthread_cleanup_pop (0);
> + return NULL;
> +}
> +
> +static void *
> +tf_clock_nanosleep (void *arg)
> +{
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> + xpthread_barrier_wait (&b);
> +
> + int r;
> + pthread_cleanup_push (cl, NULL);
> +
> + r = clock_nanosleep (CLOCK_REALTIME, 0, &(struct timespec) { 0, 250000000 },
> + NULL);
> + if (r != 0)
> + return (void*) -1;
> +
> + pthread_cleanup_pop (0);
> + return NULL;
> +}
> +
> +struct cancel_test_t
> +{
> + const char *name;
> + void * (*cf) (void *);
> +} tests[] =
> +{
> + { "sigtimedwait", tf_sigtimedwait, },
> + { "poll", tf_poll, },
> + { "ppoll", tf_ppoll, },
> + { "select", tf_select, },
> + { "pselect", tf_pselect , },
> + { "clock_nanosleep", tf_clock_nanosleep, },
> +};
> +
> +static int
> +do_test (void)
> +{
> + for (int i = 0; i < array_length (tests); i++)
> + {
> + xpthread_barrier_init (&b, NULL, 2);
> +
> + cl_called = 0;
> +
> + pthread_t th = xpthread_create (NULL, tests[i].cf, NULL);
> +
> + xpthread_barrier_wait (&b);
> +
> + struct timespec ts = { .tv_sec = 0, .tv_nsec = 100000000 };
> + while (nanosleep (&ts, &ts) != 0)
> + continue;
> +
> + xpthread_cancel (th);
> +
> + void *status = xpthread_join (th);
> + if (status != NULL)
> + printf ("test '%s' failed: %" PRIdPTR "\n", tests[i].name,
> + (intptr_t) status);
> + TEST_VERIFY (status == NULL);
> +
> + xpthread_barrier_destroy (&b);
> +
> + TEST_COMPARE (cl_called, 0);
> +
> + printf ("in-time cancel test of '%s' successful\n", tests[i].name);
> + }
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)
2022-04-14 15:49 [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029) Adhemerval Zanella
2022-04-14 18:26 ` Adhemerval Zanella
@ 2022-04-19 10:44 ` Szabolcs Nagy
2022-04-19 12:18 ` Adhemerval Zanella
2022-04-19 12:10 ` Szabolcs Nagy
2022-07-12 21:27 ` Noah Goldstein
3 siblings, 1 reply; 12+ messages in thread
From: Szabolcs Nagy @ 2022-04-19 10:44 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, Florian Weimer, Aurelien Jarno
The 04/14/2022 12:49, Adhemerval Zanella via Libc-alpha wrote:
> Some Linux interfaces never restart after being interrupted by a signal
> handler, regardless of the use of SA_RESTART [1]. It means that for
> pthread cancellation, if the target thread disables cancellation with
> pthread_setcancelstate and calls such interfaces (like poll or select),
> it should not see spurious EINTR failures due the internal SIGCANCEL.
>
> However recent changes made pthread_cancel to always sent the internal
> signal, regardless of the target thread cancellation status or type.
> To fix it, the previous semantic is restored, where the cancel signal
> is only sent if the target thread has cancelation enabled in
> asynchronous mode.
>
> The cancel state and cancel type is moved back to cancelhandling
> and atomic operation are used to synchronize between threads. The
> patch essentially revert the following commits:
>
> 8c1c0aae20 nptl: Move cancel type out of cancelhandling
> 2b51742531 nptl: Move cancel state out of cancelhandling
> 26cfbb7162 nptl: Remove CANCELING_BITMASK
>
> However I changed the atomic operation to follow the internal C11
> semantic and removed the MACRO usage, it simplifies a bit the
> resulting code (and removes another usage of the old atomic macros).
>
> Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu,
> and powerpc64-linux-gnu.
>
> [1] https://man7.org/linux/man-pages/man7/signal.7.html
>
> Reviewed-by: Florian Weimer <fweimer@redhat.com>
> Tested-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
> v2: Fixed some typos and extended pthread_cancel comments.
since this commit various cancel tests fail for me (unreliably)
on aarch64 e.g. failures from 2 different test runs:
FAIL: nptl/tst-cancel17
FAIL: nptl/tst-cancelx5
FAIL: nptl/tst-cond7
FAIL: nptl/tst-pthread-raise-blocked-self
FAIL: nptl/tst-pthread_cancel-select-loop
FAIL: nptl/tst-cancelx20
FAIL: nptl/tst-cond7
FAIL: nptl/tst-cond8
FAIL: nptl/tst-join12
FAIL: nptl/tst-key3
FAIL: nptl/tst-pthread_cancel-select-loop
an example run of nptl/tst-cond7
$ elf/ld.so --library-path . nptl/tst-cond7 --direct
round 0
child created
parent: joining now
round 1
child created
parent: joining now
round 2
child created
parent: joining now
round 3
child created
parent: joining now
round 4
child created
parent: joining now
where it is blocked forever: it seems pthread_cancel returns without
sending a signal (__pthread_kill_internal is not called) so join hangs.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)
2022-04-19 10:44 ` Szabolcs Nagy
@ 2022-04-19 12:18 ` Adhemerval Zanella
2022-04-19 12:23 ` Adhemerval Zanella
0 siblings, 1 reply; 12+ messages in thread
From: Adhemerval Zanella @ 2022-04-19 12:18 UTC (permalink / raw)
To: Szabolcs Nagy; +Cc: libc-alpha, Florian Weimer, Aurelien Jarno
On 19/04/2022 07:44, Szabolcs Nagy wrote:
> The 04/14/2022 12:49, Adhemerval Zanella via Libc-alpha wrote:
>> Some Linux interfaces never restart after being interrupted by a signal
>> handler, regardless of the use of SA_RESTART [1]. It means that for
>> pthread cancellation, if the target thread disables cancellation with
>> pthread_setcancelstate and calls such interfaces (like poll or select),
>> it should not see spurious EINTR failures due the internal SIGCANCEL.
>>
>> However recent changes made pthread_cancel to always sent the internal
>> signal, regardless of the target thread cancellation status or type.
>> To fix it, the previous semantic is restored, where the cancel signal
>> is only sent if the target thread has cancelation enabled in
>> asynchronous mode.
>>
>> The cancel state and cancel type is moved back to cancelhandling
>> and atomic operation are used to synchronize between threads. The
>> patch essentially revert the following commits:
>>
>> 8c1c0aae20 nptl: Move cancel type out of cancelhandling
>> 2b51742531 nptl: Move cancel state out of cancelhandling
>> 26cfbb7162 nptl: Remove CANCELING_BITMASK
>>
>> However I changed the atomic operation to follow the internal C11
>> semantic and removed the MACRO usage, it simplifies a bit the
>> resulting code (and removes another usage of the old atomic macros).
>>
>> Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu,
>> and powerpc64-linux-gnu.
>>
>> [1] https://man7.org/linux/man-pages/man7/signal.7.html
>>
>> Reviewed-by: Florian Weimer <fweimer@redhat.com>
>> Tested-by: Aurelien Jarno <aurelien@aurel32.net>
>> ---
>> v2: Fixed some typos and extended pthread_cancel comments.
>
>
> since this commit various cancel tests fail for me (unreliably)
> on aarch64 e.g. failures from 2 different test runs:
>
> FAIL: nptl/tst-cancel17
> FAIL: nptl/tst-cancelx5
> FAIL: nptl/tst-cond7
> FAIL: nptl/tst-pthread-raise-blocked-self
> FAIL: nptl/tst-pthread_cancel-select-loop
>
> FAIL: nptl/tst-cancelx20
> FAIL: nptl/tst-cond7
> FAIL: nptl/tst-cond8
> FAIL: nptl/tst-join12
> FAIL: nptl/tst-key3
> FAIL: nptl/tst-pthread_cancel-select-loop
>
> an example run of nptl/tst-cond7
>
> $ elf/ld.so --library-path . nptl/tst-cond7 --direct
> round 0
> child created
> parent: joining now
> round 1
> child created
> parent: joining now
> round 2
> child created
> parent: joining now
> round 3
> child created
> parent: joining now
> round 4
> child created
> parent: joining now
>
> where it is blocked forever: it seems pthread_cancel returns without
> sending a signal (__pthread_kill_internal is not called) so join hangs.
>
But the signal should be only sent if thread is is cancelled and has
async cancellation enabled, which is not the case for the tests. I am
trying to reproduce it on an aarch64 machine, but I can't see t any
failure tests above. I will double check if I revert everything and
if the atomics usage are fully correct.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)
2022-04-19 12:18 ` Adhemerval Zanella
@ 2022-04-19 12:23 ` Adhemerval Zanella
0 siblings, 0 replies; 12+ messages in thread
From: Adhemerval Zanella @ 2022-04-19 12:23 UTC (permalink / raw)
To: Szabolcs Nagy; +Cc: libc-alpha, Florian Weimer, Aurelien Jarno
On 19/04/2022 09:18, Adhemerval Zanella wrote:
>
>
> On 19/04/2022 07:44, Szabolcs Nagy wrote:
>> The 04/14/2022 12:49, Adhemerval Zanella via Libc-alpha wrote:
>>> Some Linux interfaces never restart after being interrupted by a signal
>>> handler, regardless of the use of SA_RESTART [1]. It means that for
>>> pthread cancellation, if the target thread disables cancellation with
>>> pthread_setcancelstate and calls such interfaces (like poll or select),
>>> it should not see spurious EINTR failures due the internal SIGCANCEL.
>>>
>>> However recent changes made pthread_cancel to always sent the internal
>>> signal, regardless of the target thread cancellation status or type.
>>> To fix it, the previous semantic is restored, where the cancel signal
>>> is only sent if the target thread has cancelation enabled in
>>> asynchronous mode.
>>>
>>> The cancel state and cancel type is moved back to cancelhandling
>>> and atomic operation are used to synchronize between threads. The
>>> patch essentially revert the following commits:
>>>
>>> 8c1c0aae20 nptl: Move cancel type out of cancelhandling
>>> 2b51742531 nptl: Move cancel state out of cancelhandling
>>> 26cfbb7162 nptl: Remove CANCELING_BITMASK
>>>
>>> However I changed the atomic operation to follow the internal C11
>>> semantic and removed the MACRO usage, it simplifies a bit the
>>> resulting code (and removes another usage of the old atomic macros).
>>>
>>> Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu,
>>> and powerpc64-linux-gnu.
>>>
>>> [1] https://man7.org/linux/man-pages/man7/signal.7.html
>>>
>>> Reviewed-by: Florian Weimer <fweimer@redhat.com>
>>> Tested-by: Aurelien Jarno <aurelien@aurel32.net>
>>> ---
>>> v2: Fixed some typos and extended pthread_cancel comments.
>>
>>
>> since this commit various cancel tests fail for me (unreliably)
>> on aarch64 e.g. failures from 2 different test runs:
>>
>> FAIL: nptl/tst-cancel17
>> FAIL: nptl/tst-cancelx5
>> FAIL: nptl/tst-cond7
>> FAIL: nptl/tst-pthread-raise-blocked-self
>> FAIL: nptl/tst-pthread_cancel-select-loop
>>
>> FAIL: nptl/tst-cancelx20
>> FAIL: nptl/tst-cond7
>> FAIL: nptl/tst-cond8
>> FAIL: nptl/tst-join12
>> FAIL: nptl/tst-key3
>> FAIL: nptl/tst-pthread_cancel-select-loop
>>
>> an example run of nptl/tst-cond7
>>
>> $ elf/ld.so --library-path . nptl/tst-cond7 --direct
>> round 0
>> child created
>> parent: joining now
>> round 1
>> child created
>> parent: joining now
>> round 2
>> child created
>> parent: joining now
>> round 3
>> child created
>> parent: joining now
>> round 4
>> child created
>> parent: joining now
>>
>> where it is blocked forever: it seems pthread_cancel returns without
>> sending a signal (__pthread_kill_internal is not called) so join hangs.
>>
>
> But the signal should be only sent if thread is is cancelled and has
> async cancellation enabled, which is not the case for the tests.
Scratch that, I keep forgetting about __pthread_enable_asynccancel....
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)
2022-04-14 15:49 [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029) Adhemerval Zanella
2022-04-14 18:26 ` Adhemerval Zanella
2022-04-19 10:44 ` Szabolcs Nagy
@ 2022-04-19 12:10 ` Szabolcs Nagy
2022-04-19 12:30 ` Adhemerval Zanella
2022-07-12 21:27 ` Noah Goldstein
3 siblings, 1 reply; 12+ messages in thread
From: Szabolcs Nagy @ 2022-04-19 12:10 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, Florian Weimer, Aurelien Jarno
The 04/14/2022 12:49, Adhemerval Zanella via Libc-alpha wrote:
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
...
> + /* 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 cannot send SIGCANCEL unless the cancellation
> + 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 setting 'cancelhandling' and the cancelation will be acted
> + upon on next cancellation entrypoing in the target thread.
> +
> + It also requires to atomically check if cancellation is enabled and
> + asynchronous, so both cancellation state and type are tracked on
> + 'cancelhandling'. */
> +
> + int result = 0;
> + int oldval = atomic_load_relaxed (&pd->cancelhandling);
> + int newval;
> + do
> {
> - /* A single-threaded process should be able to kill itself, since there
> - is nothing in the POSIX specification that says that it cannot. So
> - we set multiple_threads to true so that cancellation points get
> - executed. */
> - THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
> + newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
> + if (oldval == newval)
> + break;
> +
> + /* If the cancellation is handled asynchronously just send a
> + signal. We avoid this if possible since it's more
> + expensive. */
> + if (cancel_enabled_and_canceled_and_async (newval))
> + {
> + /* Mark the cancellation as "in progress". */
> + int newval2 = oldval | CANCELING_BITMASK;
> + if (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling,
> + &oldval, newval2))
> + continue;
this continue looks wrong, the cas can fail spuriously
(cancelhandling == oldval) and then continue jumps to...
> +
> + if (pd == THREAD_SELF)
> + /* This is not merely an optimization: An application may
> + call pthread_cancel (pthread_self ()) without calling
> + pthread_create, so the signal handler may not have been
> + set up for a self-cancel. */
> + {
> + pd->result = PTHREAD_CANCELED;
> + if ((newval & CANCELTYPE_BITMASK) != 0)
> + __do_cancel ();
> + }
> + else
> + /* The cancellation handler will take care of marking the
> + thread as canceled. */
> + result = __pthread_kill_internal (th, SIGCANCEL);
> +
> + break;
> + }
> +
> + /* A single-threaded process should be able to kill itself, since
> + there is nothing in the POSIX specification that says that it
> + cannot. So we set multiple_threads to true so that cancellation
> + points get executed. */
> + THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
> #ifndef TLS_MULTIPLE_THREADS_IN_TCB
> __libc_multiple_threads = 1;
> #endif
> -
> - THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
> - if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
> - && pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> - __do_cancel ();
> - return 0;
> }
> + while (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling, &oldval,
> + newval));
...here and this cas updates cancelhandling to newval without
actually doing any cancellation.
>
> - return __pthread_kill_internal (th, SIGCANCEL);
> + return result;
> }
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)
2022-04-19 12:10 ` Szabolcs Nagy
@ 2022-04-19 12:30 ` Adhemerval Zanella
2022-04-19 12:46 ` Szabolcs Nagy
0 siblings, 1 reply; 12+ messages in thread
From: Adhemerval Zanella @ 2022-04-19 12:30 UTC (permalink / raw)
To: Szabolcs Nagy; +Cc: libc-alpha, Florian Weimer, Aurelien Jarno
On 19/04/2022 09:10, Szabolcs Nagy wrote:
> The 04/14/2022 12:49, Adhemerval Zanella via Libc-alpha wrote:
>> --- a/nptl/pthread_cancel.c
>> +++ b/nptl/pthread_cancel.c
> ...
>> + /* 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 cannot send SIGCANCEL unless the cancellation
>> + 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 setting 'cancelhandling' and the cancelation will be acted
>> + upon on next cancellation entrypoing in the target thread.
>> +
>> + It also requires to atomically check if cancellation is enabled and
>> + asynchronous, so both cancellation state and type are tracked on
>> + 'cancelhandling'. */
>> +
>> + int result = 0;
>> + int oldval = atomic_load_relaxed (&pd->cancelhandling);
>> + int newval;
>> + do
>> {
>> - /* A single-threaded process should be able to kill itself, since there
>> - is nothing in the POSIX specification that says that it cannot. So
>> - we set multiple_threads to true so that cancellation points get
>> - executed. */
>> - THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
>> + newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
>> + if (oldval == newval)
>> + break;
>> +
>> + /* If the cancellation is handled asynchronously just send a
>> + signal. We avoid this if possible since it's more
>> + expensive. */
>> + if (cancel_enabled_and_canceled_and_async (newval))
>> + {
>> + /* Mark the cancellation as "in progress". */
>> + int newval2 = oldval | CANCELING_BITMASK;
>> + if (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling,
>> + &oldval, newval2))
>> + continue;
>
> this continue looks wrong, the cas can fail spuriously
> (cancelhandling == oldval) and then continue jumps to...
>
>> +
>> + if (pd == THREAD_SELF)
>> + /* This is not merely an optimization: An application may
>> + call pthread_cancel (pthread_self ()) without calling
>> + pthread_create, so the signal handler may not have been
>> + set up for a self-cancel. */
>> + {
>> + pd->result = PTHREAD_CANCELED;
>> + if ((newval & CANCELTYPE_BITMASK) != 0)
>> + __do_cancel ();
>> + }
>> + else
>> + /* The cancellation handler will take care of marking the
>> + thread as canceled. */
>> + result = __pthread_kill_internal (th, SIGCANCEL);
>> +
>> + break;
>> + }
>> +
>> + /* A single-threaded process should be able to kill itself, since
>> + there is nothing in the POSIX specification that says that it
>> + cannot. So we set multiple_threads to true so that cancellation
>> + points get executed. */
>> + THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
>> #ifndef TLS_MULTIPLE_THREADS_IN_TCB
>> __libc_multiple_threads = 1;
>> #endif
>> -
>> - THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
>> - if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
>> - && pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
>> - __do_cancel ();
>> - return 0;
>> }
>> + while (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling, &oldval,
>> + newval));
>
> ...here and this cas updates cancelhandling to newval without
> actually doing any cancellation.
Yeah, it looks wrong indeed thanks for catching it. Old code has a goto that
I tried to remove (wrongly):
do
{
again:
oldval = pd->cancelhandling;
newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
[...]
if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
{
if (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling,
oldval | CANCELING_BITMASK,
oldval);
goto again;
[...]
}
[...]
}
while (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling, newval,
oldval);
Do this fix the intermittent issue you are seeing:
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index c76882e279..e67b2df5cc 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -121,6 +121,7 @@ __pthread_cancel (pthread_t th)
int newval;
do
{
+ again:
newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
if (oldval == newval)
break;
@@ -134,7 +135,7 @@ __pthread_cancel (pthread_t th)
int newval2 = oldval | CANCELING_BITMASK;
if (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling,
&oldval, newval2))
- continue;
+ goto again;
if (pd == THREAD_SELF)
/* This is not merely an optimization: An application may
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)
2022-04-19 12:30 ` Adhemerval Zanella
@ 2022-04-19 12:46 ` Szabolcs Nagy
2022-04-19 13:12 ` Adhemerval Zanella
0 siblings, 1 reply; 12+ messages in thread
From: Szabolcs Nagy @ 2022-04-19 12:46 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, Florian Weimer, Aurelien Jarno
The 04/19/2022 09:30, Adhemerval Zanella wrote:
> Do this fix the intermittent issue you are seeing:
>
yes, this seems to fix the issue.
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index c76882e279..e67b2df5cc 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -121,6 +121,7 @@ __pthread_cancel (pthread_t th)
> int newval;
> do
> {
> + again:
> newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
> if (oldval == newval)
> break;
> @@ -134,7 +135,7 @@ __pthread_cancel (pthread_t th)
> int newval2 = oldval | CANCELING_BITMASK;
> if (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling,
> &oldval, newval2))
> - continue;
> + goto again;
>
> if (pd == THREAD_SELF)
> /* This is not merely an optimization: An application may
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)
2022-04-19 12:46 ` Szabolcs Nagy
@ 2022-04-19 13:12 ` Adhemerval Zanella
0 siblings, 0 replies; 12+ messages in thread
From: Adhemerval Zanella @ 2022-04-19 13:12 UTC (permalink / raw)
To: Szabolcs Nagy; +Cc: libc-alpha, Florian Weimer, Aurelien Jarno
On 19/04/2022 09:46, Szabolcs Nagy wrote:
> The 04/19/2022 09:30, Adhemerval Zanella wrote:
>> Do this fix the intermittent issue you are seeing:
>>
>
> yes, this seems to fix the issue.
Thanks, I think this is obvious enough since it is essentially the old
code (sorry about trying to be clever here). I will fix it upstream and
backport to affected releases as well.
>
>> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
>> index c76882e279..e67b2df5cc 100644
>> --- a/nptl/pthread_cancel.c
>> +++ b/nptl/pthread_cancel.c
>> @@ -121,6 +121,7 @@ __pthread_cancel (pthread_t th)
>> int newval;
>> do
>> {
>> + again:
>> newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
>> if (oldval == newval)
>> break;
>> @@ -134,7 +135,7 @@ __pthread_cancel (pthread_t th)
>> int newval2 = oldval | CANCELING_BITMASK;
>> if (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling,
>> &oldval, newval2))
>> - continue;
>> + goto again;
>>
>> if (pd == THREAD_SELF)
>> /* This is not merely an optimization: An application may
>>
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)
2022-04-14 15:49 [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029) Adhemerval Zanella
` (2 preceding siblings ...)
2022-04-19 12:10 ` Szabolcs Nagy
@ 2022-07-12 21:27 ` Noah Goldstein
2022-07-12 21:28 ` Noah Goldstein
2022-07-13 12:57 ` Adhemerval Zanella Netto
3 siblings, 2 replies; 12+ messages in thread
From: Noah Goldstein @ 2022-07-12 21:27 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: GNU C Library, Florian Weimer, Aurelien Jarno
On Thu, Apr 14, 2022 at 8:51 AM Adhemerval Zanella via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> Some Linux interfaces never restart after being interrupted by a signal
> handler, regardless of the use of SA_RESTART [1]. It means that for
> pthread cancellation, if the target thread disables cancellation with
> pthread_setcancelstate and calls such interfaces (like poll or select),
> it should not see spurious EINTR failures due the internal SIGCANCEL.
>
> However recent changes made pthread_cancel to always sent the internal
> signal, regardless of the target thread cancellation status or type.
> To fix it, the previous semantic is restored, where the cancel signal
> is only sent if the target thread has cancelation enabled in
> asynchronous mode.
>
> The cancel state and cancel type is moved back to cancelhandling
> and atomic operation are used to synchronize between threads. The
> patch essentially revert the following commits:
>
> 8c1c0aae20 nptl: Move cancel type out of cancelhandling
> 2b51742531 nptl: Move cancel state out of cancelhandling
> 26cfbb7162 nptl: Remove CANCELING_BITMASK
>
> However I changed the atomic operation to follow the internal C11
> semantic and removed the MACRO usage, it simplifies a bit the
> resulting code (and removes another usage of the old atomic macros).
>
> Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu,
> and powerpc64-linux-gnu.
>
> [1] https://man7.org/linux/man-pages/man7/signal.7.html
>
> Reviewed-by: Florian Weimer <fweimer@redhat.com>
> Tested-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
> v2: Fixed some typos and extended pthread_cancel comments.
> ---
> manual/process.texi | 3 +-
> nptl/allocatestack.c | 2 -
> nptl/cancellation.c | 50 ++++++--
> nptl/cleanup_defer.c | 42 ++++++-
> nptl/descr.h | 41 +++++--
> nptl/libc-cleanup.c | 39 ++++++-
> nptl/pthread_cancel.c | 110 +++++++++++++-----
> nptl/pthread_join_common.c | 7 +-
> nptl/pthread_setcancelstate.c | 26 ++++-
> nptl/pthread_setcanceltype.c | 31 ++++-
> nptl/pthread_testcancel.c | 9 +-
> sysdeps/nptl/dl-tls_init_tp.c | 3 -
> sysdeps/nptl/pthreadP.h | 2 +-
> sysdeps/pthread/Makefile | 1 +
> sysdeps/pthread/tst-cancel29.c | 207 +++++++++++++++++++++++++++++++++
> 15 files changed, 482 insertions(+), 91 deletions(-)
> create mode 100644 sysdeps/pthread/tst-cancel29.c
>
> diff --git a/manual/process.texi b/manual/process.texi
> index 28c9531f42..9307379194 100644
> --- a/manual/process.texi
> +++ b/manual/process.texi
> @@ -68,8 +68,7 @@ until the subprogram terminates before you can do anything else.
> @c CLEANUP_HANDLER @ascuplugin @ascuheap @acsmem
> @c libc_cleanup_region_start @ascuplugin @ascuheap @acsmem
> @c pthread_cleanup_push_defer @ascuplugin @ascuheap @acsmem
> -@c __pthread_testcancel @ascuplugin @ascuheap @acsmem
> -@c CANCEL_ENABLED_AND_CANCELED ok
> +@c cancel_enabled_and_canceled @ascuplugin @ascuheap @acsmem
> @c do_cancel @ascuplugin @ascuheap @acsmem
> @c cancel_handler ok
> @c kill syscall ok
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 34a33164ff..01a282f3f6 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -119,8 +119,6 @@ get_cached_stack (size_t *sizep, void **memp)
>
> /* Cancellation handling is back to the default. */
> result->cancelhandling = 0;
> - result->cancelstate = PTHREAD_CANCEL_ENABLE;
> - result->canceltype = PTHREAD_CANCEL_DEFERRED;
> result->cleanup = NULL;
> result->setup_failed = 0;
>
> diff --git a/nptl/cancellation.c b/nptl/cancellation.c
> index 8d54a6add1..f4b08902b2 100644
> --- a/nptl/cancellation.c
> +++ b/nptl/cancellation.c
> @@ -30,19 +30,26 @@ int
> __pthread_enable_asynccancel (void)
> {
> struct pthread *self = THREAD_SELF;
> + int oldval = atomic_load_relaxed (&self->cancelhandling);
>
> - int oldval = THREAD_GETMEM (self, canceltype);
> - THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_ASYNCHRONOUS);
> + while (1)
> + {
> + int newval = oldval | CANCELTYPE_BITMASK;
>
> - int ch = THREAD_GETMEM (self, cancelhandling);
> + if (newval == oldval)
> + break;
>
> - if (self->cancelstate == PTHREAD_CANCEL_ENABLE
> - && (ch & CANCELED_BITMASK)
> - && !(ch & EXITING_BITMASK)
> - && !(ch & TERMINATED_BITMASK))
> - {
> - THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> - __do_cancel ();
> + if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &oldval, newval))
> + {
> + if (cancel_enabled_and_canceled_and_async (newval))
> + {
> + self->result = PTHREAD_CANCELED;
> + __do_cancel ();
> + }
> +
> + break;
> + }
> }
>
> return oldval;
> @@ -56,10 +63,29 @@ __pthread_disable_asynccancel (int oldtype)
> {
> /* If asynchronous cancellation was enabled before we do not have
> anything to do. */
> - if (oldtype == PTHREAD_CANCEL_ASYNCHRONOUS)
> + if (oldtype & CANCELTYPE_BITMASK)
> return;
>
> struct pthread *self = THREAD_SELF;
> - self->canceltype = PTHREAD_CANCEL_DEFERRED;
> + int newval;
> + int oldval = atomic_load_relaxed (&self->cancelhandling);
> + do
> + {
> + newval = oldval & ~CANCELTYPE_BITMASK;
> + }
> + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &oldval, newval));
> +
> + /* 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);
> + }
> }
> libc_hidden_def (__pthread_disable_asynccancel)
> diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
> index f8181a40e8..eb0bc77740 100644
> --- a/nptl/cleanup_defer.c
> +++ b/nptl/cleanup_defer.c
> @@ -30,9 +30,22 @@ ___pthread_register_cancel_defer (__pthread_unwind_buf_t *buf)
> ibuf->priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
> ibuf->priv.data.cleanup = THREAD_GETMEM (self, cleanup);
>
> - /* Disable asynchronous cancellation for now. */
> - ibuf->priv.data.canceltype = THREAD_GETMEM (self, canceltype);
> - THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
> + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> + if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
> + {
> + int newval;
> + do
> + {
> + newval = cancelhandling & ~CANCELTYPE_BITMASK;
> + }
> + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &cancelhandling,
> + newval));
> + }
> +
> + ibuf->priv.data.canceltype = (cancelhandling & CANCELTYPE_BITMASK
> + ? PTHREAD_CANCEL_ASYNCHRONOUS
> + : PTHREAD_CANCEL_DEFERRED);
>
> /* Store the new cleanup handler info. */
> THREAD_SETMEM (self, cleanup_jmp_buf, (struct pthread_unwind_buf *) buf);
> @@ -54,9 +67,26 @@ ___pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
>
> THREAD_SETMEM (self, cleanup_jmp_buf, ibuf->priv.data.prev);
>
> - THREAD_SETMEM (self, canceltype, ibuf->priv.data.canceltype);
> - if (ibuf->priv.data.canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> - __pthread_testcancel ();
> + if (ibuf->priv.data.canceltype == PTHREAD_CANCEL_DEFERRED)
> + return;
> +
> + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> + if (cancelhandling & CANCELTYPE_BITMASK)
should this be:
if((cancelhandling & CANCELTYPE_BITMASK) == 0)
?
> + {
> + int newval;
> + do
> + {
> + newval = cancelhandling | CANCELTYPE_BITMASK;
> + }
> + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &cancelhandling, newval));
> +
> + if (cancel_enabled_and_canceled (cancelhandling))
> + {
> + self->result = PTHREAD_CANCELED;
> + __do_cancel ();
> + }
> + }
> }
> versioned_symbol (libc, ___pthread_unregister_cancel_restore,
> __pthread_unregister_cancel_restore, GLIBC_2_34);
> diff --git a/nptl/descr.h b/nptl/descr.h
> index ea8aca08e6..bb46b5958e 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -279,18 +279,27 @@ struct pthread
>
> /* Flags determining processing of cancellation. */
> int cancelhandling;
> + /* Bit set if cancellation is disabled. */
> +#define CANCELSTATE_BIT 0
> +#define CANCELSTATE_BITMASK (1 << CANCELSTATE_BIT)
> + /* Bit set if asynchronous cancellation mode is selected. */
> +#define CANCELTYPE_BIT 1
> +#define CANCELTYPE_BITMASK (1 << CANCELTYPE_BIT)
> + /* Bit set if canceling has been initiated. */
> +#define CANCELING_BIT 2
> +#define CANCELING_BITMASK (1 << CANCELING_BIT)
> /* Bit set if canceled. */
> #define CANCELED_BIT 3
> -#define CANCELED_BITMASK (0x01 << CANCELED_BIT)
> +#define CANCELED_BITMASK (1 << CANCELED_BIT)
> /* Bit set if thread is exiting. */
> #define EXITING_BIT 4
> -#define EXITING_BITMASK (0x01 << EXITING_BIT)
> +#define EXITING_BITMASK (1 << EXITING_BIT)
> /* Bit set if thread terminated and TCB is freed. */
> #define TERMINATED_BIT 5
> -#define TERMINATED_BITMASK (0x01 << TERMINATED_BIT)
> +#define TERMINATED_BITMASK (1 << TERMINATED_BIT)
> /* Bit set if thread is supposed to change XID. */
> #define SETXID_BIT 6
> -#define SETXID_BITMASK (0x01 << SETXID_BIT)
> +#define SETXID_BITMASK (1 << SETXID_BIT)
>
> /* Flags. Including those copied from the thread attribute. */
> int flags;
> @@ -390,14 +399,6 @@ struct pthread
> /* Indicates whether is a C11 thread created by thrd_creat. */
> bool c11;
>
> - /* Thread cancel state (PTHREAD_CANCEL_ENABLE or
> - PTHREAD_CANCEL_DISABLE). */
> - unsigned char cancelstate;
> -
> - /* Thread cancel type (PTHREAD_CANCEL_DEFERRED or
> - PTHREAD_CANCEL_ASYNCHRONOUS). */
> - unsigned char canceltype;
> -
> /* Used in __pthread_kill_internal to detected a thread that has
> exited or is about to exit. exit_lock must only be acquired
> after blocking signals. */
> @@ -417,6 +418,22 @@ struct pthread
> (sizeof (struct pthread) - offsetof (struct pthread, end_padding))
> } __attribute ((aligned (TCB_ALIGNMENT)));
>
> +static inline bool
> +cancel_enabled_and_canceled (int value)
> +{
> + return (value & (CANCELSTATE_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
> + | TERMINATED_BITMASK))
> + == CANCELED_BITMASK;
> +}
> +
> +static inline bool
> +cancel_enabled_and_canceled_and_async (int value)
> +{
> + return ((value) & (CANCELSTATE_BITMASK | CANCELTYPE_BITMASK | CANCELED_BITMASK
> + | EXITING_BITMASK | TERMINATED_BITMASK))
> + == (CANCELTYPE_BITMASK | CANCELED_BITMASK);
> +}
> +
> /* This yields the pointer that TLS support code calls the thread pointer. */
> #if TLS_TCB_AT_TP
> # define TLS_TPADJ(pd) (pd)
> diff --git a/nptl/libc-cleanup.c b/nptl/libc-cleanup.c
> index cb4c226281..c4a83591bf 100644
> --- a/nptl/libc-cleanup.c
> +++ b/nptl/libc-cleanup.c
> @@ -26,9 +26,24 @@ __libc_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer)
>
> buffer->__prev = THREAD_GETMEM (self, cleanup);
>
> + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> +
> /* Disable asynchronous cancellation for now. */
> - buffer->__canceltype = THREAD_GETMEM (self, canceltype);
> - THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
> + if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
> + {
> + int newval;
> + do
> + {
> + newval = cancelhandling & ~CANCELTYPE_BITMASK;
> + }
> + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &cancelhandling,
> + newval));
> + }
> +
> + buffer->__canceltype = (cancelhandling & CANCELTYPE_BITMASK
> + ? PTHREAD_CANCEL_ASYNCHRONOUS
> + : PTHREAD_CANCEL_DEFERRED);
>
> THREAD_SETMEM (self, cleanup, buffer);
> }
> @@ -41,8 +56,22 @@ __libc_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer)
>
> THREAD_SETMEM (self, cleanup, buffer->__prev);
>
> - THREAD_SETMEM (self, canceltype, buffer->__canceltype);
> - if (buffer->__canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> - __pthread_testcancel ();
> + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> + if (cancelhandling & CANCELTYPE_BITMASK)
> + {
> + int newval;
> + do
> + {
> + newval = cancelhandling | CANCELTYPE_BITMASK;
> + }
> + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &cancelhandling, newval));
> +
> + if (cancel_enabled_and_canceled (cancelhandling))
> + {
> + self->result = PTHREAD_CANCELED;
> + __do_cancel ();
> + }
> + }
> }
> libc_hidden_def (__libc_cleanup_pop_restore)
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index 7524c7ce4d..c76882e279 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c
> @@ -42,18 +42,29 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
>
> struct pthread *self = THREAD_SELF;
>
> - int ch = atomic_load_relaxed (&self->cancelhandling);
> - /* Cancelation not enabled, not cancelled, or already exitting. */
> - if (self->cancelstate == PTHREAD_CANCEL_DISABLE
> - || (ch & CANCELED_BITMASK) == 0
> - || (ch & EXITING_BITMASK) != 0)
> - return;
> -
> - /* Set the return value. */
> - THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> - /* Make sure asynchronous cancellation is still enabled. */
> - if (self->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> - __do_cancel ();
> + int oldval = atomic_load_relaxed (&self->cancelhandling);
> + while (1)
> + {
> + /* We are canceled now. When canceled by another thread this flag
> + is already set but if the signal is directly send (internally or
> + from another process) is has to be done here. */
> + int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
> +
> + if (oldval == newval || (oldval & EXITING_BITMASK) != 0)
> + /* Already canceled or exiting. */
> + break;
> +
> + if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &oldval, newval))
> + {
> + self->result = PTHREAD_CANCELED;
> +
> + /* Make sure asynchronous cancellation is still enabled. */
> + if ((oldval & CANCELTYPE_BITMASK) != 0)
> + /* Run the registered destructors and terminate the thread. */
> + __do_cancel ();
> + }
> + }
> }
>
> int
> @@ -92,29 +103,70 @@ __pthread_cancel (pthread_t th)
> }
> #endif
>
> - int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK);
> - if ((oldch & CANCELED_BITMASK) != 0)
> - return 0;
> -
> - if (pd == THREAD_SELF)
> + /* 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 cannot send SIGCANCEL unless the cancellation
> + 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 setting 'cancelhandling' and the cancelation will be acted
> + upon on next cancellation entrypoing in the target thread.
> +
> + It also requires to atomically check if cancellation is enabled and
> + asynchronous, so both cancellation state and type are tracked on
> + 'cancelhandling'. */
> +
> + int result = 0;
> + int oldval = atomic_load_relaxed (&pd->cancelhandling);
> + int newval;
> + do
> {
> - /* A single-threaded process should be able to kill itself, since there
> - is nothing in the POSIX specification that says that it cannot. So
> - we set multiple_threads to true so that cancellation points get
> - executed. */
> - THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
> + newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
> + if (oldval == newval)
> + break;
> +
> + /* If the cancellation is handled asynchronously just send a
> + signal. We avoid this if possible since it's more
> + expensive. */
> + if (cancel_enabled_and_canceled_and_async (newval))
> + {
> + /* Mark the cancellation as "in progress". */
> + int newval2 = oldval | CANCELING_BITMASK;
> + if (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling,
> + &oldval, newval2))
> + continue;
> +
> + if (pd == THREAD_SELF)
> + /* This is not merely an optimization: An application may
> + call pthread_cancel (pthread_self ()) without calling
> + pthread_create, so the signal handler may not have been
> + set up for a self-cancel. */
> + {
> + pd->result = PTHREAD_CANCELED;
> + if ((newval & CANCELTYPE_BITMASK) != 0)
> + __do_cancel ();
> + }
> + else
> + /* The cancellation handler will take care of marking the
> + thread as canceled. */
> + result = __pthread_kill_internal (th, SIGCANCEL);
> +
> + break;
> + }
> +
> + /* A single-threaded process should be able to kill itself, since
> + there is nothing in the POSIX specification that says that it
> + cannot. So we set multiple_threads to true so that cancellation
> + points get executed. */
> + THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
> #ifndef TLS_MULTIPLE_THREADS_IN_TCB
> __libc_multiple_threads = 1;
> #endif
> -
> - THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
> - if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
> - && pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> - __do_cancel ();
> - return 0;
> }
> + while (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling, &oldval,
> + newval));
>
> - return __pthread_kill_internal (th, SIGCANCEL);
> + return result;
> }
> versioned_symbol (libc, __pthread_cancel, pthread_cancel, GLIBC_2_34);
>
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index a8e884f341..ca3245b0af 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -57,12 +57,9 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
> if ((pd == self
> || (self->joinid == pd
> && (pd->cancelhandling
> - & (CANCELED_BITMASK | EXITING_BITMASK
> + & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
> | TERMINATED_BITMASK)) == 0))
> - && !(self->cancelstate == PTHREAD_CANCEL_ENABLE
> - && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
> - | TERMINATED_BITMASK))
> - == CANCELED_BITMASK))
> + && !cancel_enabled_and_canceled (self->cancelhandling))
> /* This is a deadlock situation. 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
> diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
> index 9905b12e4f..f8edf18fbe 100644
> --- a/nptl/pthread_setcancelstate.c
> +++ b/nptl/pthread_setcancelstate.c
> @@ -30,9 +30,29 @@ __pthread_setcancelstate (int state, int *oldstate)
>
> self = THREAD_SELF;
>
> - if (oldstate != NULL)
> - *oldstate = self->cancelstate;
> - self->cancelstate = state;
> + int oldval = atomic_load_relaxed (&self->cancelhandling);
> + while (1)
> + {
> + int newval = (state == PTHREAD_CANCEL_DISABLE
> + ? oldval | CANCELSTATE_BITMASK
> + : oldval & ~CANCELSTATE_BITMASK);
> +
> + if (oldstate != NULL)
> + *oldstate = ((oldval & CANCELSTATE_BITMASK)
> + ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
> +
> + if (oldval == newval)
> + break;
> +
> + if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &oldval, newval))
> + {
> + if (cancel_enabled_and_canceled_and_async (newval))
> + __do_cancel ();
> +
> + break;
> + }
> + }
>
> return 0;
> }
> diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
> index 94e56466de..1307d355c1 100644
> --- a/nptl/pthread_setcanceltype.c
> +++ b/nptl/pthread_setcanceltype.c
> @@ -28,11 +28,32 @@ __pthread_setcanceltype (int type, int *oldtype)
>
> volatile struct pthread *self = THREAD_SELF;
>
> - if (oldtype != NULL)
> - *oldtype = self->canceltype;
> - self->canceltype = type;
> - if (type == PTHREAD_CANCEL_ASYNCHRONOUS)
> - __pthread_testcancel ();
> + int oldval = atomic_load_relaxed (&self->cancelhandling);
> + while (1)
> + {
> + int newval = (type == PTHREAD_CANCEL_ASYNCHRONOUS
> + ? oldval | CANCELTYPE_BITMASK
> + : oldval & ~CANCELTYPE_BITMASK);
> +
> + if (oldtype != NULL)
> + *oldtype = ((oldval & CANCELTYPE_BITMASK)
> + ? PTHREAD_CANCEL_ASYNCHRONOUS : PTHREAD_CANCEL_DEFERRED);
> +
> + if (oldval == newval)
> + break;
> +
> + if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> + &oldval, newval))
> + {
> + if (cancel_enabled_and_canceled_and_async (newval))
> + {
> + THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> + __do_cancel ();
> + }
> +
> + break;
> + }
> + }
>
> return 0;
> }
> diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
> index 13123608e8..b81928c000 100644
> --- a/nptl/pthread_testcancel.c
> +++ b/nptl/pthread_testcancel.c
> @@ -23,13 +23,10 @@ void
> ___pthread_testcancel (void)
> {
> struct pthread *self = THREAD_SELF;
> - int cancelhandling = THREAD_GETMEM (self, cancelhandling);
> - if (self->cancelstate == PTHREAD_CANCEL_ENABLE
> - && (cancelhandling & CANCELED_BITMASK)
> - && !(cancelhandling & EXITING_BITMASK)
> - && !(cancelhandling & TERMINATED_BITMASK))
> + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> + if (cancel_enabled_and_canceled (cancelhandling))
> {
> - THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> + self->result = PTHREAD_CANCELED;
> __do_cancel ();
> }
> }
> diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
> index 1294c91816..53fba774a5 100644
> --- a/sysdeps/nptl/dl-tls_init_tp.c
> +++ b/sysdeps/nptl/dl-tls_init_tp.c
> @@ -128,7 +128,4 @@ __tls_init_tp (void)
> It will be bigger than it actually is, but for unwind.c/pt-longjmp.c
> purposes this is good enough. */
> THREAD_SETMEM (pd, stackblock_size, (size_t) __libc_stack_end);
> -
> - THREAD_SETMEM (pd, cancelstate, PTHREAD_CANCEL_ENABLE);
> - THREAD_SETMEM (pd, canceltype, PTHREAD_CANCEL_DEFERRED);
> }
> diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
> index 708bd92469..601db4ff2b 100644
> --- a/sysdeps/nptl/pthreadP.h
> +++ b/sysdeps/nptl/pthreadP.h
> @@ -275,7 +275,7 @@ __do_cancel (void)
> struct pthread *self = THREAD_SELF;
>
> /* Make sure we get no more cancellations. */
> - THREAD_ATOMIC_BIT_SET (self, cancelhandling, EXITING_BIT);
> + atomic_bit_set (&self->cancelhandling, EXITING_BIT);
>
> __pthread_unwind ((__pthread_unwind_buf_t *)
> THREAD_GETMEM (self, cleanup_jmp_buf));
> diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> index 318dfdc04a..e901c51df0 100644
> --- a/sysdeps/pthread/Makefile
> +++ b/sysdeps/pthread/Makefile
> @@ -69,6 +69,7 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
> tst-cancel12 tst-cancel13 tst-cancel14 tst-cancel15 tst-cancel16 \
> tst-cancel18 tst-cancel19 tst-cancel20 tst-cancel21 \
> tst-cancel22 tst-cancel23 tst-cancel26 tst-cancel27 tst-cancel28 \
> + tst-cancel29 \
> tst-cleanup0 tst-cleanup1 tst-cleanup2 tst-cleanup3 \
> tst-clock1 \
> tst-cond-except \
> diff --git a/sysdeps/pthread/tst-cancel29.c b/sysdeps/pthread/tst-cancel29.c
> new file mode 100644
> index 0000000000..4f0d99e002
> --- /dev/null
> +++ b/sysdeps/pthread/tst-cancel29.c
> @@ -0,0 +1,207 @@
> +/* Check if a thread that disables cancellation and which call functions
> + that might be interrupted by a signal do not see the internal SIGCANCEL.
> +
> + Copyright (C) 2022 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <array_length.h>
> +#include <errno.h>
> +#include <inttypes.h>
> +#include <poll.h>
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +#include <support/xthread.h>
> +#include <sys/socket.h>
> +#include <signal.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +
> +/* On Linux some interfaces are never restarted after being interrupted by
> + a signal handler, regardless of the use of SA_RESTART. It means that
> + if asynchronous cancellation is not enabled, the pthread_cancel can not
> + set the internal SIGCANCEL otherwise the interface might see a spurious
> + EINTR failure. */
> +
> +static pthread_barrier_t b;
> +
> +/* Cleanup handling test. */
> +static int cl_called;
> +static void
> +cl (void *arg)
> +{
> + ++cl_called;
> +}
> +
> +static void *
> +tf_sigtimedwait (void *arg)
> +{
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> + xpthread_barrier_wait (&b);
> +
> + int r;
> + pthread_cleanup_push (cl, NULL);
> +
> + sigset_t mask;
> + sigemptyset (&mask);
> + r = sigtimedwait (&mask, NULL, &(struct timespec) { 0, 250000000 });
> + if (r != -1)
> + return (void*) -1;
> + if (errno != EAGAIN)
> + return (void*) -2;
> +
> + pthread_cleanup_pop (0);
> + return NULL;
> +}
> +
> +static void *
> +tf_poll (void *arg)
> +{
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> + xpthread_barrier_wait (&b);
> +
> + int r;
> + pthread_cleanup_push (cl, NULL);
> +
> + r = poll (NULL, 0, 250);
> + if (r != 0)
> + return (void*) -1;
> +
> + pthread_cleanup_pop (0);
> + return NULL;
> +}
> +
> +static void *
> +tf_ppoll (void *arg)
> +{
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> +
> + xpthread_barrier_wait (&b);
> +
> + int r;
> + pthread_cleanup_push (cl, NULL);
> +
> + r = ppoll (NULL, 0, &(struct timespec) { 0, 250000000 }, NULL);
> + if (r != 0)
> + return (void*) -1;
> +
> + pthread_cleanup_pop (0);
> + return NULL;
> +}
> +
> +static void *
> +tf_select (void *arg)
> +{
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> + xpthread_barrier_wait (&b);
> +
> + int r;
> + pthread_cleanup_push (cl, NULL);
> +
> + r = select (0, NULL, NULL, NULL, &(struct timeval) { 0, 250000 });
> + if (r != 0)
> + return (void*) -1;
> +
> + pthread_cleanup_pop (0);
> + return NULL;
> +}
> +
> +static void *
> +tf_pselect (void *arg)
> +{
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> + xpthread_barrier_wait (&b);
> +
> + int r;
> + pthread_cleanup_push (cl, NULL);
> +
> + r = pselect (0, NULL, NULL, NULL, &(struct timespec) { 0, 250000000 }, NULL);
> + if (r != 0)
> + return (void*) -1;
> +
> + pthread_cleanup_pop (0);
> + return NULL;
> +}
> +
> +static void *
> +tf_clock_nanosleep (void *arg)
> +{
> + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> + xpthread_barrier_wait (&b);
> +
> + int r;
> + pthread_cleanup_push (cl, NULL);
> +
> + r = clock_nanosleep (CLOCK_REALTIME, 0, &(struct timespec) { 0, 250000000 },
> + NULL);
> + if (r != 0)
> + return (void*) -1;
> +
> + pthread_cleanup_pop (0);
> + return NULL;
> +}
> +
> +struct cancel_test_t
> +{
> + const char *name;
> + void * (*cf) (void *);
> +} tests[] =
> +{
> + { "sigtimedwait", tf_sigtimedwait, },
> + { "poll", tf_poll, },
> + { "ppoll", tf_ppoll, },
> + { "select", tf_select, },
> + { "pselect", tf_pselect , },
> + { "clock_nanosleep", tf_clock_nanosleep, },
> +};
> +
> +static int
> +do_test (void)
> +{
> + for (int i = 0; i < array_length (tests); i++)
> + {
> + xpthread_barrier_init (&b, NULL, 2);
> +
> + cl_called = 0;
> +
> + pthread_t th = xpthread_create (NULL, tests[i].cf, NULL);
> +
> + xpthread_barrier_wait (&b);
> +
> + struct timespec ts = { .tv_sec = 0, .tv_nsec = 100000000 };
> + while (nanosleep (&ts, &ts) != 0)
> + continue;
> +
> + xpthread_cancel (th);
> +
> + void *status = xpthread_join (th);
> + if (status != NULL)
> + printf ("test '%s' failed: %" PRIdPTR "\n", tests[i].name,
> + (intptr_t) status);
> + TEST_VERIFY (status == NULL);
> +
> + xpthread_barrier_destroy (&b);
> +
> + TEST_COMPARE (cl_called, 0);
> +
> + printf ("in-time cancel test of '%s' successful\n", tests[i].name);
> + }
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)
2022-07-12 21:27 ` Noah Goldstein
@ 2022-07-12 21:28 ` Noah Goldstein
2022-07-13 12:57 ` Adhemerval Zanella Netto
1 sibling, 0 replies; 12+ messages in thread
From: Noah Goldstein @ 2022-07-12 21:28 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: GNU C Library, Florian Weimer, Aurelien Jarno
On Tue, Jul 12, 2022 at 2:27 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
> On Thu, Apr 14, 2022 at 8:51 AM Adhemerval Zanella via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> > Some Linux interfaces never restart after being interrupted by a signal
> > handler, regardless of the use of SA_RESTART [1]. It means that for
> > pthread cancellation, if the target thread disables cancellation with
> > pthread_setcancelstate and calls such interfaces (like poll or select),
> > it should not see spurious EINTR failures due the internal SIGCANCEL.
> >
> > However recent changes made pthread_cancel to always sent the internal
> > signal, regardless of the target thread cancellation status or type.
> > To fix it, the previous semantic is restored, where the cancel signal
> > is only sent if the target thread has cancelation enabled in
> > asynchronous mode.
> >
> > The cancel state and cancel type is moved back to cancelhandling
> > and atomic operation are used to synchronize between threads. The
> > patch essentially revert the following commits:
> >
> > 8c1c0aae20 nptl: Move cancel type out of cancelhandling
> > 2b51742531 nptl: Move cancel state out of cancelhandling
> > 26cfbb7162 nptl: Remove CANCELING_BITMASK
> >
> > However I changed the atomic operation to follow the internal C11
> > semantic and removed the MACRO usage, it simplifies a bit the
> > resulting code (and removes another usage of the old atomic macros).
> >
> > Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu,
> > and powerpc64-linux-gnu.
> >
> > [1] https://man7.org/linux/man-pages/man7/signal.7.html
> >
> > Reviewed-by: Florian Weimer <fweimer@redhat.com>
> > Tested-by: Aurelien Jarno <aurelien@aurel32.net>
> > ---
> > v2: Fixed some typos and extended pthread_cancel comments.
> > ---
> > manual/process.texi | 3 +-
> > nptl/allocatestack.c | 2 -
> > nptl/cancellation.c | 50 ++++++--
> > nptl/cleanup_defer.c | 42 ++++++-
> > nptl/descr.h | 41 +++++--
> > nptl/libc-cleanup.c | 39 ++++++-
> > nptl/pthread_cancel.c | 110 +++++++++++++-----
> > nptl/pthread_join_common.c | 7 +-
> > nptl/pthread_setcancelstate.c | 26 ++++-
> > nptl/pthread_setcanceltype.c | 31 ++++-
> > nptl/pthread_testcancel.c | 9 +-
> > sysdeps/nptl/dl-tls_init_tp.c | 3 -
> > sysdeps/nptl/pthreadP.h | 2 +-
> > sysdeps/pthread/Makefile | 1 +
> > sysdeps/pthread/tst-cancel29.c | 207 +++++++++++++++++++++++++++++++++
> > 15 files changed, 482 insertions(+), 91 deletions(-)
> > create mode 100644 sysdeps/pthread/tst-cancel29.c
> >
> > diff --git a/manual/process.texi b/manual/process.texi
> > index 28c9531f42..9307379194 100644
> > --- a/manual/process.texi
> > +++ b/manual/process.texi
> > @@ -68,8 +68,7 @@ until the subprogram terminates before you can do anything else.
> > @c CLEANUP_HANDLER @ascuplugin @ascuheap @acsmem
> > @c libc_cleanup_region_start @ascuplugin @ascuheap @acsmem
> > @c pthread_cleanup_push_defer @ascuplugin @ascuheap @acsmem
> > -@c __pthread_testcancel @ascuplugin @ascuheap @acsmem
> > -@c CANCEL_ENABLED_AND_CANCELED ok
> > +@c cancel_enabled_and_canceled @ascuplugin @ascuheap @acsmem
> > @c do_cancel @ascuplugin @ascuheap @acsmem
> > @c cancel_handler ok
> > @c kill syscall ok
> > diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> > index 34a33164ff..01a282f3f6 100644
> > --- a/nptl/allocatestack.c
> > +++ b/nptl/allocatestack.c
> > @@ -119,8 +119,6 @@ get_cached_stack (size_t *sizep, void **memp)
> >
> > /* Cancellation handling is back to the default. */
> > result->cancelhandling = 0;
> > - result->cancelstate = PTHREAD_CANCEL_ENABLE;
> > - result->canceltype = PTHREAD_CANCEL_DEFERRED;
> > result->cleanup = NULL;
> > result->setup_failed = 0;
> >
> > diff --git a/nptl/cancellation.c b/nptl/cancellation.c
> > index 8d54a6add1..f4b08902b2 100644
> > --- a/nptl/cancellation.c
> > +++ b/nptl/cancellation.c
> > @@ -30,19 +30,26 @@ int
> > __pthread_enable_asynccancel (void)
> > {
> > struct pthread *self = THREAD_SELF;
> > + int oldval = atomic_load_relaxed (&self->cancelhandling);
> >
> > - int oldval = THREAD_GETMEM (self, canceltype);
> > - THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_ASYNCHRONOUS);
> > + while (1)
> > + {
> > + int newval = oldval | CANCELTYPE_BITMASK;
> >
> > - int ch = THREAD_GETMEM (self, cancelhandling);
> > + if (newval == oldval)
> > + break;
> >
> > - if (self->cancelstate == PTHREAD_CANCEL_ENABLE
> > - && (ch & CANCELED_BITMASK)
> > - && !(ch & EXITING_BITMASK)
> > - && !(ch & TERMINATED_BITMASK))
> > - {
> > - THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> > - __do_cancel ();
> > + if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> > + &oldval, newval))
> > + {
> > + if (cancel_enabled_and_canceled_and_async (newval))
> > + {
> > + self->result = PTHREAD_CANCELED;
> > + __do_cancel ();
> > + }
> > +
> > + break;
> > + }
> > }
> >
> > return oldval;
> > @@ -56,10 +63,29 @@ __pthread_disable_asynccancel (int oldtype)
> > {
> > /* If asynchronous cancellation was enabled before we do not have
> > anything to do. */
> > - if (oldtype == PTHREAD_CANCEL_ASYNCHRONOUS)
> > + if (oldtype & CANCELTYPE_BITMASK)
> > return;
> >
> > struct pthread *self = THREAD_SELF;
> > - self->canceltype = PTHREAD_CANCEL_DEFERRED;
> > + int newval;
> > + int oldval = atomic_load_relaxed (&self->cancelhandling);
> > + do
> > + {
> > + newval = oldval & ~CANCELTYPE_BITMASK;
> > + }
> > + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> > + &oldval, newval));
> > +
> > + /* 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);
> > + }
> > }
> > libc_hidden_def (__pthread_disable_asynccancel)
> > diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
> > index f8181a40e8..eb0bc77740 100644
> > --- a/nptl/cleanup_defer.c
> > +++ b/nptl/cleanup_defer.c
> > @@ -30,9 +30,22 @@ ___pthread_register_cancel_defer (__pthread_unwind_buf_t *buf)
> > ibuf->priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
> > ibuf->priv.data.cleanup = THREAD_GETMEM (self, cleanup);
> >
> > - /* Disable asynchronous cancellation for now. */
> > - ibuf->priv.data.canceltype = THREAD_GETMEM (self, canceltype);
> > - THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
> > + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> > + if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
> > + {
> > + int newval;
> > + do
> > + {
> > + newval = cancelhandling & ~CANCELTYPE_BITMASK;
> > + }
> > + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> > + &cancelhandling,
> > + newval));
> > + }
> > +
> > + ibuf->priv.data.canceltype = (cancelhandling & CANCELTYPE_BITMASK
> > + ? PTHREAD_CANCEL_ASYNCHRONOUS
> > + : PTHREAD_CANCEL_DEFERRED);
> >
> > /* Store the new cleanup handler info. */
> > THREAD_SETMEM (self, cleanup_jmp_buf, (struct pthread_unwind_buf *) buf);
> > @@ -54,9 +67,26 @@ ___pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
> >
> > THREAD_SETMEM (self, cleanup_jmp_buf, ibuf->priv.data.prev);
> >
> > - THREAD_SETMEM (self, canceltype, ibuf->priv.data.canceltype);
> > - if (ibuf->priv.data.canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> > - __pthread_testcancel ();
> > + if (ibuf->priv.data.canceltype == PTHREAD_CANCEL_DEFERRED)
> > + return;
> > +
> > + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> > + if (cancelhandling & CANCELTYPE_BITMASK)
>
> should this be:
> if((cancelhandling & CANCELTYPE_BITMASK) == 0)
> ?
If not, can we remove the CAS?
> > + {
> > + int newval;
> > + do
> > + {
> > + newval = cancelhandling | CANCELTYPE_BITMASK;
> > + }
> > + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> > + &cancelhandling, newval));
> > +
> > + if (cancel_enabled_and_canceled (cancelhandling))
> > + {
> > + self->result = PTHREAD_CANCELED;
> > + __do_cancel ();
> > + }
> > + }
> > }
> > versioned_symbol (libc, ___pthread_unregister_cancel_restore,
> > __pthread_unregister_cancel_restore, GLIBC_2_34);
> > diff --git a/nptl/descr.h b/nptl/descr.h
> > index ea8aca08e6..bb46b5958e 100644
> > --- a/nptl/descr.h
> > +++ b/nptl/descr.h
> > @@ -279,18 +279,27 @@ struct pthread
> >
> > /* Flags determining processing of cancellation. */
> > int cancelhandling;
> > + /* Bit set if cancellation is disabled. */
> > +#define CANCELSTATE_BIT 0
> > +#define CANCELSTATE_BITMASK (1 << CANCELSTATE_BIT)
> > + /* Bit set if asynchronous cancellation mode is selected. */
> > +#define CANCELTYPE_BIT 1
> > +#define CANCELTYPE_BITMASK (1 << CANCELTYPE_BIT)
> > + /* Bit set if canceling has been initiated. */
> > +#define CANCELING_BIT 2
> > +#define CANCELING_BITMASK (1 << CANCELING_BIT)
> > /* Bit set if canceled. */
> > #define CANCELED_BIT 3
> > -#define CANCELED_BITMASK (0x01 << CANCELED_BIT)
> > +#define CANCELED_BITMASK (1 << CANCELED_BIT)
> > /* Bit set if thread is exiting. */
> > #define EXITING_BIT 4
> > -#define EXITING_BITMASK (0x01 << EXITING_BIT)
> > +#define EXITING_BITMASK (1 << EXITING_BIT)
> > /* Bit set if thread terminated and TCB is freed. */
> > #define TERMINATED_BIT 5
> > -#define TERMINATED_BITMASK (0x01 << TERMINATED_BIT)
> > +#define TERMINATED_BITMASK (1 << TERMINATED_BIT)
> > /* Bit set if thread is supposed to change XID. */
> > #define SETXID_BIT 6
> > -#define SETXID_BITMASK (0x01 << SETXID_BIT)
> > +#define SETXID_BITMASK (1 << SETXID_BIT)
> >
> > /* Flags. Including those copied from the thread attribute. */
> > int flags;
> > @@ -390,14 +399,6 @@ struct pthread
> > /* Indicates whether is a C11 thread created by thrd_creat. */
> > bool c11;
> >
> > - /* Thread cancel state (PTHREAD_CANCEL_ENABLE or
> > - PTHREAD_CANCEL_DISABLE). */
> > - unsigned char cancelstate;
> > -
> > - /* Thread cancel type (PTHREAD_CANCEL_DEFERRED or
> > - PTHREAD_CANCEL_ASYNCHRONOUS). */
> > - unsigned char canceltype;
> > -
> > /* Used in __pthread_kill_internal to detected a thread that has
> > exited or is about to exit. exit_lock must only be acquired
> > after blocking signals. */
> > @@ -417,6 +418,22 @@ struct pthread
> > (sizeof (struct pthread) - offsetof (struct pthread, end_padding))
> > } __attribute ((aligned (TCB_ALIGNMENT)));
> >
> > +static inline bool
> > +cancel_enabled_and_canceled (int value)
> > +{
> > + return (value & (CANCELSTATE_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
> > + | TERMINATED_BITMASK))
> > + == CANCELED_BITMASK;
> > +}
> > +
> > +static inline bool
> > +cancel_enabled_and_canceled_and_async (int value)
> > +{
> > + return ((value) & (CANCELSTATE_BITMASK | CANCELTYPE_BITMASK | CANCELED_BITMASK
> > + | EXITING_BITMASK | TERMINATED_BITMASK))
> > + == (CANCELTYPE_BITMASK | CANCELED_BITMASK);
> > +}
> > +
> > /* This yields the pointer that TLS support code calls the thread pointer. */
> > #if TLS_TCB_AT_TP
> > # define TLS_TPADJ(pd) (pd)
> > diff --git a/nptl/libc-cleanup.c b/nptl/libc-cleanup.c
> > index cb4c226281..c4a83591bf 100644
> > --- a/nptl/libc-cleanup.c
> > +++ b/nptl/libc-cleanup.c
> > @@ -26,9 +26,24 @@ __libc_cleanup_push_defer (struct _pthread_cleanup_buffer *buffer)
> >
> > buffer->__prev = THREAD_GETMEM (self, cleanup);
> >
> > + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> > +
> > /* Disable asynchronous cancellation for now. */
> > - buffer->__canceltype = THREAD_GETMEM (self, canceltype);
> > - THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
> > + if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
> > + {
> > + int newval;
> > + do
> > + {
> > + newval = cancelhandling & ~CANCELTYPE_BITMASK;
> > + }
> > + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> > + &cancelhandling,
> > + newval));
> > + }
> > +
> > + buffer->__canceltype = (cancelhandling & CANCELTYPE_BITMASK
> > + ? PTHREAD_CANCEL_ASYNCHRONOUS
> > + : PTHREAD_CANCEL_DEFERRED);
> >
> > THREAD_SETMEM (self, cleanup, buffer);
> > }
> > @@ -41,8 +56,22 @@ __libc_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer)
> >
> > THREAD_SETMEM (self, cleanup, buffer->__prev);
> >
> > - THREAD_SETMEM (self, canceltype, buffer->__canceltype);
> > - if (buffer->__canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> > - __pthread_testcancel ();
> > + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> > + if (cancelhandling & CANCELTYPE_BITMASK)
> > + {
> > + int newval;
> > + do
> > + {
> > + newval = cancelhandling | CANCELTYPE_BITMASK;
> > + }
> > + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> > + &cancelhandling, newval));
> > +
> > + if (cancel_enabled_and_canceled (cancelhandling))
> > + {
> > + self->result = PTHREAD_CANCELED;
> > + __do_cancel ();
> > + }
> > + }
> > }
> > libc_hidden_def (__libc_cleanup_pop_restore)
> > diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> > index 7524c7ce4d..c76882e279 100644
> > --- a/nptl/pthread_cancel.c
> > +++ b/nptl/pthread_cancel.c
> > @@ -42,18 +42,29 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
> >
> > struct pthread *self = THREAD_SELF;
> >
> > - int ch = atomic_load_relaxed (&self->cancelhandling);
> > - /* Cancelation not enabled, not cancelled, or already exitting. */
> > - if (self->cancelstate == PTHREAD_CANCEL_DISABLE
> > - || (ch & CANCELED_BITMASK) == 0
> > - || (ch & EXITING_BITMASK) != 0)
> > - return;
> > -
> > - /* Set the return value. */
> > - THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> > - /* Make sure asynchronous cancellation is still enabled. */
> > - if (self->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> > - __do_cancel ();
> > + int oldval = atomic_load_relaxed (&self->cancelhandling);
> > + while (1)
> > + {
> > + /* We are canceled now. When canceled by another thread this flag
> > + is already set but if the signal is directly send (internally or
> > + from another process) is has to be done here. */
> > + int newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
> > +
> > + if (oldval == newval || (oldval & EXITING_BITMASK) != 0)
> > + /* Already canceled or exiting. */
> > + break;
> > +
> > + if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> > + &oldval, newval))
> > + {
> > + self->result = PTHREAD_CANCELED;
> > +
> > + /* Make sure asynchronous cancellation is still enabled. */
> > + if ((oldval & CANCELTYPE_BITMASK) != 0)
> > + /* Run the registered destructors and terminate the thread. */
> > + __do_cancel ();
> > + }
> > + }
> > }
> >
> > int
> > @@ -92,29 +103,70 @@ __pthread_cancel (pthread_t th)
> > }
> > #endif
> >
> > - int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK);
> > - if ((oldch & CANCELED_BITMASK) != 0)
> > - return 0;
> > -
> > - if (pd == THREAD_SELF)
> > + /* 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 cannot send SIGCANCEL unless the cancellation
> > + 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 setting 'cancelhandling' and the cancelation will be acted
> > + upon on next cancellation entrypoing in the target thread.
> > +
> > + It also requires to atomically check if cancellation is enabled and
> > + asynchronous, so both cancellation state and type are tracked on
> > + 'cancelhandling'. */
> > +
> > + int result = 0;
> > + int oldval = atomic_load_relaxed (&pd->cancelhandling);
> > + int newval;
> > + do
> > {
> > - /* A single-threaded process should be able to kill itself, since there
> > - is nothing in the POSIX specification that says that it cannot. So
> > - we set multiple_threads to true so that cancellation points get
> > - executed. */
> > - THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
> > + newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
> > + if (oldval == newval)
> > + break;
> > +
> > + /* If the cancellation is handled asynchronously just send a
> > + signal. We avoid this if possible since it's more
> > + expensive. */
> > + if (cancel_enabled_and_canceled_and_async (newval))
> > + {
> > + /* Mark the cancellation as "in progress". */
> > + int newval2 = oldval | CANCELING_BITMASK;
> > + if (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling,
> > + &oldval, newval2))
> > + continue;
> > +
> > + if (pd == THREAD_SELF)
> > + /* This is not merely an optimization: An application may
> > + call pthread_cancel (pthread_self ()) without calling
> > + pthread_create, so the signal handler may not have been
> > + set up for a self-cancel. */
> > + {
> > + pd->result = PTHREAD_CANCELED;
> > + if ((newval & CANCELTYPE_BITMASK) != 0)
> > + __do_cancel ();
> > + }
> > + else
> > + /* The cancellation handler will take care of marking the
> > + thread as canceled. */
> > + result = __pthread_kill_internal (th, SIGCANCEL);
> > +
> > + break;
> > + }
> > +
> > + /* A single-threaded process should be able to kill itself, since
> > + there is nothing in the POSIX specification that says that it
> > + cannot. So we set multiple_threads to true so that cancellation
> > + points get executed. */
> > + THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
> > #ifndef TLS_MULTIPLE_THREADS_IN_TCB
> > __libc_multiple_threads = 1;
> > #endif
> > -
> > - THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
> > - if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
> > - && pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
> > - __do_cancel ();
> > - return 0;
> > }
> > + while (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling, &oldval,
> > + newval));
> >
> > - return __pthread_kill_internal (th, SIGCANCEL);
> > + return result;
> > }
> > versioned_symbol (libc, __pthread_cancel, pthread_cancel, GLIBC_2_34);
> >
> > diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> > index a8e884f341..ca3245b0af 100644
> > --- a/nptl/pthread_join_common.c
> > +++ b/nptl/pthread_join_common.c
> > @@ -57,12 +57,9 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
> > if ((pd == self
> > || (self->joinid == pd
> > && (pd->cancelhandling
> > - & (CANCELED_BITMASK | EXITING_BITMASK
> > + & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
> > | TERMINATED_BITMASK)) == 0))
> > - && !(self->cancelstate == PTHREAD_CANCEL_ENABLE
> > - && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
> > - | TERMINATED_BITMASK))
> > - == CANCELED_BITMASK))
> > + && !cancel_enabled_and_canceled (self->cancelhandling))
> > /* This is a deadlock situation. 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
> > diff --git a/nptl/pthread_setcancelstate.c b/nptl/pthread_setcancelstate.c
> > index 9905b12e4f..f8edf18fbe 100644
> > --- a/nptl/pthread_setcancelstate.c
> > +++ b/nptl/pthread_setcancelstate.c
> > @@ -30,9 +30,29 @@ __pthread_setcancelstate (int state, int *oldstate)
> >
> > self = THREAD_SELF;
> >
> > - if (oldstate != NULL)
> > - *oldstate = self->cancelstate;
> > - self->cancelstate = state;
> > + int oldval = atomic_load_relaxed (&self->cancelhandling);
> > + while (1)
> > + {
> > + int newval = (state == PTHREAD_CANCEL_DISABLE
> > + ? oldval | CANCELSTATE_BITMASK
> > + : oldval & ~CANCELSTATE_BITMASK);
> > +
> > + if (oldstate != NULL)
> > + *oldstate = ((oldval & CANCELSTATE_BITMASK)
> > + ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
> > +
> > + if (oldval == newval)
> > + break;
> > +
> > + if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> > + &oldval, newval))
> > + {
> > + if (cancel_enabled_and_canceled_and_async (newval))
> > + __do_cancel ();
> > +
> > + break;
> > + }
> > + }
> >
> > return 0;
> > }
> > diff --git a/nptl/pthread_setcanceltype.c b/nptl/pthread_setcanceltype.c
> > index 94e56466de..1307d355c1 100644
> > --- a/nptl/pthread_setcanceltype.c
> > +++ b/nptl/pthread_setcanceltype.c
> > @@ -28,11 +28,32 @@ __pthread_setcanceltype (int type, int *oldtype)
> >
> > volatile struct pthread *self = THREAD_SELF;
> >
> > - if (oldtype != NULL)
> > - *oldtype = self->canceltype;
> > - self->canceltype = type;
> > - if (type == PTHREAD_CANCEL_ASYNCHRONOUS)
> > - __pthread_testcancel ();
> > + int oldval = atomic_load_relaxed (&self->cancelhandling);
> > + while (1)
> > + {
> > + int newval = (type == PTHREAD_CANCEL_ASYNCHRONOUS
> > + ? oldval | CANCELTYPE_BITMASK
> > + : oldval & ~CANCELTYPE_BITMASK);
> > +
> > + if (oldtype != NULL)
> > + *oldtype = ((oldval & CANCELTYPE_BITMASK)
> > + ? PTHREAD_CANCEL_ASYNCHRONOUS : PTHREAD_CANCEL_DEFERRED);
> > +
> > + if (oldval == newval)
> > + break;
> > +
> > + if (atomic_compare_exchange_weak_acquire (&self->cancelhandling,
> > + &oldval, newval))
> > + {
> > + if (cancel_enabled_and_canceled_and_async (newval))
> > + {
> > + THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> > + __do_cancel ();
> > + }
> > +
> > + break;
> > + }
> > + }
> >
> > return 0;
> > }
> > diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
> > index 13123608e8..b81928c000 100644
> > --- a/nptl/pthread_testcancel.c
> > +++ b/nptl/pthread_testcancel.c
> > @@ -23,13 +23,10 @@ void
> > ___pthread_testcancel (void)
> > {
> > struct pthread *self = THREAD_SELF;
> > - int cancelhandling = THREAD_GETMEM (self, cancelhandling);
> > - if (self->cancelstate == PTHREAD_CANCEL_ENABLE
> > - && (cancelhandling & CANCELED_BITMASK)
> > - && !(cancelhandling & EXITING_BITMASK)
> > - && !(cancelhandling & TERMINATED_BITMASK))
> > + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
> > + if (cancel_enabled_and_canceled (cancelhandling))
> > {
> > - THREAD_SETMEM (self, result, PTHREAD_CANCELED);
> > + self->result = PTHREAD_CANCELED;
> > __do_cancel ();
> > }
> > }
> > diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
> > index 1294c91816..53fba774a5 100644
> > --- a/sysdeps/nptl/dl-tls_init_tp.c
> > +++ b/sysdeps/nptl/dl-tls_init_tp.c
> > @@ -128,7 +128,4 @@ __tls_init_tp (void)
> > It will be bigger than it actually is, but for unwind.c/pt-longjmp.c
> > purposes this is good enough. */
> > THREAD_SETMEM (pd, stackblock_size, (size_t) __libc_stack_end);
> > -
> > - THREAD_SETMEM (pd, cancelstate, PTHREAD_CANCEL_ENABLE);
> > - THREAD_SETMEM (pd, canceltype, PTHREAD_CANCEL_DEFERRED);
> > }
> > diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
> > index 708bd92469..601db4ff2b 100644
> > --- a/sysdeps/nptl/pthreadP.h
> > +++ b/sysdeps/nptl/pthreadP.h
> > @@ -275,7 +275,7 @@ __do_cancel (void)
> > struct pthread *self = THREAD_SELF;
> >
> > /* Make sure we get no more cancellations. */
> > - THREAD_ATOMIC_BIT_SET (self, cancelhandling, EXITING_BIT);
> > + atomic_bit_set (&self->cancelhandling, EXITING_BIT);
> >
> > __pthread_unwind ((__pthread_unwind_buf_t *)
> > THREAD_GETMEM (self, cleanup_jmp_buf));
> > diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> > index 318dfdc04a..e901c51df0 100644
> > --- a/sysdeps/pthread/Makefile
> > +++ b/sysdeps/pthread/Makefile
> > @@ -69,6 +69,7 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
> > tst-cancel12 tst-cancel13 tst-cancel14 tst-cancel15 tst-cancel16 \
> > tst-cancel18 tst-cancel19 tst-cancel20 tst-cancel21 \
> > tst-cancel22 tst-cancel23 tst-cancel26 tst-cancel27 tst-cancel28 \
> > + tst-cancel29 \
> > tst-cleanup0 tst-cleanup1 tst-cleanup2 tst-cleanup3 \
> > tst-clock1 \
> > tst-cond-except \
> > diff --git a/sysdeps/pthread/tst-cancel29.c b/sysdeps/pthread/tst-cancel29.c
> > new file mode 100644
> > index 0000000000..4f0d99e002
> > --- /dev/null
> > +++ b/sysdeps/pthread/tst-cancel29.c
> > @@ -0,0 +1,207 @@
> > +/* Check if a thread that disables cancellation and which call functions
> > + that might be interrupted by a signal do not see the internal SIGCANCEL.
> > +
> > + Copyright (C) 2022 Free Software Foundation, Inc.
> > + This file is part of the GNU C Library.
> > +
> > + The GNU C Library is free software; you can redistribute it and/or
> > + modify it under the terms of the GNU Lesser General Public
> > + License as published by the Free Software Foundation; either
> > + version 2.1 of the License, or (at your option) any later version.
> > +
> > + The GNU C Library is distributed in the hope that it will be useful,
> > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + Lesser General Public License for more details.
> > +
> > + You should have received a copy of the GNU Lesser General Public
> > + License along with the GNU C Library; if not, see
> > + <https://www.gnu.org/licenses/>. */
> > +
> > +#include <array_length.h>
> > +#include <errno.h>
> > +#include <inttypes.h>
> > +#include <poll.h>
> > +#include <support/check.h>
> > +#include <support/support.h>
> > +#include <support/temp_file.h>
> > +#include <support/xthread.h>
> > +#include <sys/socket.h>
> > +#include <signal.h>
> > +#include <stdio.h>
> > +#include <unistd.h>
> > +
> > +/* On Linux some interfaces are never restarted after being interrupted by
> > + a signal handler, regardless of the use of SA_RESTART. It means that
> > + if asynchronous cancellation is not enabled, the pthread_cancel can not
> > + set the internal SIGCANCEL otherwise the interface might see a spurious
> > + EINTR failure. */
> > +
> > +static pthread_barrier_t b;
> > +
> > +/* Cleanup handling test. */
> > +static int cl_called;
> > +static void
> > +cl (void *arg)
> > +{
> > + ++cl_called;
> > +}
> > +
> > +static void *
> > +tf_sigtimedwait (void *arg)
> > +{
> > + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> > + xpthread_barrier_wait (&b);
> > +
> > + int r;
> > + pthread_cleanup_push (cl, NULL);
> > +
> > + sigset_t mask;
> > + sigemptyset (&mask);
> > + r = sigtimedwait (&mask, NULL, &(struct timespec) { 0, 250000000 });
> > + if (r != -1)
> > + return (void*) -1;
> > + if (errno != EAGAIN)
> > + return (void*) -2;
> > +
> > + pthread_cleanup_pop (0);
> > + return NULL;
> > +}
> > +
> > +static void *
> > +tf_poll (void *arg)
> > +{
> > + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> > + xpthread_barrier_wait (&b);
> > +
> > + int r;
> > + pthread_cleanup_push (cl, NULL);
> > +
> > + r = poll (NULL, 0, 250);
> > + if (r != 0)
> > + return (void*) -1;
> > +
> > + pthread_cleanup_pop (0);
> > + return NULL;
> > +}
> > +
> > +static void *
> > +tf_ppoll (void *arg)
> > +{
> > + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> > +
> > + xpthread_barrier_wait (&b);
> > +
> > + int r;
> > + pthread_cleanup_push (cl, NULL);
> > +
> > + r = ppoll (NULL, 0, &(struct timespec) { 0, 250000000 }, NULL);
> > + if (r != 0)
> > + return (void*) -1;
> > +
> > + pthread_cleanup_pop (0);
> > + return NULL;
> > +}
> > +
> > +static void *
> > +tf_select (void *arg)
> > +{
> > + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> > + xpthread_barrier_wait (&b);
> > +
> > + int r;
> > + pthread_cleanup_push (cl, NULL);
> > +
> > + r = select (0, NULL, NULL, NULL, &(struct timeval) { 0, 250000 });
> > + if (r != 0)
> > + return (void*) -1;
> > +
> > + pthread_cleanup_pop (0);
> > + return NULL;
> > +}
> > +
> > +static void *
> > +tf_pselect (void *arg)
> > +{
> > + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> > + xpthread_barrier_wait (&b);
> > +
> > + int r;
> > + pthread_cleanup_push (cl, NULL);
> > +
> > + r = pselect (0, NULL, NULL, NULL, &(struct timespec) { 0, 250000000 }, NULL);
> > + if (r != 0)
> > + return (void*) -1;
> > +
> > + pthread_cleanup_pop (0);
> > + return NULL;
> > +}
> > +
> > +static void *
> > +tf_clock_nanosleep (void *arg)
> > +{
> > + pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, NULL);
> > + xpthread_barrier_wait (&b);
> > +
> > + int r;
> > + pthread_cleanup_push (cl, NULL);
> > +
> > + r = clock_nanosleep (CLOCK_REALTIME, 0, &(struct timespec) { 0, 250000000 },
> > + NULL);
> > + if (r != 0)
> > + return (void*) -1;
> > +
> > + pthread_cleanup_pop (0);
> > + return NULL;
> > +}
> > +
> > +struct cancel_test_t
> > +{
> > + const char *name;
> > + void * (*cf) (void *);
> > +} tests[] =
> > +{
> > + { "sigtimedwait", tf_sigtimedwait, },
> > + { "poll", tf_poll, },
> > + { "ppoll", tf_ppoll, },
> > + { "select", tf_select, },
> > + { "pselect", tf_pselect , },
> > + { "clock_nanosleep", tf_clock_nanosleep, },
> > +};
> > +
> > +static int
> > +do_test (void)
> > +{
> > + for (int i = 0; i < array_length (tests); i++)
> > + {
> > + xpthread_barrier_init (&b, NULL, 2);
> > +
> > + cl_called = 0;
> > +
> > + pthread_t th = xpthread_create (NULL, tests[i].cf, NULL);
> > +
> > + xpthread_barrier_wait (&b);
> > +
> > + struct timespec ts = { .tv_sec = 0, .tv_nsec = 100000000 };
> > + while (nanosleep (&ts, &ts) != 0)
> > + continue;
> > +
> > + xpthread_cancel (th);
> > +
> > + void *status = xpthread_join (th);
> > + if (status != NULL)
> > + printf ("test '%s' failed: %" PRIdPTR "\n", tests[i].name,
> > + (intptr_t) status);
> > + TEST_VERIFY (status == NULL);
> > +
> > + xpthread_barrier_destroy (&b);
> > +
> > + TEST_COMPARE (cl_called, 0);
> > +
> > + printf ("in-time cancel test of '%s' successful\n", tests[i].name);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +#include <support/test-driver.c>
> > --
> > 2.32.0
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)
2022-07-12 21:27 ` Noah Goldstein
2022-07-12 21:28 ` Noah Goldstein
@ 2022-07-13 12:57 ` Adhemerval Zanella Netto
1 sibling, 0 replies; 12+ messages in thread
From: Adhemerval Zanella Netto @ 2022-07-13 12:57 UTC (permalink / raw)
To: Noah Goldstein; +Cc: GNU C Library, Florian Weimer, Aurelien Jarno
On 12/07/22 18:27, Noah Goldstein wrote:
>> diff --git a/nptl/cleanup_defer.c b/nptl/cleanup_defer.c
>> index f8181a40e8..eb0bc77740 100644
>> --- a/nptl/cleanup_defer.c
>> +++ b/nptl/cleanup_defer.c
>> @@ -30,9 +30,22 @@ ___pthread_register_cancel_defer (__pthread_unwind_buf_t *buf)
>> ibuf->priv.data.prev = THREAD_GETMEM (self, cleanup_jmp_buf);
>> ibuf->priv.data.cleanup = THREAD_GETMEM (self, cleanup);
>>
>> - /* Disable asynchronous cancellation for now. */
>> - ibuf->priv.data.canceltype = THREAD_GETMEM (self, canceltype);
>> - THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
>> + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
>> + if (__glibc_unlikely (cancelhandling & CANCELTYPE_BITMASK))
>> + {
>> + int newval;
>> + do
>> + {
>> + newval = cancelhandling & ~CANCELTYPE_BITMASK;
>> + }
>> + while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
>> + &cancelhandling,
>> + newval));
>> + }
>> +
>> + ibuf->priv.data.canceltype = (cancelhandling & CANCELTYPE_BITMASK
>> + ? PTHREAD_CANCEL_ASYNCHRONOUS
>> + : PTHREAD_CANCEL_DEFERRED);
>>
>> /* Store the new cleanup handler info. */
>> THREAD_SETMEM (self, cleanup_jmp_buf, (struct pthread_unwind_buf *) buf);
>> @@ -54,9 +67,26 @@ ___pthread_unregister_cancel_restore (__pthread_unwind_buf_t *buf)
>>
>> THREAD_SETMEM (self, cleanup_jmp_buf, ibuf->priv.data.prev);
>>
>> - THREAD_SETMEM (self, canceltype, ibuf->priv.data.canceltype);
>> - if (ibuf->priv.data.canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
>> - __pthread_testcancel ();
>> + if (ibuf->priv.data.canceltype == PTHREAD_CANCEL_DEFERRED)
>> + return;
>> +
>> + int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
>> + if (cancelhandling & CANCELTYPE_BITMASK)
>
> should this be:
> if((cancelhandling & CANCELTYPE_BITMASK) == 0)
> ?
Yes, I will fix it.
^ permalink raw reply [flat|nested] 12+ messages in thread