* [PATCH 00/15] Fix various NPTL synchronization issues
@ 2021-09-30 20:00 Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 01/15] nptl: Set cancellation type and state on pthread_exit (BZ #28267) Adhemerval Zanella
` (14 more replies)
0 siblings, 15 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
This is an update of my previous set to fix some NPTL issues [1].
The main changes are:
- Rebased against master.
- Added various Florian's suggestions.
Patch 03 is the main change of this patchset, it uses a different
field instead of the pthread 'tid' to synchrnonize the internal
thread state (BZ#19951).
It allows to both move the thread setxid internal state out of
'cancelhandling' (used on setuid() call in multi-thread information),
and remove the EXITING_BIT and TERMINATED_BIT (since 'joinstate' now
track such it). This is done on patch 04 and 05.
Adhemerval Zanella (15):
nptl: Set cancellation type and state on pthread_exit (BZ #28267)
nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST (BZ
#28268)
nptl: Do not use pthread set_tid_address as state synchronization (BZ
#19951)
nptl: Move setxid flag out of cancelhandling
nptl: Replace struct thread cancelhandling field
nptl: Use exit_lock when accessing TID on pthread_getaffinity_np
nptl: Use exit_lock when accessing TID on pthread_setaffinity
nptl: Use exit_lock when accessing TID on pthread_getcpuclockid
nptl: Use exit_lock when accessing TID on pthread_setschedparam
nptl: Use exit_lock when accessing TID on pthread_getschedparam
nptl: Use exit_lock when accessing TID on pthread_getname_np
nptl: Use exit_lock when accessing TID on pthread_setname_np
nptl: Use exit_lock when accessing TID on pthread_sigqueue
nptl: Use exit_lock when accessing TID on pthread_setschedprio
nptl: Remove INVALID_TD_P
nptl/Makefile | 3 +-
nptl/Versions | 1 +
nptl/allocatestack.c | 5 +-
nptl/cancellation.c | 17 ++--
nptl/descr.h | 45 +++++-----
nptl/nptl-stack.h | 2 +-
nptl/nptl_free_tcb.c | 26 +++---
nptl/nptl_setxid.c | 55 ++++--------
nptl/pthread_cancel.c | 14 ++-
nptl/pthread_clockjoin.c | 2 +-
nptl/pthread_create.c | 109 ++++++++++++----------
nptl/pthread_detach.c | 40 ++++-----
nptl/pthread_exit.c | 4 +-
nptl/pthread_getaffinity.c | 28 +++---
nptl/pthread_getattr_np.c | 2 +-
nptl/pthread_getcpuclockid.c | 26 +++---
nptl/pthread_getname.c | 45 ++++++----
nptl/pthread_getschedparam.c | 55 ++++++------
nptl/pthread_join.c | 2 +-
nptl/pthread_join_common.c | 126 +++++++++-----------------
nptl/pthread_setaffinity.c | 25 +++---
nptl/pthread_setname.c | 38 +++++---
nptl/pthread_setschedparam.c | 51 ++++++-----
nptl/pthread_setschedprio.c | 45 +++++-----
nptl/pthread_sigqueue.c | 56 ++++++------
nptl/pthread_testcancel.c | 11 +--
nptl/pthread_timedjoin.c | 2 +-
nptl/pthread_tryjoin.c | 18 ++--
nptl/tst-cleanup5.c | 129 +++++++++++++++++++++++++++
nptl_db/structs.def | 2 +-
nptl_db/td_thr_get_info.c | 16 ++--
nptl_db/td_thr_getfpregs.c | 9 +-
nptl_db/td_thr_getgregs.c | 9 +-
nptl_db/td_thr_setfpregs.c | 9 +-
nptl_db/td_thr_setgregs.c | 9 +-
sysdeps/hppa/nptl/tcb-offsets.sym | 1 -
sysdeps/i386/nptl/tcb-offsets.sym | 1 -
sysdeps/nptl/dl-tls_init_tp.c | 4 +-
sysdeps/nptl/libc_start_call_main.h | 7 ++
sysdeps/nptl/pthreadP.h | 27 +++---
sysdeps/pthread/Makefile | 1 +
sysdeps/pthread/tst-pthread-exited.c | 101 +++++++++++++++++++++
sysdeps/pthread/tst-thrd-detach.c | 16 ++--
sysdeps/sh/nptl/tcb-offsets.sym | 1 -
sysdeps/x86_64/nptl/tcb-offsets.sym | 4 -
45 files changed, 706 insertions(+), 493 deletions(-)
create mode 100644 nptl/tst-cleanup5.c
create mode 100644 sysdeps/pthread/tst-pthread-exited.c
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 01/15] nptl: Set cancellation type and state on pthread_exit (BZ #28267)
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 02/15] nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST (BZ #28268) Adhemerval Zanella
` (13 subsequent siblings)
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
It is required by POSIX XSH 2.9.5 Thread Cancellation under the heading
Thread Cancellation Cleanup Handlers.
Checked x86_64-linux-gnu.
---
nptl/Makefile | 3 +-
nptl/cancellation.c | 7 ++-
nptl/pthread_cancel.c | 1 -
nptl/pthread_exit.c | 4 +-
nptl/pthread_testcancel.c | 1 -
nptl/tst-cleanup5.c | 129 ++++++++++++++++++++++++++++++++++++++
sysdeps/nptl/pthreadP.h | 18 ++++--
7 files changed, 152 insertions(+), 11 deletions(-)
create mode 100644 nptl/tst-cleanup5.c
diff --git a/nptl/Makefile b/nptl/Makefile
index ff4d590f11..e1e195cc15 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -306,7 +306,8 @@ tests = tst-attr2 tst-attr3 tst-default-attr \
tst-pthread-gdb-attach tst-pthread-gdb-attach-static \
tst-pthread_exit-nothreads \
tst-pthread_exit-nothreads-static \
- tst-thread-setspecific
+ tst-thread-setspecific \
+ tst-cleanup5
tests-nolibpthread = \
tst-pthread_exit-nothreads \
diff --git a/nptl/cancellation.c b/nptl/cancellation.c
index 44f8417845..7cef9487bf 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -41,7 +41,6 @@ __pthread_enable_asynccancel (void)
&& !(ch & EXITING_BITMASK)
&& !(ch & TERMINATED_BITMASK))
{
- THREAD_SETMEM (self, result, PTHREAD_CANCELED);
__do_cancel ();
}
@@ -63,3 +62,9 @@ __pthread_disable_asynccancel (int oldtype)
self->canceltype = PTHREAD_CANCEL_DEFERRED;
}
libc_hidden_def (__pthread_disable_asynccancel)
+
+void
+__do_cancel (void)
+{
+ __exit_thread (PTHREAD_CANCELED);
+}
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index a8aa3b3d15..868cf676a3 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -107,7 +107,6 @@ __pthread_cancel (pthread_t th)
__libc_multiple_threads = 1;
#endif
- THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
&& pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
__do_cancel ();
diff --git a/nptl/pthread_exit.c b/nptl/pthread_exit.c
index f54388a8e2..14a73043bf 100644
--- a/nptl/pthread_exit.c
+++ b/nptl/pthread_exit.c
@@ -31,9 +31,7 @@ __pthread_exit (void *value)
" must be installed for pthread_exit to work\n");
}
- THREAD_SETMEM (THREAD_SELF, result, value);
-
- __do_cancel ();
+ __exit_thread (value);
}
libc_hidden_def (__pthread_exit)
weak_alias (__pthread_exit, pthread_exit)
diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
index 7a7a199b6d..6c613462ab 100644
--- a/nptl/pthread_testcancel.c
+++ b/nptl/pthread_testcancel.c
@@ -29,7 +29,6 @@ ___pthread_testcancel (void)
&& !(cancelhandling & EXITING_BITMASK)
&& !(cancelhandling & TERMINATED_BITMASK))
{
- THREAD_SETMEM (self, result, PTHREAD_CANCELED);
__do_cancel ();
}
}
diff --git a/nptl/tst-cleanup5.c b/nptl/tst-cleanup5.c
new file mode 100644
index 0000000000..f1f81a40fe
--- /dev/null
+++ b/nptl/tst-cleanup5.c
@@ -0,0 +1,129 @@
+/* Check if cancellation state and type is correctly set on thread exit.
+ Copyright (C) 2021 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 <support/check.h>
+#include <support/xthread.h>
+#include <support/xunistd.h>
+
+static int pipefds[2];
+static pthread_barrier_t b;
+
+static void
+clh (void *arg)
+{
+ /* Although POSIX state setting either the cancellation state or type is
+ undefined during cleanup handler execution, both calls should be safe
+ since none has any side-effect (they should not change current state
+ neither trigger a pending cancellation). */
+
+ int state;
+ TEST_VERIFY (pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, &state) == 0);
+ TEST_COMPARE (state, PTHREAD_CANCEL_DISABLE);
+
+ int type;
+ TEST_VERIFY (pthread_setcanceltype (PTHREAD_CANCEL_DEFERRED, &type) == 0);
+ TEST_COMPARE (type, PTHREAD_CANCEL_DEFERRED);
+}
+
+/* Check if a thread with PTHREAD_CANCEL_DEFERRED cancellation on
+ pthread_cleanup_pop sets the correct state and type as pthread_exit. */
+static void *
+tf_cancel_deferred (void *arg)
+{
+ xpthread_barrier_wait (&b);
+
+ pthread_cleanup_push (clh, NULL);
+
+ char c;
+ read (pipefds[0], &c, 1);
+
+ pthread_cleanup_pop (1);
+
+ return NULL;
+}
+
+/* Check if a thread with PTHREAD_CANCEL_DEFERRED cancellation on
+ blocked read() sets the correct state and type as pthread_exit. */
+static void *
+tf_testcancel (void *arg)
+{
+ xpthread_barrier_wait (&b);
+
+ pthread_cleanup_push (clh, NULL);
+
+ char c;
+ read (pipefds[0], &c, 1);
+
+ pthread_testcancel ();
+
+ pthread_cleanup_pop (1);
+
+ return NULL;
+}
+
+#define EXIT_EXPECTED_VALUE ((void *) 42)
+
+/* Check if a thread with PTHREAD_CANCEL_DEFERRED cancellation on
+ pthread_exit() sets the correct state and type. */
+static void *
+tf_exit (void *arg)
+{
+ xpthread_barrier_wait (&b);
+
+ pthread_cleanup_push (clh, NULL);
+
+ pthread_exit (EXIT_EXPECTED_VALUE);
+
+ pthread_cleanup_pop (1);
+
+ return NULL;
+}
+
+static int
+do_test (void)
+{
+ xpipe (pipefds);
+
+ xpthread_barrier_init (&b, NULL, 2);
+ {
+ pthread_t th = xpthread_create (NULL, tf_cancel_deferred, NULL);
+ xpthread_barrier_wait (&b);
+ xpthread_cancel (th);
+ void *r = xpthread_join (th);
+ TEST_VERIFY (r == PTHREAD_CANCELED);
+ }
+
+ {
+ pthread_t th = xpthread_create (NULL, tf_testcancel, NULL);
+ xpthread_barrier_wait (&b);
+ xpthread_cancel (th);
+ void *r = xpthread_join (th);
+ TEST_VERIFY (r == PTHREAD_CANCELED);
+ }
+
+ {
+ pthread_t th = xpthread_create (NULL, tf_exit, NULL);
+ xpthread_barrier_wait (&b);
+ void *r = xpthread_join (th);
+ TEST_VERIFY (r == EXIT_EXPECTED_VALUE);
+ }
+
+ return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
index 0e39afecc6..aebddb361b 100644
--- a/sysdeps/nptl/pthreadP.h
+++ b/sysdeps/nptl/pthreadP.h
@@ -267,20 +267,30 @@ extern void __pthread_unregister_cancel (__pthread_unwind_buf_t *__buf)
__cleanup_fct_attribute;
libc_hidden_proto (__pthread_unregister_cancel)
-/* Called when a thread reacts on a cancellation request. */
-static inline void
-__attribute ((noreturn, always_inline))
-__do_cancel (void)
+static _Noreturn inline void
+__exit_thread (void *value)
{
struct pthread *self = THREAD_SELF;
/* Make sure we get no more cancellations. */
THREAD_ATOMIC_BIT_SET (self, cancelhandling, EXITING_BIT);
+ THREAD_SETMEM (self, result, value);
+
+ /* It is required by POSIX XSH 2.9.5 Thread Cancellation under the heading
+ Thread Cancellation Cleanup Handlers and also avoid further cancellation
+ wrapper to act on cancellation. */
+ THREAD_SETMEM (self, cancelstate, PTHREAD_CANCEL_DISABLE);
+ THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_DEFERRED);
+
__pthread_unwind ((__pthread_unwind_buf_t *)
THREAD_GETMEM (self, cleanup_jmp_buf));
}
+/* It is a wrapper over __exit_thread (PTHREAD_CANCELED). It is has its own
+ implementation because it might be called by arch-specific asm code. */
+_Noreturn void __do_cancel (void) attribute_hidden;
+
/* Internal prototypes. */
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 02/15] nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST (BZ #28268)
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 01/15] nptl: Set cancellation type and state on pthread_exit (BZ #28267) Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 03/15] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951) Adhemerval Zanella
` (12 subsequent siblings)
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
The robust PI mutexes are signaled by setting the LSB bit to 1, so
the code requires to take this consideration before access the
__pthread_mutex_s.
The code is also simplified: the initialization code is not really
required, PD->robust_head.list and PD->robust_list.__next are
essentially the same regardless of __PTHREAD_MUTEX_HAVE_PREV, the futex
wake is optimized to be issued only when required, and the futex shared
bit is set only when required.
Checked on a build for m68k-linux-gnu. I also checked on
x86_64-linux-gnu by removing the check for !__ASSUME_SET_ROBUST_LIST.
---
nptl/pthread_create.c | 52 +++++++++++++++++++++----------------------
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index d6ea43a754..cd8401c878 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -507,35 +507,35 @@ start_thread (void *arg)
__libc_lock_unlock (pd->exit_lock);
#ifndef __ASSUME_SET_ROBUST_LIST
- /* If this thread has any robust mutexes locked, handle them now. */
-# if __PTHREAD_MUTEX_HAVE_PREV
- void *robust = pd->robust_head.list;
-# else
- __pthread_slist_t *robust = pd->robust_list.__next;
-# endif
- /* We let the kernel do the notification if it is able to do so.
- If we have to do it here there for sure are no PI mutexes involved
- since the kernel support for them is even more recent. */
- if (!__nptl_set_robust_list_avail
- && __builtin_expect (robust != (void *) &pd->robust_head, 0))
+ /* We let the kernel do the notification if it is able to do so on the exit
+ syscall. Otherwise we need to handle before the thread terminates. */
+ void **robust;
+ while ((robust = pd->robust_head.list)
+ && robust != (void *) &pd->robust_head)
{
- do
+ /* Note: robust PI futexes are signaled by setting bit 0. */
+ void **robustp = (void **) ((uintptr_t) robust & ~1UL);
+
+ struct __pthread_mutex_s *mtx = (struct __pthread_mutex_s *)
+ ((char *) robustp - offsetof (struct __pthread_mutex_s,
+ __list.__next));
+ int shared = mtx->__kind & 128;
+
+ pd->robust_head.list_op_pending = robust;
+ pd->robust_head.list = *robustp;
+ /* Although the list will not be changed at this point, it follows the
+ expected kernel ABI. */
+ __asm ("" ::: "memory");
+
+ int lock = atomic_exchange_relaxed (&mtx->__lock, FUTEX_OWNER_DIED);
+ /* Wake any users if mutex is acquired. */
+ if (lock > 1)
{
- struct __pthread_mutex_s *this = (struct __pthread_mutex_s *)
- ((char *) robust - offsetof (struct __pthread_mutex_s,
- __list.__next));
- robust = *((void **) robust);
-
-# if __PTHREAD_MUTEX_HAVE_PREV
- this->__list.__prev = NULL;
-# endif
- this->__list.__next = NULL;
-
- atomic_or (&this->__lock, FUTEX_OWNER_DIED);
- futex_wake ((unsigned int *) &this->__lock, 1,
- /* XYZ */ FUTEX_SHARED);
+ if ((uintptr_t) robust & 1)
+ futex_unlock_pi ((unsigned int *) &mtx->__lock, shared);
+ else
+ futex_wake ((unsigned int *) &mtx->__lock, 1, shared);
}
- while (robust != (void *) &pd->robust_head);
}
#endif
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 03/15] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951)
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 01/15] nptl: Set cancellation type and state on pthread_exit (BZ #28267) Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 02/15] nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST (BZ #28268) Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 04/15] nptl: Move setxid flag out of cancelhandling Adhemerval Zanella
` (11 subsequent siblings)
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
The use after free described in BZ#19951 is due the use of two different
PD fields, 'joinid' and 'cancelhandling', to describe the thread state
to synchronize the calls of pthread_join(), pthread_detach(),
pthread_exit(), and normal thread exit.
And any state change potentially requires to check for both fields
atomically to handle partial state (such as pthread_join() with a
cancellation handler to issue a 'joinstate' field rollback).
This patch uses a different PD member with 4 possible states (JOINABLE,
DETACHED, EXITING, and EXITED) instead of pthread 'tid' field:
1. On pthread_create() the inital state is set either to JOINABLE or
DETACHED depending of the pthread attribute used.
2. On pthread_detach(), a CAS is issued on the state. If the CAS
fails it means that thread is already detached (DETACHED) or is
being terminated (EXITING). For former an EINVAL is returned,
while for latter pthread_detach() should be reponsible to join the
thread (and deallocate any internal resources).
3. In the exit phase of the wrapper function for the thread start
routine (reached either if the thread function has returned,
pthread_exit() has being called, or cancellation handled has been
acted upon) we issue a CAS on state to set to EXITING mode. If the
thread is previously on DETACHED mode the thread itself is
responsible for arranging the deallocation of any resource,
otherwise the thread needs to be joined (detached threads cannot
immediately deallocate themselves)
4. The clear_tid_field on 'clone' call is changed to set the new
'state' field on thread exit (EXITED). This state ins only
reached at thread termination.
5. The pthread_join() implementation is now simpler: the futex wait
is done directly on thread state and there is no need to reset it
in case of timeout (since the state is now set either by
pthread_detach() or by the kernel on process termination).
The race condition on pthread_detach is avoided with only one atomic
operation on PD state: once the mode is set to THREAD_STATE_DETACHED
it is up to thread itself to deallocate its memory (done on the exit
phase at pthread_create()).
Also, the INVALID_NOT_TERMINATED_TD_P is removed since a a negative
tid is not possible and the macro is not used anywhere.
This change trigger an invalid C11 thread tests: it crates a thread,
which detaches itself, and after a timeout the creating thread checks
if the join fails. The issue is once thrd_join() is called the thread
lifetime has already expired. The test is changed so the sleep is done
by the thread itself, so the creating thread will try to join a valid
thread.
Checked on x86_64-linux-gnu, i686-linux-gnu, aarch64-linux-gnu,
arm-linux-gnueabihf, and powerpc64-linux-gnu.
---
nptl/descr.h | 26 +++---
nptl/nptl-stack.h | 2 +-
nptl/pthread_cancel.c | 3 +-
nptl/pthread_clockjoin.c | 2 +-
nptl/pthread_create.c | 47 +++++++---
nptl/pthread_detach.c | 40 ++++-----
nptl/pthread_getattr_np.c | 2 +-
nptl/pthread_join.c | 2 +-
nptl/pthread_join_common.c | 130 ++++++++++------------------
nptl/pthread_timedjoin.c | 2 +-
nptl/pthread_tryjoin.c | 18 ++--
sysdeps/nptl/dl-tls_init_tp.c | 4 +-
sysdeps/nptl/libc_start_call_main.h | 7 ++
sysdeps/nptl/pthreadP.h | 3 +-
sysdeps/pthread/tst-thrd-detach.c | 16 ++--
15 files changed, 145 insertions(+), 159 deletions(-)
diff --git a/nptl/descr.h b/nptl/descr.h
index 41ee56feb2..76522b85b3 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -123,6 +123,18 @@ struct priority_protection_data
};
+/* Define a possible thread state on 'joinstate' field. The value will be
+ cleared by the kernel when the thread terminates (CLONE_CHILD_CLEARTID),
+ so THREAD_STATE_EXITED must be 0. */
+enum
+ {
+ THREAD_STATE_EXITED = 0,
+ THREAD_STATE_EXITING,
+ THREAD_STATE_JOINABLE,
+ THREAD_STATE_DETACHED,
+ };
+
+
/* Thread descriptor data structure. */
struct pthread
{
@@ -165,8 +177,7 @@ struct pthread
GL (dl_stack_user) list. */
list_t list;
- /* Thread ID - which is also a 'is this thread descriptor (and
- therefore stack) used' flag. */
+ /* Thread ID set by the kernel with CLONE_PARENT_SETTID. */
pid_t tid;
/* Ununsed. */
@@ -334,15 +345,8 @@ struct pthread
hp_timing_t cpuclock_offset_ununsed;
#endif
- /* If the thread waits to join another one the ID of the latter is
- stored here.
-
- In case a thread is detached this field contains a pointer of the
- TCB if the thread itself. This is something which cannot happen
- in normal operation. */
- struct pthread *joinid;
- /* Check whether a thread is detached. */
-#define IS_DETACHED(pd) ((pd)->joinid == (pd))
+ /* The current thread state defined by the THREAD_STATE_* enumeration. */
+ unsigned int joinstate;
/* The result of the thread function. */
void *result;
diff --git a/nptl/nptl-stack.h b/nptl/nptl-stack.h
index c0b8e48f70..f703ae449a 100644
--- a/nptl/nptl-stack.h
+++ b/nptl/nptl-stack.h
@@ -31,7 +31,7 @@ extern size_t __nptl_stack_cache_maxsize attribute_hidden;
static inline bool
__nptl_stack_in_use (struct pthread *pd)
{
- return pd->tid <= 0;
+ return atomic_load_relaxed (&pd->joinstate) == THREAD_STATE_EXITED;
}
/* Remove the stack ELEM from its list. */
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index 868cf676a3..d09947807e 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -61,7 +61,8 @@ __pthread_cancel (pthread_t th)
{
volatile struct pthread *pd = (volatile struct pthread *) th;
- if (pd->tid == 0)
+ int state = atomic_load_acquire (&pd->joinstate);
+ if (state == THREAD_STATE_EXITED || state == THREAD_STATE_EXITING)
/* The thread has already exited on the kernel side. Its outcome
(regular exit, other cancelation) has already been
determined. */
diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
index 2d01ba03a2..76598ff0fa 100644
--- a/nptl/pthread_clockjoin.c
+++ b/nptl/pthread_clockjoin.c
@@ -30,7 +30,7 @@ ___pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
return EINVAL;
return __pthread_clockjoin_ex (threadid, thread_return,
- clockid, abstime, true);
+ clockid, abstime);
}
#if __TIMESIZE == 64
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index cd8401c878..c7d53edbb8 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -286,7 +286,7 @@ static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
.flags = clone_flags,
.pidfd = (uintptr_t) &pd->tid,
.parent_tid = (uintptr_t) &pd->tid,
- .child_tid = (uintptr_t) &pd->tid,
+ .child_tid = (uintptr_t) &pd->joinstate,
.stack = (uintptr_t) stackaddr,
.stack_size = stacksize,
.tls = (uintptr_t) tp,
@@ -351,12 +351,14 @@ start_thread (void *arg)
and free any resource prior return to the pthread_create caller. */
setup_failed = pd->setup_failed == 1;
if (setup_failed)
- pd->joinid = NULL;
+ pd->joinstate = THREAD_STATE_JOINABLE;
/* And give it up right away. */
lll_unlock (pd->lock, LLL_PRIVATE);
if (setup_failed)
+ /* No need to clear the tid here, pthread_create() will join the
+ thread prior returning to caller. */
goto out;
}
@@ -481,6 +483,23 @@ start_thread (void *arg)
the breakpoint reports TD_THR_RUN state rather than TD_THR_ZOMBIE. */
atomic_bit_set (&pd->cancelhandling, EXITING_BIT);
+
+ /* CONCURRENCY NOTES:
+
+ Concurrent pthread_detach() will either set state to
+ THREAD_STATE_DETACHED or wait for the thread to terminate. The exiting
+ state set here is set so a pthread_join() wait until all the required
+ cleanup steps are done.
+
+ The 'prevstate' field will be used to determine who is responsible to
+ call __nptl_free_tcb below. */
+
+ unsigned int prevstate;
+ do
+ prevstate = atomic_load_relaxed (&pd->joinstate);
+ while (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &prevstate,
+ THREAD_STATE_EXITING));
+
if (__glibc_unlikely (atomic_decrement_and_test (&__nptl_nthreads)))
/* This was the last thread. */
exit (0);
@@ -559,17 +578,17 @@ start_thread (void *arg)
pd->setxid_futex = 0;
}
- /* If the thread is detached free the TCB. */
- if (IS_DETACHED (pd))
- /* Free the TCB. */
+ if (prevstate == THREAD_STATE_DETACHED)
__nptl_free_tcb (pd);
+ pd->tid = 0;
+
out:
/* We cannot call '_exit' here. '_exit' will terminate the process.
The 'exit' implementation in the kernel will signal when the
process is really dead since 'clone' got passed the CLONE_CHILD_CLEARTID
- flag. The 'tid' field in the TCB will be set to zero.
+ flag. The 'joinstate' field in the TCB will be set to zero.
The exit code is zero since in case all threads exit by calling
'pthread_exit' the exit status must be 0 (zero). */
@@ -664,10 +683,9 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
pd->flags = ((iattr->flags & ~(ATTR_FLAG_SCHED_SET | ATTR_FLAG_POLICY_SET))
| (self->flags & (ATTR_FLAG_SCHED_SET | ATTR_FLAG_POLICY_SET)));
- /* Initialize the field for the ID of the thread which is waiting
- for us. This is a self-reference in case the thread is created
- detached. */
- pd->joinid = iattr->flags & ATTR_FLAG_DETACHSTATE ? pd : NULL;
+ pd->joinstate = iattr->flags & ATTR_FLAG_DETACHSTATE
+ ? THREAD_STATE_DETACHED
+ : THREAD_STATE_JOINABLE;
/* The debug events are inherited from the parent. */
pd->eventbuf = self->eventbuf;
@@ -826,10 +844,11 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
/* Similar to pthread_join, but since thread creation has failed at
startup there is no need to handle all the steps. */
- pid_t tid;
- while ((tid = atomic_load_acquire (&pd->tid)) != 0)
- __futex_abstimed_wait_cancelable64 ((unsigned int *) &pd->tid,
- tid, 0, NULL, LLL_SHARED);
+ unsigned int state;
+ while ((state = atomic_load_acquire (&pd->joinstate))
+ != THREAD_STATE_EXITED)
+ __futex_abstimed_wait_cancelable64 (&pd->joinstate, state, 0,
+ NULL, LLL_SHARED);
}
/* State (c) or (d) and we have ownership of PD (see CONCURRENCY
diff --git a/nptl/pthread_detach.c b/nptl/pthread_detach.c
index 3bfc3c2ff0..c0fe9618a4 100644
--- a/nptl/pthread_detach.c
+++ b/nptl/pthread_detach.c
@@ -25,32 +25,28 @@ ___pthread_detach (pthread_t th)
{
struct pthread *pd = (struct pthread *) th;
- /* Make sure the descriptor is valid. */
- if (INVALID_NOT_TERMINATED_TD_P (pd))
- /* Not a valid thread handle. */
- return ESRCH;
+ /* CONCURRENCY NOTES:
- int result = 0;
+ Concurrent pthread_detach() will return EINVAL for the case the thread
+ is already detached (THREAD_STATE_DETACHED). POSIX states it is
+ undefined to call pthread_detach if TH refers to a non joinable thread.
- /* Mark the thread as detached. */
- if (atomic_compare_and_exchange_bool_acq (&pd->joinid, pd, NULL))
+ For the case the thread is being terminated (THREAD_STATE_EXITING),
+ pthread_detach() will responsible to clean up the stack. */
+
+ unsigned int prevstate = atomic_load_relaxed (&pd->joinstate);
+ do
{
- /* There are two possibilities here. First, the thread might
- already be detached. In this case we return EINVAL.
- Otherwise there might already be a waiter. The standard does
- not mention what happens in this case. */
- if (IS_DETACHED (pd))
- result = EINVAL;
+ if (prevstate != THREAD_STATE_JOINABLE)
+ {
+ if (prevstate == THREAD_STATE_DETACHED)
+ return EINVAL;
+ return __pthread_join (th, 0);
+ }
}
- else
- /* Check whether the thread terminated meanwhile. In this case we
- will just free the TCB. */
- if ((pd->cancelhandling & EXITING_BITMASK) != 0)
- /* Note that the code in __free_tcb makes sure each thread
- control block is freed only once. */
- __nptl_free_tcb (pd);
-
- return result;
+ while (!atomic_compare_exchange_weak_acquire (&pd->joinstate, &prevstate,
+ THREAD_STATE_DETACHED));
+ return 0;
}
versioned_symbol (libc, ___pthread_detach, pthread_detach, GLIBC_2_34);
libc_hidden_ver (___pthread_detach, __pthread_detach)
diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
index 9d4e7ecff1..73e6137bb6 100644
--- a/nptl/pthread_getattr_np.c
+++ b/nptl/pthread_getattr_np.c
@@ -52,7 +52,7 @@ __pthread_getattr_np (pthread_t thread_id, pthread_attr_t *attr)
iattr->flags = thread->flags;
/* The thread might be detached by now. */
- if (IS_DETACHED (thread))
+ if (atomic_load_acquire (&thread->joinstate) == THREAD_STATE_DETACHED)
iattr->flags |= ATTR_FLAG_DETACHSTATE;
/* This is the guardsize after adjusting it. */
diff --git a/nptl/pthread_join.c b/nptl/pthread_join.c
index d4cffccf49..37b809034a 100644
--- a/nptl/pthread_join.c
+++ b/nptl/pthread_join.c
@@ -22,7 +22,7 @@ int
___pthread_join (pthread_t threadid, void **thread_return)
{
return __pthread_clockjoin_ex (threadid, thread_return, 0 /* Ignored */,
- NULL, true);
+ NULL);
}
versioned_symbol (libc, ___pthread_join, pthread_join, GLIBC_2_34);
libc_hidden_ver (___pthread_join, __pthread_join)
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index 7303069316..978ca56af6 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -22,113 +22,73 @@
#include <time.h>
#include <futex-internal.h>
-static void
-cleanup (void *arg)
+/* Check for a possible deadlock situation where the threads are waiting for
+ each other to finish. Note that this is a "may" error. To be 100% sure we
+ catch this error we would have to lock the data structures but it is not
+ necessary. In the unlikely case that two threads are really caught in this
+ situation they will deadlock. It is the programmer's problem to figure
+ this out. */
+static inline bool
+check_for_deadlock (struct pthread *pd)
{
- /* If we already changed the waiter ID, reset it. The call cannot
- fail for any reason but the thread not having done that yet so
- there is no reason for a loop. */
struct pthread *self = THREAD_SELF;
- atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
+ return ((pd == self
+ || (atomic_load_acquire (&self->joinstate) == THREAD_STATE_DETACHED
+ && (pd->cancelhandling
+ & (CANCELED_BITMASK | EXITING_BITMASK
+ | TERMINATED_BITMASK)) == 0))
+ && !(self->cancelstate == PTHREAD_CANCEL_ENABLE
+ && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
+ | TERMINATED_BITMASK))
+ == CANCELED_BITMASK));
}
int
__pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
clockid_t clockid,
- const struct __timespec64 *abstime, bool block)
+ const struct __timespec64 *abstime)
{
struct pthread *pd = (struct pthread *) threadid;
- /* Make sure the descriptor is valid. */
- if (INVALID_NOT_TERMINATED_TD_P (pd))
- /* Not a valid thread handle. */
- return ESRCH;
-
- /* Is the thread joinable?. */
- if (IS_DETACHED (pd))
- /* We cannot wait for the thread. */
- return EINVAL;
-
- struct pthread *self = THREAD_SELF;
- int result = 0;
-
LIBC_PROBE (pthread_join, 1, threadid);
- if ((pd == self
- || (self->joinid == pd
- && (pd->cancelhandling
- & (CANCELED_BITMASK | EXITING_BITMASK
- | TERMINATED_BITMASK)) == 0))
- && !(self->cancelstate == PTHREAD_CANCEL_ENABLE
- && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
- | TERMINATED_BITMASK))
- == CANCELED_BITMASK))
- /* 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
- structures but it is not necessary. In the unlikely case that
- two threads are really caught in this situation they will
- deadlock. It is the programmer's problem to figure this
- out. */
- return EDEADLK;
-
- /* Wait for the thread to finish. If it is already locked something
- is wrong. There can only be one waiter. */
- else if (__glibc_unlikely (atomic_compare_exchange_weak_acquire (&pd->joinid,
- &self,
- NULL)))
- /* There is already somebody waiting for the thread. */
- return EINVAL;
-
- /* BLOCK waits either indefinitely or based on an absolute time. POSIX also
- states a cancellation point shall occur for pthread_join, and we use the
- same rationale for posix_timedjoin_np. Both clockwait_tid and the futex
- call use the cancellable variant. */
- if (block)
+ int result = 0;
+ unsigned int state;
+ while ((state = atomic_load_acquire (&pd->joinstate))
+ != THREAD_STATE_EXITED)
{
- /* During the wait we change to asynchronous cancellation. If we
- are cancelled the thread we are waiting for must be marked as
- un-wait-ed for again. */
- pthread_cleanup_push (cleanup, &pd->joinid);
-
- /* We need acquire MO here so that we synchronize with the
- kernel's store to 0 when the clone terminates. (see above) */
- pid_t tid;
- while ((tid = atomic_load_acquire (&pd->tid)) != 0)
- {
- /* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via
- futex wake-up when the clone terminates. The memory location
- contains the thread ID while the clone is running and is reset to
- zero by the kernel afterwards. The kernel up to version 3.16.3
- does not use the private futex operations for futex wake-up when
- the clone terminates. */
- int ret = __futex_abstimed_wait_cancelable64 (
- (unsigned int *) &pd->tid, tid, clockid, abstime, LLL_SHARED);
- if (ret == ETIMEDOUT || ret == EOVERFLOW)
- {
- result = ret;
- break;
- }
+ if (check_for_deadlock (pd))
+ return EDEADLK;
+
+ /* POSIX states calling pthread_join() on a non joinable thread is
+ undefined. However, if PD is still in the cache we can still warn
+ the caller. */
+ if (state == THREAD_STATE_DETACHED)
+ return EINVAL;
+
+ /* pthread_join() is a cancellation entrypoint and we use the same
+ rationale for pthread_timedjoin_np().
+
+ The kernel notifies a process which uses CLONE_CHILD_CLEARTID via
+ a memory zeroing and futex wake-up when the process terminates.
+ The futex operation is not private. */
+ int ret = __futex_abstimed_wait_cancelable64 (&pd->joinstate, state,
+ clockid, abstime,
+ LLL_SHARED);
+ if (ret == ETIMEDOUT || ret == EOVERFLOW)
+ {
+ result = ret;
+ break;
}
-
- pthread_cleanup_pop (0);
}
void *pd_result = pd->result;
- if (__glibc_likely (result == 0))
+ if (result == 0)
{
- /* We mark the thread as terminated and as joined. */
- pd->tid = -1;
-
- /* Store the return value if the caller is interested. */
if (thread_return != NULL)
*thread_return = pd_result;
-
- /* Free the TCB. */
__nptl_free_tcb (pd);
}
- else
- pd->joinid = NULL;
LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd_result);
diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
index a30aef5903..32374170b6 100644
--- a/nptl/pthread_timedjoin.c
+++ b/nptl/pthread_timedjoin.c
@@ -24,7 +24,7 @@ ___pthread_timedjoin_np64 (pthread_t threadid, void **thread_return,
const struct __timespec64 *abstime)
{
return __pthread_clockjoin_ex (threadid, thread_return,
- CLOCK_REALTIME, abstime, true);
+ CLOCK_REALTIME, abstime);
}
#if __TIMESIZE == 64
diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c
index e31c83de9e..60130e9d64 100644
--- a/nptl/pthread_tryjoin.c
+++ b/nptl/pthread_tryjoin.c
@@ -21,15 +21,17 @@
int
__pthread_tryjoin_np (pthread_t threadid, void **thread_return)
{
- /* Return right away if the thread hasn't terminated yet. */
- struct pthread *pd = (struct pthread *) threadid;
- if (pd->tid != 0)
- return EBUSY;
+ /* The joinable state (THREAD_STATE_JOINABLE) is straigthforward since the
+ thread hasn't finished yet and trying to join might block.
+ The exiting thread (THREAD_STATE_EXITING) also migth result in ablocking
+ call: a detached thread might change its state to exiting and a exiting
+ thread my take some time to exit (and thus let the kernel set the state
+ to THREAD_STATE_EXITED). */
- /* If pd->tid == 0 then lll_wait_tid will not block on futex
- operation. */
- return __pthread_clockjoin_ex (threadid, thread_return, 0 /* Ignored */,
- NULL, false);
+ struct pthread *pd = (struct pthread *) threadid;
+ return atomic_load_acquire (&pd->joinstate) != THREAD_STATE_EXITED
+ ? EBUSY
+ : __pthread_clockjoin_ex (threadid, thread_return, 0, NULL);
}
versioned_symbol (libc, __pthread_tryjoin_np, pthread_tryjoin_np, GLIBC_2_34);
diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
index ca494dd3a5..35d54abd92 100644
--- a/sysdeps/nptl/dl-tls_init_tp.c
+++ b/sysdeps/nptl/dl-tls_init_tp.c
@@ -62,7 +62,7 @@ __tls_init_tp (void)
/* Early initialization of the TCB. */
struct pthread *pd = THREAD_SELF;
- pd->tid = INTERNAL_SYSCALL_CALL (set_tid_address, &pd->tid);
+ pd->tid = INTERNAL_SYSCALL_CALL (set_tid_address, &pd->joinstate);
THREAD_SETMEM (pd, specific[0], &pd->specific_1stblock[0]);
THREAD_SETMEM (pd, user_stack, true);
@@ -97,4 +97,6 @@ __tls_init_tp (void)
THREAD_SETMEM (pd, cancelstate, PTHREAD_CANCEL_ENABLE);
THREAD_SETMEM (pd, canceltype, PTHREAD_CANCEL_DEFERRED);
+
+ THREAD_SETMEM (pd, joinstate, THREAD_STATE_JOINABLE);
}
diff --git a/sysdeps/nptl/libc_start_call_main.h b/sysdeps/nptl/libc_start_call_main.h
index 06d72c1e38..e7a7f7b4c4 100644
--- a/sysdeps/nptl/libc_start_call_main.h
+++ b/sysdeps/nptl/libc_start_call_main.h
@@ -18,6 +18,7 @@
#include <atomic.h>
#include <pthreadP.h>
+#include <futex-internal.h>
_Noreturn static void
__libc_start_call_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
@@ -65,6 +66,12 @@ __libc_start_call_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
/* One less thread. Decrement the counter. If it is zero we
terminate the entire process. */
result = 0;
+
+ /* For the case a thread is waiting for the main thread to finish. */
+ struct pthread *self = THREAD_SELF;
+ atomic_store_release (&self->joinstate, THREAD_STATE_EXITED);
+ futex_wake (&self->joinstate, 1, FUTEX_SHARED);
+
if (! atomic_decrement_and_test (&__nptl_nthreads))
/* Not much left to do but to exit the thread, not the process. */
while (1)
diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
index aebddb361b..8dfd55e87d 100644
--- a/sysdeps/nptl/pthreadP.h
+++ b/sysdeps/nptl/pthreadP.h
@@ -242,7 +242,6 @@ libc_hidden_proto (__pthread_current_priority)
nothing. And if the test triggers the thread descriptor is
guaranteed to be invalid. */
#define INVALID_TD_P(pd) __builtin_expect ((pd)->tid <= 0, 0)
-#define INVALID_NOT_TERMINATED_TD_P(pd) __builtin_expect ((pd)->tid < 0, 0)
extern void __pthread_unwind (__pthread_unwind_buf_t *__buf)
__cleanup_fct_attribute __attribute ((__noreturn__))
@@ -536,7 +535,7 @@ libc_hidden_proto (__pthread_setcanceltype)
extern void __pthread_testcancel (void);
libc_hidden_proto (__pthread_testcancel)
extern int __pthread_clockjoin_ex (pthread_t, void **, clockid_t,
- const struct __timespec64 *, bool)
+ const struct __timespec64 *)
attribute_hidden;
extern int __pthread_sigmask (int, const sigset_t *, sigset_t *);
libc_hidden_proto (__pthread_sigmask);
diff --git a/sysdeps/pthread/tst-thrd-detach.c b/sysdeps/pthread/tst-thrd-detach.c
index c844767748..e1906a0e10 100644
--- a/sysdeps/pthread/tst-thrd-detach.c
+++ b/sysdeps/pthread/tst-thrd-detach.c
@@ -20,14 +20,14 @@
#include <time.h>
#include <stdio.h>
#include <unistd.h>
-
+#include <limits.h>
#include <support/check.h>
static int
detach_thrd (void *arg)
{
- if (thrd_detach (thrd_current ()) != thrd_success)
- FAIL_EXIT1 ("thrd_detach failed");
+ thrd_sleep (&(struct timespec) { .tv_sec = INT_MAX }, NULL);
+
thrd_exit (thrd_success);
}
@@ -36,15 +36,11 @@ do_test (void)
{
thrd_t id;
- /* Create new thread. */
- if (thrd_create (&id, detach_thrd, NULL) != thrd_success)
- FAIL_EXIT1 ("thrd_create failed");
+ TEST_COMPARE (thrd_create (&id, detach_thrd, NULL), thrd_success);
- /* Give some time so the thread can finish. */
- thrd_sleep (&(struct timespec) {.tv_sec = 2}, NULL);
+ TEST_COMPARE (thrd_detach (id), thrd_success);
- if (thrd_join (id, NULL) == thrd_success)
- FAIL_EXIT1 ("thrd_join succeed where it should fail");
+ TEST_COMPARE (thrd_join (id, NULL), thrd_error);
return 0;
}
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 04/15] nptl: Move setxid flag out of cancelhandling
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
` (2 preceding siblings ...)
2021-09-30 20:00 ` [PATCH 03/15] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951) Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 05/15] nptl: Replace struct thread cancelhandling field Adhemerval Zanella
` (10 subsequent siblings)
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
Now that the thread state is tracked by 'joinstate' field, there is no
need to keep the setxid flag within the 'cancelhandling'. It simplifies
the atomic code to set and reset it (since there is no need to handle
the thread state concurrent update).
Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
nptl/allocatestack.c | 3 +++
nptl/descr.h | 6 ++---
nptl/nptl_setxid.c | 55 ++++++++++++++-----------------------------
nptl/pthread_create.c | 4 ++--
4 files changed, 26 insertions(+), 42 deletions(-)
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index fa8100719f..74a8bbc19c 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -99,6 +99,7 @@ get_cached_stack (size_t *sizep, void **memp)
}
/* Don't allow setxid until cloned. */
+ result->setxid_flag = 0;
result->setxid_futex = -1;
/* Dequeue the entry. */
@@ -303,6 +304,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
#endif
/* Don't allow setxid until cloned. */
+ pd->setxid_flag = 0;
pd->setxid_futex = -1;
/* Allocate the DTV for this thread. */
@@ -424,6 +426,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
#endif
/* Don't allow setxid until cloned. */
+ pd->setxid_flag = 0;
pd->setxid_futex = -1;
/* Allocate the DTV for this thread. */
diff --git a/nptl/descr.h b/nptl/descr.h
index 76522b85b3..db011af500 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -296,9 +296,6 @@ struct pthread
/* Bit set if thread terminated and TCB is freed. */
#define TERMINATED_BIT 5
#define TERMINATED_BITMASK (0x01 << TERMINATED_BIT)
- /* Bit set if thread is supposed to change XID. */
-#define SETXID_BIT 6
-#define SETXID_BITMASK (0x01 << SETXID_BIT)
/* Flags. Including those copied from the thread attribute. */
int flags;
@@ -338,6 +335,9 @@ struct pthread
/* Lock to synchronize access to the descriptor. */
int lock;
+ /* Indicate whether thread is supposed to change XID, it acts a boolean
+ variable (0 means no operation pending). */
+ int setxid_flag;
/* Lock for synchronizing setxid calls. */
unsigned int setxid_futex;
diff --git a/nptl/nptl_setxid.c b/nptl/nptl_setxid.c
index 1bd7b5f803..244536260d 100644
--- a/nptl/nptl_setxid.c
+++ b/nptl/nptl_setxid.c
@@ -75,14 +75,7 @@ __nptl_setxid_sighandler (int sig, siginfo_t *si, void *ctx)
/* Reset the SETXID flag. */
struct pthread *self = THREAD_SELF;
- int flags, newval;
- do
- {
- flags = THREAD_GETMEM (self, cancelhandling);
- newval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
- flags & ~SETXID_BITMASK, flags);
- }
- while (flags != newval);
+ atomic_store_release (&self->setxid_flag, 0);
/* And release the futex. */
self->setxid_futex = 1;
@@ -96,8 +89,6 @@ libc_hidden_def (__nptl_setxid_sighandler)
static void
setxid_mark_thread (struct xid_command *cmdp, struct pthread *t)
{
- int ch;
-
/* Wait until this thread is cloned. */
if (t->setxid_futex == -1
&& ! atomic_compare_and_exchange_bool_acq (&t->setxid_futex, -2, -1))
@@ -108,41 +99,31 @@ setxid_mark_thread (struct xid_command *cmdp, struct pthread *t)
/* Don't let the thread exit before the setxid handler runs. */
t->setxid_futex = 0;
- do
+ /* If thread is exiting right now, ignore it. */
+ if (atomic_load_acquire (&t->joinstate) == THREAD_STATE_EXITING)
{
- ch = t->cancelhandling;
-
- /* If the thread is exiting right now, ignore it. */
- if ((ch & EXITING_BITMASK) != 0)
- {
- /* Release the futex if there is no other setxid in
- progress. */
- if ((ch & SETXID_BITMASK) == 0)
- {
- t->setxid_futex = 1;
- futex_wake (&t->setxid_futex, 1, FUTEX_PRIVATE);
- }
- return;
+ /* Release the futex if there is no other setxid in progress. */
+ if (atomic_load_relaxed (&t->setxid_flag) == 0)
+ {
+ t->setxid_futex = 1;
+ futex_wake (&t->setxid_futex, 1, FUTEX_PRIVATE);
}
+ return;
+ }
+
+ /* Release the futex if there is no other setxid in progress. */
+ if (atomic_exchange_acquire (&t->setxid_flag, 1) == 0)
+ {
+ t->setxid_futex = 1;
+ futex_wake (&t->setxid_futex, 1, FUTEX_PRIVATE);
}
- while (atomic_compare_and_exchange_bool_acq (&t->cancelhandling,
- ch | SETXID_BITMASK, ch));
}
static void
setxid_unmark_thread (struct xid_command *cmdp, struct pthread *t)
{
- int ch;
-
- do
- {
- ch = t->cancelhandling;
- if ((ch & SETXID_BITMASK) == 0)
- return;
- }
- while (atomic_compare_and_exchange_bool_acq (&t->cancelhandling,
- ch & ~SETXID_BITMASK, ch));
+ atomic_store_release (&t->setxid_flag, 0);
/* Release the futex just in case. */
t->setxid_futex = 1;
@@ -153,7 +134,7 @@ setxid_unmark_thread (struct xid_command *cmdp, struct pthread *t)
static int
setxid_signal_thread (struct xid_command *cmdp, struct pthread *t)
{
- if ((t->cancelhandling & SETXID_BITMASK) == 0)
+ if (atomic_load_relaxed (&t->setxid_flag) == 0)
return 0;
int val;
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index c7d53edbb8..3659f51c46 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -562,7 +562,7 @@ start_thread (void *arg)
advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd,
pd->guardsize);
- if (__glibc_unlikely (pd->cancelhandling & SETXID_BITMASK))
+ if (__glibc_unlikely (atomic_load_relaxed (&pd->setxid_flag) == 1))
{
/* Some other thread might call any of the setXid functions and expect
us to reply. In this case wait until we did that. */
@@ -572,7 +572,7 @@ start_thread (void *arg)
condition used in the surrounding loop (cancelhandling). We need
to check and document why this is correct. */
futex_wait_simple (&pd->setxid_futex, 0, FUTEX_PRIVATE);
- while (pd->cancelhandling & SETXID_BITMASK);
+ while (atomic_load_relaxed (&pd->setxid_flag) == 1);
/* Reset the value so that the stack can be reused. */
pd->setxid_futex = 0;
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 05/15] nptl: Replace struct thread cancelhandling field
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
` (3 preceding siblings ...)
2021-09-30 20:00 ` [PATCH 04/15] nptl: Move setxid flag out of cancelhandling Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np Adhemerval Zanella
` (9 subsequent siblings)
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
Now that both thread state and setxid is being tracked by two different
fields ('joinstate' and 'setxid_flag'), there is no need to keep track
of exiting (EXITING_BIT) and terminated (TERMINATED_BIT) state in
'cancelhandling'.
The 'cancelhandling' field is renamed to 'cancel_requested' and it only
signals whether the thread has been cancelled (set by pthread_cancel()).
It allows simplify the atomic operation required to avoid CAS
operations.
There is no need to check the thread state on
__pthread_enable_asynccancel() nor on pthread_testcancel () anymore,
since cancellation is explicitlly disabled when thread start the exit
code on __do_cancel().
On __nptl_free_tcb(), the 'joinstate' now defines whether it is the
creating thread or the created thread that calls it. So there is
no concurrent call within the function and thus no need to set the
TERMINATED_BIT.
For SIGCANCEL handler, sigcancel_handler(), 'joinstate' is used instead
(pthread_cancel() might still be called concurrenty in asynchronous
mode).
Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
nptl/Versions | 1 +
nptl/allocatestack.c | 2 +-
nptl/cancellation.c | 10 ++--------
nptl/descr.h | 13 ++-----------
nptl/nptl_free_tcb.c | 26 +++++++++++---------------
nptl/pthread_cancel.c | 10 ++++------
nptl/pthread_create.c | 10 ++--------
nptl/pthread_join_common.c | 10 +++-------
nptl/pthread_testcancel.c | 10 ++--------
nptl_db/structs.def | 2 +-
nptl_db/td_thr_get_info.c | 16 +++++++---------
nptl_db/td_thr_getfpregs.c | 9 +++++----
nptl_db/td_thr_getgregs.c | 9 +++++----
nptl_db/td_thr_setfpregs.c | 9 +++++----
nptl_db/td_thr_setgregs.c | 9 +++++----
sysdeps/hppa/nptl/tcb-offsets.sym | 1 -
sysdeps/i386/nptl/tcb-offsets.sym | 1 -
sysdeps/nptl/pthreadP.h | 3 ---
sysdeps/sh/nptl/tcb-offsets.sym | 1 -
sysdeps/x86_64/nptl/tcb-offsets.sym | 4 ----
20 files changed, 56 insertions(+), 100 deletions(-)
diff --git a/nptl/Versions b/nptl/Versions
index 3221de89d1..3b23c6e248 100644
--- a/nptl/Versions
+++ b/nptl/Versions
@@ -413,6 +413,7 @@ libc {
_thread_db_pthread_eventbuf;
_thread_db_pthread_eventbuf_eventmask;
_thread_db_pthread_eventbuf_eventmask_event_bits;
+ _thread_db_pthread_joinstate;
_thread_db_pthread_key_data_data;
_thread_db_pthread_key_data_level2_data;
_thread_db_pthread_key_data_seq;
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 74a8bbc19c..2b6baf583f 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -119,7 +119,7 @@ get_cached_stack (size_t *sizep, void **memp)
*memp = result->stackblock;
/* Cancellation handling is back to the default. */
- result->cancelhandling = 0;
+ result->cancel_requested = 0;
result->cancelstate = PTHREAD_CANCEL_ENABLE;
result->canceltype = PTHREAD_CANCEL_DEFERRED;
result->cleanup = NULL;
diff --git a/nptl/cancellation.c b/nptl/cancellation.c
index 7cef9487bf..35c1b5c7e2 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -34,15 +34,9 @@ __pthread_enable_asynccancel (void)
int oldval = THREAD_GETMEM (self, canceltype);
THREAD_SETMEM (self, canceltype, PTHREAD_CANCEL_ASYNCHRONOUS);
- int ch = THREAD_GETMEM (self, cancelhandling);
-
if (self->cancelstate == PTHREAD_CANCEL_ENABLE
- && (ch & CANCELED_BITMASK)
- && !(ch & EXITING_BITMASK)
- && !(ch & TERMINATED_BITMASK))
- {
- __do_cancel ();
- }
+ && self->cancel_requested == 1)
+ __do_cancel ();
return oldval;
}
diff --git a/nptl/descr.h b/nptl/descr.h
index db011af500..6dc84ba47e 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -285,17 +285,8 @@ struct pthread
struct pthread_unwind_buf *cleanup_jmp_buf;
#define HAVE_CLEANUP_JMP_BUF
- /* Flags determining processing of cancellation. */
- int cancelhandling;
- /* Bit set if canceled. */
-#define CANCELED_BIT 3
-#define CANCELED_BITMASK (0x01 << CANCELED_BIT)
- /* Bit set if thread is exiting. */
-#define EXITING_BIT 4
-#define EXITING_BITMASK (0x01 << EXITING_BIT)
- /* Bit set if thread terminated and TCB is freed. */
-#define TERMINATED_BIT 5
-#define TERMINATED_BITMASK (0x01 << TERMINATED_BIT)
+ /* Flag to determine whether the thread is signaled to be cancelled. */
+ int cancel_requested;
/* Flags. Including those copied from the thread attribute. */
int flags;
diff --git a/nptl/nptl_free_tcb.c b/nptl/nptl_free_tcb.c
index 987a45f23c..56d5effae3 100644
--- a/nptl/nptl_free_tcb.c
+++ b/nptl/nptl_free_tcb.c
@@ -23,22 +23,18 @@
void
__nptl_free_tcb (struct pthread *pd)
{
- /* The thread is exiting now. */
- if (atomic_bit_test_set (&pd->cancelhandling, TERMINATED_BIT) == 0)
- {
- /* Free TPP data. */
- if (pd->tpp != NULL)
- {
- struct priority_protection_data *tpp = pd->tpp;
+ /* Free TPP data. */
+ if (pd->tpp != NULL)
+ {
+ struct priority_protection_data *tpp = pd->tpp;
- pd->tpp = NULL;
- free (tpp);
- }
+ pd->tpp = NULL;
+ free (tpp);
+ }
- /* Queue the stack memory block for reuse and exit the process. The
- kernel will signal via writing to the address returned by
- QUEUE-STACK when the stack is available. */
- __nptl_deallocate_stack (pd);
- }
+ /* Queue the stack memory block for reuse and exit the process. The kernel
+ will signal via writing to the address of pthread 'joinstate' member
+ (due CLONE_CHILD_CLEARTID) when the child exits. */
+ __nptl_deallocate_stack (pd);
}
libc_hidden_def (__nptl_free_tcb)
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index d09947807e..f8cdbb6708 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -42,11 +42,8 @@ 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)
+ || atomic_load_relaxed (&self->joinstate) == THREAD_STATE_EXITING)
return;
/* Set the return value. */
@@ -93,8 +90,9 @@ __pthread_cancel (pthread_t th)
}
#endif
- int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK);
- if ((oldch & CANCELED_BITMASK) != 0)
+ /* If already cancelled just return (cancellation will be acted upon in next
+ cancellation entrypoint). */
+ if (atomic_exchange_relaxed (&pd->cancel_requested, 1) == 1)
return 0;
if (pd == THREAD_SELF)
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 3659f51c46..4ad5d8de26 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -478,12 +478,6 @@ start_thread (void *arg)
}
}
- /* The thread is exiting now. Don't set this bit until after we've hit
- the event-reporting breakpoint, so that td_thr_get_info on us while at
- the breakpoint reports TD_THR_RUN state rather than TD_THR_ZOMBIE. */
- atomic_bit_set (&pd->cancelhandling, EXITING_BIT);
-
-
/* CONCURRENCY NOTES:
Concurrent pthread_detach() will either set state to
@@ -569,8 +563,8 @@ start_thread (void *arg)
do
/* XXX This differs from the typical futex_wait_simple pattern in that
the futex_wait condition (setxid_futex) is different from the
- condition used in the surrounding loop (cancelhandling). We need
- to check and document why this is correct. */
+ condition used in the surrounding loop. We need to check and
+ document why this is correct. */
futex_wait_simple (&pd->setxid_futex, 0, FUTEX_PRIVATE);
while (atomic_load_relaxed (&pd->setxid_flag) == 1);
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index 978ca56af6..3fed02d6be 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -33,14 +33,10 @@ check_for_deadlock (struct pthread *pd)
{
struct pthread *self = THREAD_SELF;
return ((pd == self
- || (atomic_load_acquire (&self->joinstate) == THREAD_STATE_DETACHED
- && (pd->cancelhandling
- & (CANCELED_BITMASK | EXITING_BITMASK
- | TERMINATED_BITMASK)) == 0))
+ || (atomic_load_acquire (&self->joinstate)
+ == THREAD_STATE_DETACHED))
&& !(self->cancelstate == PTHREAD_CANCEL_ENABLE
- && (pd->cancelhandling & (CANCELED_BITMASK | EXITING_BITMASK
- | TERMINATED_BITMASK))
- == CANCELED_BITMASK));
+ && atomic_load_relaxed (&self->cancel_requested) == 1));
}
int
diff --git a/nptl/pthread_testcancel.c b/nptl/pthread_testcancel.c
index 6c613462ab..a45b8e3176 100644
--- a/nptl/pthread_testcancel.c
+++ b/nptl/pthread_testcancel.c
@@ -23,14 +23,8 @@ 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))
- {
- __do_cancel ();
- }
+ if (self->cancelstate == PTHREAD_CANCEL_ENABLE && self->cancel_requested == 1)
+ __do_cancel ();
}
versioned_symbol (libc, ___pthread_testcancel, pthread_testcancel, GLIBC_2_34);
libc_hidden_ver (___pthread_testcancel, __pthread_testcancel)
diff --git a/nptl_db/structs.def b/nptl_db/structs.def
index 248ecf4335..65a068e9d1 100644
--- a/nptl_db/structs.def
+++ b/nptl_db/structs.def
@@ -53,7 +53,7 @@ DB_STRUCT_FIELD (pthread, list)
DB_STRUCT_FIELD (pthread, report_events)
DB_STRUCT_FIELD (pthread, tid)
DB_STRUCT_FIELD (pthread, start_routine)
-DB_STRUCT_FIELD (pthread, cancelhandling)
+DB_STRUCT_FIELD (pthread, joinstate)
DB_STRUCT_FIELD (pthread, schedpolicy)
DB_STRUCT_FIELD (pthread, schedparam_sched_priority)
DB_STRUCT_FIELD (pthread, specific)
diff --git a/nptl_db/td_thr_get_info.c b/nptl_db/td_thr_get_info.c
index 7d79bd4152..c3ab444a9f 100644
--- a/nptl_db/td_thr_get_info.c
+++ b/nptl_db/td_thr_get_info.c
@@ -27,7 +27,7 @@ td_thr_get_info (const td_thrhandle_t *th, td_thrinfo_t *infop)
{
td_err_e err;
void *copy;
- psaddr_t tls, schedpolicy, schedprio, cancelhandling, tid, report_events;
+ psaddr_t tls, schedpolicy, schedprio, joinstate, tid, report_events;
LOG ("td_thr_get_info");
@@ -36,7 +36,7 @@ td_thr_get_info (const td_thrhandle_t *th, td_thrinfo_t *infop)
/* Special case for the main thread before initialization. */
copy = NULL;
tls = 0;
- cancelhandling = 0;
+ joinstate = 0;
schedpolicy = SCHED_OTHER;
schedprio = 0;
tid = 0;
@@ -75,8 +75,8 @@ td_thr_get_info (const td_thrhandle_t *th, td_thrinfo_t *infop)
err = DB_GET_FIELD_LOCAL (tid, th->th_ta_p, copy, pthread, tid, 0);
if (err != TD_OK)
return err;
- err = DB_GET_FIELD_LOCAL (cancelhandling, th->th_ta_p, copy, pthread,
- cancelhandling, 0);
+ err = DB_GET_FIELD_LOCAL (joinstate, th->th_ta_p, copy, pthread,
+ joinstate, 0);
if (err != TD_OK)
return err;
err = DB_GET_FIELD_LOCAL (report_events, th->th_ta_p, copy, pthread,
@@ -95,13 +95,11 @@ td_thr_get_info (const td_thrhandle_t *th, td_thrinfo_t *infop)
? 0 : (uintptr_t) schedprio);
infop->ti_type = TD_THR_USER;
- if ((((int) (uintptr_t) cancelhandling) & EXITING_BITMASK) == 0)
- /* XXX For now there is no way to get more information. */
+ int js = (int) (uintptr_t) joinstate;
+ if (js == THREAD_STATE_JOINABLE || js == THREAD_STATE_DETACHED)
infop->ti_state = TD_THR_ACTIVE;
- else if ((((int) (uintptr_t) cancelhandling) & TERMINATED_BITMASK) == 0)
- infop->ti_state = TD_THR_ZOMBIE;
else
- infop->ti_state = TD_THR_UNKNOWN;
+ infop->ti_state = TD_THR_ZOMBIE;
/* Initialization which are the same in both cases. */
infop->ti_ta_p = th->th_ta_p;
diff --git a/nptl_db/td_thr_getfpregs.c b/nptl_db/td_thr_getfpregs.c
index 4dc4e392e9..1429d9fd8f 100644
--- a/nptl_db/td_thr_getfpregs.c
+++ b/nptl_db/td_thr_getfpregs.c
@@ -22,7 +22,7 @@
td_err_e
td_thr_getfpregs (const td_thrhandle_t *th, prfpregset_t *regset)
{
- psaddr_t cancelhandling, tid;
+ psaddr_t joinstate, tid;
td_err_e err;
LOG ("td_thr_getfpregs");
@@ -33,13 +33,14 @@ td_thr_getfpregs (const td_thrhandle_t *th, prfpregset_t *regset)
regset) != PS_OK ? TD_ERR : TD_OK;
/* We have to get the state and the PID for this thread. */
- err = DB_GET_FIELD (cancelhandling, th->th_ta_p, th->th_unique, pthread,
- cancelhandling, 0);
+ err = DB_GET_FIELD (joinstate, th->th_ta_p, th->th_unique, pthread,
+ joinstate, 0);
if (err != TD_OK)
return err;
/* If the thread already terminated we return all zeroes. */
- if (((int) (uintptr_t) cancelhandling) & TERMINATED_BITMASK)
+ int js = (int) (uintptr_t) joinstate;
+ if (js == THREAD_STATE_EXITING || js == THREAD_STATE_EXITED)
memset (regset, '\0', sizeof (*regset));
/* Otherwise get the register content through the callback. */
else
diff --git a/nptl_db/td_thr_getgregs.c b/nptl_db/td_thr_getgregs.c
index 2dfa4fe3c2..1acabe9dc0 100644
--- a/nptl_db/td_thr_getgregs.c
+++ b/nptl_db/td_thr_getgregs.c
@@ -22,7 +22,7 @@
td_err_e
td_thr_getgregs (const td_thrhandle_t *th, prgregset_t regset)
{
- psaddr_t cancelhandling, tid;
+ psaddr_t joinstate, tid;
td_err_e err;
LOG ("td_thr_getgregs");
@@ -33,13 +33,14 @@ td_thr_getgregs (const td_thrhandle_t *th, prgregset_t regset)
regset) != PS_OK ? TD_ERR : TD_OK;
/* We have to get the state and the PID for this thread. */
- err = DB_GET_FIELD (cancelhandling, th->th_ta_p, th->th_unique, pthread,
- cancelhandling, 0);
+ err = DB_GET_FIELD (joinstate, th->th_ta_p, th->th_unique, pthread,
+ joinstate, 0);
if (err != TD_OK)
return err;
/* If the thread already terminated we return all zeroes. */
- if (((int) (uintptr_t) cancelhandling) & TERMINATED_BITMASK)
+ int js = (int) (uintptr_t) joinstate;
+ if (js == THREAD_STATE_EXITING || js == THREAD_STATE_EXITED)
memset (regset, '\0', sizeof (*regset));
/* Otherwise get the register content through the callback. */
else
diff --git a/nptl_db/td_thr_setfpregs.c b/nptl_db/td_thr_setfpregs.c
index f4dfc8b97a..04d148e0f4 100644
--- a/nptl_db/td_thr_setfpregs.c
+++ b/nptl_db/td_thr_setfpregs.c
@@ -22,7 +22,7 @@
td_err_e
td_thr_setfpregs (const td_thrhandle_t *th, const prfpregset_t *fpregs)
{
- psaddr_t cancelhandling, tid;
+ psaddr_t joinstate, tid;
td_err_e err;
LOG ("td_thr_setfpregs");
@@ -33,13 +33,14 @@ td_thr_setfpregs (const td_thrhandle_t *th, const prfpregset_t *fpregs)
fpregs) != PS_OK ? TD_ERR : TD_OK;
/* We have to get the state and the PID for this thread. */
- err = DB_GET_FIELD (cancelhandling, th->th_ta_p, th->th_unique, pthread,
- cancelhandling, 0);
+ err = DB_GET_FIELD (joinstate, th->th_ta_p, th->th_unique, pthread,
+ joinstate, 0);
if (err != TD_OK)
return err;
/* Only set the registers if the thread hasn't yet terminated. */
- if ((((int) (uintptr_t) cancelhandling) & TERMINATED_BITMASK) == 0)
+ int js = (int) (uintptr_t) joinstate;
+ if (js != THREAD_STATE_EXITING || js != THREAD_STATE_EXITED)
{
err = DB_GET_FIELD (tid, th->th_ta_p, th->th_unique, pthread, tid, 0);
if (err != TD_OK)
diff --git a/nptl_db/td_thr_setgregs.c b/nptl_db/td_thr_setgregs.c
index 2e5c20f0ea..50dd12b8b3 100644
--- a/nptl_db/td_thr_setgregs.c
+++ b/nptl_db/td_thr_setgregs.c
@@ -22,7 +22,7 @@
td_err_e
td_thr_setgregs (const td_thrhandle_t *th, prgregset_t gregs)
{
- psaddr_t cancelhandling, tid;
+ psaddr_t joinstate, tid;
td_err_e err;
LOG ("td_thr_setgregs");
@@ -33,13 +33,14 @@ td_thr_setgregs (const td_thrhandle_t *th, prgregset_t gregs)
gregs) != PS_OK ? TD_ERR : TD_OK;
/* We have to get the state and the PID for this thread. */
- err = DB_GET_FIELD (cancelhandling, th->th_ta_p, th->th_unique, pthread,
- cancelhandling, 0);
+ err = DB_GET_FIELD (joinstate, th->th_ta_p, th->th_unique, pthread,
+ joinstate, 0);
if (err != TD_OK)
return err;
/* Only set the registers if the thread hasn't yet terminated. */
- if ((((int) (uintptr_t) cancelhandling) & TERMINATED_BITMASK) == 0)
+ int js = (int) (uintptr_t) joinstate;
+ if (js != THREAD_STATE_EXITING || js != THREAD_STATE_EXITED)
{
err = DB_GET_FIELD (tid, th->th_ta_p, th->th_unique, pthread, tid, 0);
if (err != TD_OK)
diff --git a/sysdeps/hppa/nptl/tcb-offsets.sym b/sysdeps/hppa/nptl/tcb-offsets.sym
index 6e852f35b1..8937f1ec21 100644
--- a/sysdeps/hppa/nptl/tcb-offsets.sym
+++ b/sysdeps/hppa/nptl/tcb-offsets.sym
@@ -3,7 +3,6 @@
RESULT offsetof (struct pthread, result)
TID offsetof (struct pthread, tid)
-CANCELHANDLING offsetof (struct pthread, cancelhandling)
CLEANUP_JMP_BUF offsetof (struct pthread, cleanup_jmp_buf)
MULTIPLE_THREADS_OFFSET offsetof (struct pthread, header.multiple_threads)
TLS_PRE_TCB_SIZE sizeof (struct pthread)
diff --git a/sysdeps/i386/nptl/tcb-offsets.sym b/sysdeps/i386/nptl/tcb-offsets.sym
index 2ec9e787c1..85a27dc29f 100644
--- a/sysdeps/i386/nptl/tcb-offsets.sym
+++ b/sysdeps/i386/nptl/tcb-offsets.sym
@@ -4,7 +4,6 @@
RESULT offsetof (struct pthread, result)
TID offsetof (struct pthread, tid)
-CANCELHANDLING offsetof (struct pthread, cancelhandling)
CLEANUP_JMP_BUF offsetof (struct pthread, cleanup_jmp_buf)
MULTIPLE_THREADS_OFFSET offsetof (tcbhead_t, multiple_threads)
SYSINFO_OFFSET offsetof (tcbhead_t, sysinfo)
diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
index 8dfd55e87d..6427a11cd2 100644
--- a/sysdeps/nptl/pthreadP.h
+++ b/sysdeps/nptl/pthreadP.h
@@ -271,9 +271,6 @@ __exit_thread (void *value)
{
struct pthread *self = THREAD_SELF;
- /* Make sure we get no more cancellations. */
- THREAD_ATOMIC_BIT_SET (self, cancelhandling, EXITING_BIT);
-
THREAD_SETMEM (self, result, value);
/* It is required by POSIX XSH 2.9.5 Thread Cancellation under the heading
diff --git a/sysdeps/sh/nptl/tcb-offsets.sym b/sysdeps/sh/nptl/tcb-offsets.sym
index 234207779d..60c9e40b72 100644
--- a/sysdeps/sh/nptl/tcb-offsets.sym
+++ b/sysdeps/sh/nptl/tcb-offsets.sym
@@ -4,7 +4,6 @@
RESULT offsetof (struct pthread, result)
TID offsetof (struct pthread, tid)
-CANCELHANDLING offsetof (struct pthread, cancelhandling)
CLEANUP_JMP_BUF offsetof (struct pthread, cleanup_jmp_buf)
MULTIPLE_THREADS_OFFSET offsetof (struct pthread, header.multiple_threads)
TLS_PRE_TCB_SIZE sizeof (struct pthread)
diff --git a/sysdeps/x86_64/nptl/tcb-offsets.sym b/sysdeps/x86_64/nptl/tcb-offsets.sym
index 2bbd563a6c..6cc845f7ed 100644
--- a/sysdeps/x86_64/nptl/tcb-offsets.sym
+++ b/sysdeps/x86_64/nptl/tcb-offsets.sym
@@ -4,7 +4,6 @@
RESULT offsetof (struct pthread, result)
TID offsetof (struct pthread, tid)
-CANCELHANDLING offsetof (struct pthread, cancelhandling)
CLEANUP_JMP_BUF offsetof (struct pthread, cleanup_jmp_buf)
CLEANUP offsetof (struct pthread, cleanup)
CLEANUP_PREV offsetof (struct _pthread_cleanup_buffer, __prev)
@@ -13,6 +12,3 @@ MULTIPLE_THREADS_OFFSET offsetof (tcbhead_t, multiple_threads)
POINTER_GUARD offsetof (tcbhead_t, pointer_guard)
FEATURE_1_OFFSET offsetof (tcbhead_t, feature_1)
SSP_BASE_OFFSET offsetof (tcbhead_t, ssp_base)
-
--- Not strictly offsets, but these values are also used in the TCB.
-TCB_CANCELED_BITMASK CANCELED_BITMASK
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
` (4 preceding siblings ...)
2021-09-30 20:00 ` [PATCH 05/15] nptl: Replace struct thread cancelhandling field Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:54 ` Florian Weimer
2021-09-30 20:00 ` [PATCH 07/15] nptl: Use exit_lock when accessing TID on pthread_setaffinity Adhemerval Zanella
` (8 subsequent siblings)
14 siblings, 1 reply; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
Also return ESRCH if the thread is already terminated at the time of
the call. This is slight better than returning the calling thread
affinity (current behaviour), since the thread lifetime is defined.
Checked on x86_64-linux-gnu.
---
nptl/pthread_getaffinity.c | 28 +++++++++------
sysdeps/pthread/Makefile | 1 +
sysdeps/pthread/tst-pthread-exited.c | 51 ++++++++++++++++++++++++++++
3 files changed, 69 insertions(+), 11 deletions(-)
create mode 100644 sysdeps/pthread/tst-pthread-exited.c
diff --git a/nptl/pthread_getaffinity.c b/nptl/pthread_getaffinity.c
index 3b23a4b50d..7cf239c335 100644
--- a/nptl/pthread_getaffinity.c
+++ b/nptl/pthread_getaffinity.c
@@ -15,25 +15,31 @@
License along with the GNU C Library; if not, see
<https://www.gnu.org/licenses/>. */
-#include <errno.h>
-#include <limits.h>
+#include <libc-lock.h>
#include <pthreadP.h>
-#include <string.h>
-#include <sysdep.h>
-#include <sys/param.h>
-#include <sys/types.h>
#include <shlib-compat.h>
int
__pthread_getaffinity_np (pthread_t th, size_t cpusetsize, cpu_set_t *cpuset)
{
- const struct pthread *pd = (const struct pthread *) th;
+ struct pthread *pd = (struct pthread *) th;
- int res = INTERNAL_SYSCALL_CALL (sched_getaffinity, pd->tid,
- MIN (INT_MAX, cpusetsize), cpuset);
- if (INTERNAL_SYSCALL_ERROR_P (res))
- return INTERNAL_SYSCALL_ERRNO (res);
+ /* Block all signals, as required by pd->exit_lock. */
+ sigset_t old_mask;
+ __libc_signal_block_all (&old_mask);
+ __libc_lock_lock (pd->exit_lock);
+
+ int res = pd->tid == 0
+ ? -ESRCH
+ : INTERNAL_SYSCALL_CALL (sched_getaffinity, pd->tid,
+ MIN (INT_MAX, cpusetsize), cpuset);
+
+ __libc_lock_unlock (pd->exit_lock);
+ __libc_signal_restore_set (&old_mask);
+
+ if (res < 0)
+ return -res;
/* Clean the rest of the memory the kernel didn't do. */
memset ((char *) cpuset + res, '\0', cpusetsize - res);
diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index d4bd2d4e3e..e4c32f9d65 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -123,6 +123,7 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
tst-pthread_cancel-select-loop \
tst-pthread_kill-exited \
tst-pthread_kill-exiting \
+ tst-pthread-exited
# tests
tests-time64 := \
diff --git a/sysdeps/pthread/tst-pthread-exited.c b/sysdeps/pthread/tst-pthread-exited.c
new file mode 100644
index 0000000000..c27e856dea
--- /dev/null
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -0,0 +1,51 @@
+/* Test pthread interface which should return ESRCH when issues along
+ with a terminated pthread_t.
+ Copyright (C) 2021 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 <errno.h>
+#include <signal.h>
+#include <stddef.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/xthread.h>
+
+static void *
+noop_thread (void *closure)
+{
+ return NULL;
+}
+
+static int
+do_test (void)
+{
+ pthread_t thr = xpthread_create (NULL, noop_thread, NULL);
+
+ support_wait_for_thread_exit ();
+
+ {
+ cpu_set_t cpuset;
+ int r = pthread_getaffinity_np (thr, sizeof (cpuset), &cpuset);
+ TEST_COMPARE (r, ESRCH);
+ }
+
+ xpthread_join (thr);
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 07/15] nptl: Use exit_lock when accessing TID on pthread_setaffinity
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
` (5 preceding siblings ...)
2021-09-30 20:00 ` [PATCH 06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 08/15] nptl: Use exit_lock when accessing TID on pthread_getcpuclockid Adhemerval Zanella
` (7 subsequent siblings)
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
Also return ESRCH if the thread is already terminated at the time of
the call. This is slight better than setting the calling thread
affinity (current behaviour).
Checked on x86_64-linux-gnu.
---
nptl/pthread_setaffinity.c | 25 +++++++++++++++----------
sysdeps/pthread/tst-pthread-exited.c | 7 +++++++
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/nptl/pthread_setaffinity.c b/nptl/pthread_setaffinity.c
index 2ae6ae694c..cefc48e7c5 100644
--- a/nptl/pthread_setaffinity.c
+++ b/nptl/pthread_setaffinity.c
@@ -15,10 +15,8 @@
License along with the GNU C Library; if not, see
<https://www.gnu.org/licenses/>. */
-#include <errno.h>
+#include <libc-lock.h>
#include <pthreadP.h>
-#include <sysdep.h>
-#include <sys/types.h>
#include <shlib-compat.h>
@@ -26,15 +24,22 @@ int
__pthread_setaffinity_new (pthread_t th, size_t cpusetsize,
const cpu_set_t *cpuset)
{
- const struct pthread *pd = (const struct pthread *) th;
- int res;
+ struct pthread *pd = (struct pthread *) th;
- res = INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid, cpusetsize,
- cpuset);
+ /* Block all signals, as required by pd->exit_lock. */
+ sigset_t old_mask;
+ __libc_signal_block_all (&old_mask);
+ __libc_lock_lock (pd->exit_lock);
- return (INTERNAL_SYSCALL_ERROR_P (res)
- ? INTERNAL_SYSCALL_ERRNO (res)
- : 0);
+ int res = pd->tid == 0
+ ? ESRCH
+ : -INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid, cpusetsize,
+ cpuset);
+
+ __libc_lock_unlock (pd->exit_lock);
+ __libc_signal_restore_set (&old_mask);
+
+ return res;
}
versioned_symbol (libc, __pthread_setaffinity_new,
pthread_setaffinity_np, GLIBC_2_34);
diff --git a/sysdeps/pthread/tst-pthread-exited.c b/sysdeps/pthread/tst-pthread-exited.c
index c27e856dea..567d6b9b49 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -43,6 +43,13 @@ do_test (void)
TEST_COMPARE (r, ESRCH);
}
+ {
+ cpu_set_t cpuset;
+ CPU_ZERO (&cpuset);
+ int r = pthread_setaffinity_np (thr, sizeof (cpuset), &cpuset);
+ TEST_COMPARE (r, ESRCH);
+ }
+
xpthread_join (thr);
return 0;
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 08/15] nptl: Use exit_lock when accessing TID on pthread_getcpuclockid
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
` (6 preceding siblings ...)
2021-09-30 20:00 ` [PATCH 07/15] nptl: Use exit_lock when accessing TID on pthread_setaffinity Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 09/15] nptl: Use exit_lock when accessing TID on pthread_setschedparam Adhemerval Zanella
` (6 subsequent siblings)
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
Checked on x86_64-linux-gnu.
---
nptl/pthread_getcpuclockid.c | 26 ++++++++++++++------------
sysdeps/pthread/tst-pthread-exited.c | 7 +++++++
2 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/nptl/pthread_getcpuclockid.c b/nptl/pthread_getcpuclockid.c
index 0a6656ea4c..63f1978665 100644
--- a/nptl/pthread_getcpuclockid.c
+++ b/nptl/pthread_getcpuclockid.c
@@ -16,11 +16,9 @@
License along with the GNU C Library; see the file COPYING.LIB. If
not, see <https://www.gnu.org/licenses/>. */
-#include <errno.h>
-#include <pthreadP.h>
-#include <sys/time.h>
-#include <tls.h>
+#include <libc-lock.h>
#include <kernel-posix-cpu-timers.h>
+#include <pthreadP.h>
#include <shlib-compat.h>
int
@@ -28,17 +26,21 @@ __pthread_getcpuclockid (pthread_t threadid, clockid_t *clockid)
{
struct pthread *pd = (struct pthread *) threadid;
- /* Make sure the descriptor is valid. */
- if (INVALID_TD_P (pd))
- /* Not a valid thread handle. */
- return ESRCH;
+ /* Block all signals, as required by pd->exit_lock. */
+ sigset_t old_mask;
+ __libc_signal_block_all (&old_mask);
+ __libc_lock_lock (pd->exit_lock);
- /* The clockid_t value is a simple computation from the TID. */
+ int res = 0;
+ if (pd->tid != 0)
+ *clockid = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
+ else
+ res = ESRCH;
- const clockid_t tidclock = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
+ __libc_lock_unlock (pd->exit_lock);
+ __libc_signal_restore_set (&old_mask);
- *clockid = tidclock;
- return 0;
+ return res;
}
versioned_symbol (libc, __pthread_getcpuclockid, pthread_getcpuclockid,
GLIBC_2_34);
diff --git a/sysdeps/pthread/tst-pthread-exited.c b/sysdeps/pthread/tst-pthread-exited.c
index 567d6b9b49..d33e6b28fe 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -23,6 +23,7 @@
#include <support/check.h>
#include <support/support.h>
#include <support/xthread.h>
+#include <time.h>
static void *
noop_thread (void *closure)
@@ -50,6 +51,12 @@ do_test (void)
TEST_COMPARE (r, ESRCH);
}
+ {
+ clockid_t clkid;
+ int r = pthread_getcpuclockid (thr, &clkid);
+ TEST_COMPARE (r, ESRCH);
+ }
+
xpthread_join (thr);
return 0;
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 09/15] nptl: Use exit_lock when accessing TID on pthread_setschedparam
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
` (7 preceding siblings ...)
2021-09-30 20:00 ` [PATCH 08/15] nptl: Use exit_lock when accessing TID on pthread_getcpuclockid Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 10/15] nptl: Use exit_lock when accessing TID on pthread_getschedparam Adhemerval Zanella
` (5 subsequent siblings)
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
The sched_getparam call is also replaced with a INTERNAL_SYSCALL_CALL
to avoid clobbering errno.
Checked on x86_64-linux-gnu.
---
nptl/pthread_setschedparam.c | 51 +++++++++++++++-------------
sysdeps/pthread/tst-pthread-exited.c | 6 ++++
2 files changed, 34 insertions(+), 23 deletions(-)
diff --git a/nptl/pthread_setschedparam.c b/nptl/pthread_setschedparam.c
index f38bd788af..d6027824db 100644
--- a/nptl/pthread_setschedparam.c
+++ b/nptl/pthread_setschedparam.c
@@ -15,26 +15,15 @@
License along with the GNU C Library; if not, see
<https://www.gnu.org/licenses/>. */
-#include <errno.h>
-#include <sched.h>
-#include <string.h>
-#include "pthreadP.h"
-#include <lowlevellock.h>
+#include <libc-lock.h>
+#include <kernel-posix-cpu-timers.h>
+#include <pthreadP.h>
-int
-__pthread_setschedparam (pthread_t threadid, int policy,
- const struct sched_param *param)
+static int
+setschedparam (struct pthread *pd, int policy,
+ const struct sched_param *param)
{
- struct pthread *pd = (struct pthread *) threadid;
-
- /* Make sure the descriptor is valid. */
- if (INVALID_TD_P (pd))
- /* Not a valid thread handle. */
- return ESRCH;
-
- int result = 0;
-
/* See CREATE THREAD NOTES in nptl/pthread_create.c. */
lll_lock (pd->lock, LLL_PRIVATE);
@@ -43,8 +32,7 @@ __pthread_setschedparam (pthread_t threadid, int policy,
/* If the thread should have higher priority because of some
PTHREAD_PRIO_PROTECT mutexes it holds, adjust the priority. */
- if (__builtin_expect (pd->tpp != NULL, 0)
- && pd->tpp->priomax > param->sched_priority)
+ if (pd->tpp != NULL && pd->tpp->priomax > param->sched_priority)
{
p = *param;
p.sched_priority = pd->tpp->priomax;
@@ -52,10 +40,8 @@ __pthread_setschedparam (pthread_t threadid, int policy,
}
/* Try to set the scheduler information. */
- if (__builtin_expect (__sched_setscheduler (pd->tid, policy,
- param) == -1, 0))
- result = errno;
- else
+ int r = INTERNAL_SYSCALL_CALL (sched_setscheduler, pd->tid, policy, param);
+ if (r == 0)
{
/* We succeeded changing the kernel information. Reflect this
change in the thread descriptor. */
@@ -66,6 +52,25 @@ __pthread_setschedparam (pthread_t threadid, int policy,
lll_unlock (pd->lock, LLL_PRIVATE);
+ return -r;
+}
+
+int
+__pthread_setschedparam (pthread_t threadid, int policy,
+ const struct sched_param *param)
+{
+ struct pthread *pd = (struct pthread *) threadid;
+
+ /* Block all signals, as required by pd->exit_lock. */
+ sigset_t old_mask;
+ __libc_signal_block_all (&old_mask);
+ __libc_lock_lock (pd->exit_lock);
+
+ int result = pd->tid != 0 ? setschedparam (pd, policy, param) : ESRCH;
+
+ __libc_lock_unlock (pd->exit_lock);
+ __libc_signal_restore_set (&old_mask);
+
return result;
}
strong_alias (__pthread_setschedparam, pthread_setschedparam)
diff --git a/sysdeps/pthread/tst-pthread-exited.c b/sysdeps/pthread/tst-pthread-exited.c
index d33e6b28fe..208546a546 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -57,6 +57,12 @@ do_test (void)
TEST_COMPARE (r, ESRCH);
}
+ {
+ struct sched_param sch = { 0 };
+ int r = pthread_setschedparam (thr, SCHED_FIFO, &sch);
+ TEST_COMPARE (r, ESRCH);
+ }
+
xpthread_join (thr);
return 0;
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 10/15] nptl: Use exit_lock when accessing TID on pthread_getschedparam
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
` (8 preceding siblings ...)
2021-09-30 20:00 ` [PATCH 09/15] nptl: Use exit_lock when accessing TID on pthread_setschedparam Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 11/15] nptl: Use exit_lock when accessing TID on pthread_getname_np Adhemerval Zanella
` (4 subsequent siblings)
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
The sched_getparam call is also replaced with a INTERNAL_SYSCALL_CALL
to avoid clobbering errno.
Checked on x86_64-linux-gnu.
---
nptl/pthread_getschedparam.c | 55 +++++++++++++++-------------
sysdeps/pthread/tst-pthread-exited.c | 7 ++++
2 files changed, 37 insertions(+), 25 deletions(-)
diff --git a/nptl/pthread_getschedparam.c b/nptl/pthread_getschedparam.c
index ef4022b8ac..09d07c8a17 100644
--- a/nptl/pthread_getschedparam.c
+++ b/nptl/pthread_getschedparam.c
@@ -15,24 +15,14 @@
License along with the GNU C Library; if not, see
<https://www.gnu.org/licenses/>. */
-#include <errno.h>
-#include <string.h>
-#include "pthreadP.h"
-#include <lowlevellock.h>
+#include <libc-lock.h>
+#include <kernel-posix-cpu-timers.h>
+#include <pthreadP.h>
-
-int
-__pthread_getschedparam (pthread_t threadid, int *policy,
- struct sched_param *param)
+static int
+getschedparam (struct pthread *pd, int *policy, struct sched_param *param)
{
- struct pthread *pd = (struct pthread *) threadid;
-
- /* Make sure the descriptor is valid. */
- if (INVALID_TD_P (pd))
- /* Not a valid thread handle. */
- return ESRCH;
-
- int result = 0;
+ int r = 0;
/* See CREATE THREAD NOTES in nptl/pthread_create.c. */
lll_lock (pd->lock, LLL_PRIVATE);
@@ -44,22 +34,18 @@ __pthread_getschedparam (pthread_t threadid, int *policy,
not yet been retrieved do it now. */
if ((pd->flags & ATTR_FLAG_SCHED_SET) == 0)
{
- if (__sched_getparam (pd->tid, &pd->schedparam) != 0)
- result = 1;
- else
+ r = INTERNAL_SYSCALL_CALL (sched_getparam, pd->tid, &pd->schedparam);
+ if (r == 0)
pd->flags |= ATTR_FLAG_SCHED_SET;
}
-
if ((pd->flags & ATTR_FLAG_POLICY_SET) == 0)
{
- pd->schedpolicy = __sched_getscheduler (pd->tid);
- if (pd->schedpolicy == -1)
- result = 1;
- else
+ r = pd->schedpolicy = INTERNAL_SYSCALL_CALL (sched_getscheduler, pd->tid);
+ if (r >= 0)
pd->flags |= ATTR_FLAG_POLICY_SET;
}
- if (result == 0)
+ if (r >= 0)
{
*policy = pd->schedpolicy;
memcpy (param, &pd->schedparam, sizeof (struct sched_param));
@@ -67,6 +53,25 @@ __pthread_getschedparam (pthread_t threadid, int *policy,
lll_unlock (pd->lock, LLL_PRIVATE);
+ return -r;
+}
+
+int
+__pthread_getschedparam (pthread_t threadid, int *policy,
+ struct sched_param *param)
+{
+ struct pthread *pd = (struct pthread *) threadid;
+
+ /* Block all signals, as required by pd->exit_lock. */
+ sigset_t old_mask;
+ __libc_signal_block_all (&old_mask);
+ __libc_lock_lock (pd->exit_lock);
+
+ int result = pd->tid != 0 ? getschedparam (pd, policy, param) : ESRCH;
+
+ __libc_lock_unlock (pd->exit_lock);
+ __libc_signal_restore_set (&old_mask);
+
return result;
}
libc_hidden_def (__pthread_getschedparam)
diff --git a/sysdeps/pthread/tst-pthread-exited.c b/sysdeps/pthread/tst-pthread-exited.c
index 208546a546..25015d4f74 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -63,6 +63,13 @@ do_test (void)
TEST_COMPARE (r, ESRCH);
}
+ {
+ struct sched_param sch;
+ int policy;
+ int r = pthread_getschedparam (thr, &policy, &sch);
+ TEST_COMPARE (r, ESRCH);
+ }
+
xpthread_join (thr);
return 0;
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 11/15] nptl: Use exit_lock when accessing TID on pthread_getname_np
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
` (9 preceding siblings ...)
2021-09-30 20:00 ` [PATCH 10/15] nptl: Use exit_lock when accessing TID on pthread_getschedparam Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 12/15] nptl: Use exit_lock when accessing TID on pthread_setname_np Adhemerval Zanella
` (3 subsequent siblings)
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
Checked on x86_64-linux-gnu.
---
nptl/pthread_getname.c | 45 +++++++++++++++++-----------
sysdeps/pthread/tst-pthread-exited.c | 6 ++++
2 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/nptl/pthread_getname.c b/nptl/pthread_getname.c
index 8ac814366a..26011b55c9 100644
--- a/nptl/pthread_getname.c
+++ b/nptl/pthread_getname.c
@@ -18,10 +18,12 @@
#include <errno.h>
#include <fcntl.h>
+#include <libc-lock.h>
#include <pthreadP.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
+#include <intprops.h>
#include <sys/prctl.h>
#include <not-cancel.h>
#include <shlib-compat.h>
@@ -29,7 +31,7 @@
int
__pthread_getname_np (pthread_t th, char *buf, size_t len)
{
- const struct pthread *pd = (const struct pthread *) th;
+ struct pthread *pd = (struct pthread *) th;
/* Unfortunately the kernel headers do not export the TASK_COMM_LEN
macro. So we have to define it here. */
@@ -40,29 +42,38 @@ __pthread_getname_np (pthread_t th, char *buf, size_t len)
if (pd == THREAD_SELF)
return __prctl (PR_GET_NAME, buf) ? errno : 0;
-#define FMT "/proc/self/task/%u/comm"
- char fname[sizeof (FMT) + 8];
- sprintf (fname, FMT, (unsigned int) pd->tid);
+ /* Block all signals, as required by pd->exit_lock. */
+ sigset_t old_mask;
+ __libc_signal_block_all (&old_mask);
+ __libc_lock_lock (pd->exit_lock);
- int fd = __open64_nocancel (fname, O_RDONLY);
- if (fd == -1)
- return errno;
+ char fname[sizeof ("/proc/self/task//comm" ) + INT_BUFSIZE_BOUND (pid_t)];
+ __snprintf (fname, sizeof (fname), "/proc/self/task/%d/comm", pd->tid);
int res = 0;
- ssize_t n = TEMP_FAILURE_RETRY (__read_nocancel (fd, buf, len));
- if (n < 0)
- res = errno;
- else
+ int fd = __open64_nocancel (fname, O_RDONLY);
+ if (fd != -1)
{
- if (buf[n - 1] == '\n')
- buf[n - 1] = '\0';
- else if (n == len)
- res = ERANGE;
+ ssize_t n = TEMP_FAILURE_RETRY (__read_nocancel (fd, buf, len));
+ if (n > 0)
+ {
+ if (buf[n - 1] == '\n')
+ buf[n - 1] = '\0';
+ else if (n == len)
+ res = ERANGE;
+ else
+ buf[n] = '\0';
+ }
else
- buf[n] = '\0';
+ res = errno;
+
+ __close_nocancel_nostatus (fd);
}
+ else
+ res = errno == ENOENT ? ESRCH : errno;
- __close_nocancel_nostatus (fd);
+ __libc_lock_unlock (pd->exit_lock);
+ __libc_signal_restore_set (&old_mask);
return res;
}
diff --git a/sysdeps/pthread/tst-pthread-exited.c b/sysdeps/pthread/tst-pthread-exited.c
index 25015d4f74..6638afbf88 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -70,6 +70,12 @@ do_test (void)
TEST_COMPARE (r, ESRCH);
}
+ {
+ char thread_name[32];
+ int r = pthread_getname_np (thr, thread_name, 32);
+ TEST_COMPARE (r, ESRCH);
+ }
+
xpthread_join (thr);
return 0;
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 12/15] nptl: Use exit_lock when accessing TID on pthread_setname_np
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
` (10 preceding siblings ...)
2021-09-30 20:00 ` [PATCH 11/15] nptl: Use exit_lock when accessing TID on pthread_getname_np Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 13/15] nptl: Use exit_lock when accessing TID on pthread_sigqueue Adhemerval Zanella
` (2 subsequent siblings)
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
Checked on x86_64-linux-gnu.
---
nptl/pthread_setname.c | 38 ++++++++++++++++++----------
sysdeps/pthread/tst-pthread-exited.c | 6 +++++
2 files changed, 30 insertions(+), 14 deletions(-)
diff --git a/nptl/pthread_setname.c b/nptl/pthread_setname.c
index 6d2d8a1723..8aa31616d2 100644
--- a/nptl/pthread_setname.c
+++ b/nptl/pthread_setname.c
@@ -18,19 +18,20 @@
#include <errno.h>
#include <fcntl.h>
+#include <libc-lock.h>
#include <pthreadP.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
#include <sys/prctl.h>
-
+#include <intprops.h>
#include <not-cancel.h>
int
__pthread_setname_np (pthread_t th, const char *name)
{
- const struct pthread *pd = (const struct pthread *) th;
+ struct pthread *pd = (struct pthread *) th;
/* Unfortunately the kernel headers do not export the TASK_COMM_LEN
macro. So we have to define it here. */
@@ -42,22 +43,31 @@ __pthread_setname_np (pthread_t th, const char *name)
if (pd == THREAD_SELF)
return __prctl (PR_SET_NAME, name) ? errno : 0;
-#define FMT "/proc/self/task/%u/comm"
- char fname[sizeof (FMT) + 8];
- sprintf (fname, FMT, (unsigned int) pd->tid);
+ /* Block all signals, as required by pd->exit_lock. */
+ sigset_t old_mask;
+ __libc_signal_block_all (&old_mask);
+ __libc_lock_lock (pd->exit_lock);
- int fd = __open64_nocancel (fname, O_RDWR);
- if (fd == -1)
- return errno;
+ char fname[sizeof ("/proc/self/task//comm" ) + INT_BUFSIZE_BOUND (pid_t)];
+ __snprintf (fname, sizeof (fname), "/proc/self/task/%d/comm", pd->tid);
int res = 0;
- ssize_t n = TEMP_FAILURE_RETRY (__write_nocancel (fd, name, name_len));
- if (n < 0)
- res = errno;
- else if (n != name_len)
- res = EIO;
+ int fd = __open64_nocancel (fname, O_RDWR);
+ if (fd != -1)
+ {
+ ssize_t n = TEMP_FAILURE_RETRY (__write_nocancel (fd, name, name_len));
+ if (n < 0)
+ res = errno;
+ else if (n != name_len)
+ res = EIO;
+
+ __close_nocancel_nostatus (fd);
+ }
+ else
+ res = errno == ENOENT ? ESRCH : errno;
- __close_nocancel_nostatus (fd);
+ __libc_lock_unlock (pd->exit_lock);
+ __libc_signal_restore_set (&old_mask);
return res;
}
diff --git a/sysdeps/pthread/tst-pthread-exited.c b/sysdeps/pthread/tst-pthread-exited.c
index 6638afbf88..eaaf00a9b4 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -76,6 +76,12 @@ do_test (void)
TEST_COMPARE (r, ESRCH);
}
+ {
+ char thread_name[] = "test";
+ int r = pthread_setname_np (thr, thread_name);
+ TEST_COMPARE (r, ESRCH);
+ }
+
xpthread_join (thr);
return 0;
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 13/15] nptl: Use exit_lock when accessing TID on pthread_sigqueue
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
` (11 preceding siblings ...)
2021-09-30 20:00 ` [PATCH 12/15] nptl: Use exit_lock when accessing TID on pthread_setname_np Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 14/15] nptl: Use exit_lock when accessing TID on pthread_setschedprio Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 15/15] nptl: Remove INVALID_TD_P Adhemerval Zanella
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
Checked on x86_64-linux-gnu.
---
nptl/pthread_sigqueue.c | 56 ++++++++++++++--------------
sysdeps/pthread/tst-pthread-exited.c | 6 +++
2 files changed, 34 insertions(+), 28 deletions(-)
diff --git a/nptl/pthread_sigqueue.c b/nptl/pthread_sigqueue.c
index edc18bf4c0..c4a1e0a04d 100644
--- a/nptl/pthread_sigqueue.c
+++ b/nptl/pthread_sigqueue.c
@@ -16,6 +16,7 @@
<https://www.gnu.org/licenses/>. */
#include <errno.h>
+#include <libc-lock.h>
#include <signal.h>
#include <string.h>
#include <unistd.h>
@@ -27,41 +28,40 @@
int
__pthread_sigqueue (pthread_t threadid, int signo, const union sigval value)
{
-#ifdef __NR_rt_tgsigqueueinfo
- struct pthread *pd = (struct pthread *) threadid;
-
- /* Force load of pd->tid into local variable or register. Otherwise
- if a thread exits between ESRCH test and tgkill, we might return
- EINVAL, because pd->tid would be cleared by the kernel. */
- pid_t tid = atomic_forced_read (pd->tid);
- if (__glibc_unlikely (tid <= 0))
- /* Not a valid thread handle. */
- return ESRCH;
-
/* Disallow sending the signal we use for cancellation, timers,
for the setxid implementation. */
if (signo == SIGCANCEL || signo == SIGTIMER || signo == SIGSETXID)
return EINVAL;
- pid_t pid = getpid ();
+ struct pthread *pd = (struct pthread *) threadid;
- /* Set up the siginfo_t structure. */
- siginfo_t info;
- memset (&info, '\0', sizeof (siginfo_t));
- info.si_signo = signo;
- info.si_code = SI_QUEUE;
- info.si_pid = pid;
- info.si_uid = __getuid ();
- info.si_value = value;
+ /* Block all signals, as required by pd->exit_lock. */
+ sigset_t old_mask;
+ __libc_signal_block_all (&old_mask);
+ __libc_lock_lock (pd->exit_lock);
- /* We have a special syscall to do the work. */
- int val = INTERNAL_SYSCALL_CALL (rt_tgsigqueueinfo, pid, tid, signo,
- &info);
- return (INTERNAL_SYSCALL_ERROR_P (val)
- ? INTERNAL_SYSCALL_ERRNO (val) : 0);
-#else
- return ENOSYS;
-#endif
+ int res;
+ if (pd->tid != 0)
+ {
+ pid_t pid = getpid ();
+
+ siginfo_t info = { 0 };
+ info.si_signo = signo;
+ info.si_code = SI_QUEUE;
+ info.si_pid = pid;
+ info.si_uid = __getuid ();
+ info.si_value = value;
+
+ res = -INTERNAL_SYSCALL_CALL (rt_tgsigqueueinfo, pid, pd->tid, signo,
+ &info);
+ }
+ else
+ res = ESRCH;
+
+ __libc_lock_unlock (pd->exit_lock);
+ __libc_signal_restore_set (&old_mask);
+
+ return res;
}
versioned_symbol (libc, __pthread_sigqueue, pthread_sigqueue, GLIBC_2_34);
diff --git a/sysdeps/pthread/tst-pthread-exited.c b/sysdeps/pthread/tst-pthread-exited.c
index eaaf00a9b4..bfb3870cb6 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -82,6 +82,12 @@ do_test (void)
TEST_COMPARE (r, ESRCH);
}
+ {
+ union sigval value = { 0 };
+ int r = pthread_sigqueue (thr, SIGUSR1, value);
+ TEST_COMPARE (r, ESRCH);
+ }
+
xpthread_join (thr);
return 0;
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 14/15] nptl: Use exit_lock when accessing TID on pthread_setschedprio
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
` (12 preceding siblings ...)
2021-09-30 20:00 ` [PATCH 13/15] nptl: Use exit_lock when accessing TID on pthread_sigqueue Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 15/15] nptl: Remove INVALID_TD_P Adhemerval Zanella
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
Checked on x86_64-linux-gnu
---
nptl/pthread_setschedprio.c | 45 +++++++++++++++-------------
sysdeps/pthread/tst-pthread-exited.c | 5 ++++
2 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/nptl/pthread_setschedprio.c b/nptl/pthread_setschedprio.c
index c095e346c8..65cf8912ca 100644
--- a/nptl/pthread_setschedprio.c
+++ b/nptl/pthread_setschedprio.c
@@ -15,25 +15,13 @@
License along with the GNU C Library; if not, see
<https://www.gnu.org/licenses/>. */
-#include <errno.h>
-#include <sched.h>
-#include <string.h>
-#include <sched.h>
-#include "pthreadP.h"
-#include <lowlevellock.h>
+#include <libc-lock.h>
+#include <pthreadP.h>
#include <shlib-compat.h>
-int
-__pthread_setschedprio (pthread_t threadid, int prio)
+static int
+setschedprio (struct pthread *pd, int prio)
{
- struct pthread *pd = (struct pthread *) threadid;
-
- /* Make sure the descriptor is valid. */
- if (INVALID_TD_P (pd))
- /* Not a valid thread handle. */
- return ESRCH;
-
- int result = 0;
struct sched_param param;
param.sched_priority = prio;
@@ -42,13 +30,12 @@ __pthread_setschedprio (pthread_t threadid, int prio)
/* If the thread should have higher priority because of some
PTHREAD_PRIO_PROTECT mutexes it holds, adjust the priority. */
- if (__builtin_expect (pd->tpp != NULL, 0) && pd->tpp->priomax > prio)
+ if (pd->tpp != NULL && pd->tpp->priomax > prio)
param.sched_priority = pd->tpp->priomax;
/* Try to set the scheduler information. */
- if (__glibc_unlikely (__sched_setparam (pd->tid, ¶m) == -1))
- result = errno;
- else
+ int r = INTERNAL_SYSCALL_CALL (sched_setparam, pd->tid, ¶m);
+ if (r == 0)
{
/* We succeeded changing the kernel information. Reflect this
change in the thread descriptor. */
@@ -59,6 +46,24 @@ __pthread_setschedprio (pthread_t threadid, int prio)
lll_unlock (pd->lock, LLL_PRIVATE);
+ return -r;
+}
+
+int
+__pthread_setschedprio (pthread_t threadid, int prio)
+{
+ struct pthread *pd = (struct pthread *) threadid;
+
+ /* Block all signals, as required by pd->exit_lock. */
+ sigset_t old_mask;
+ __libc_signal_block_all (&old_mask);
+ __libc_lock_lock (pd->exit_lock);
+
+ int result = pd->tid != 0 ? setschedprio (pd, prio) : ESRCH;
+
+ __libc_lock_unlock (pd->exit_lock);
+ __libc_signal_restore_set (&old_mask);
+
return result;
}
versioned_symbol (libc, __pthread_setschedprio, pthread_setschedprio,
diff --git a/sysdeps/pthread/tst-pthread-exited.c b/sysdeps/pthread/tst-pthread-exited.c
index bfb3870cb6..03cf2e21a1 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -88,6 +88,11 @@ do_test (void)
TEST_COMPARE (r, ESRCH);
}
+ {
+ int r = pthread_setschedprio (thr, 0);
+ TEST_COMPARE (r, ESRCH);
+ }
+
xpthread_join (thr);
return 0;
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 15/15] nptl: Remove INVALID_TD_P
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
` (13 preceding siblings ...)
2021-09-30 20:00 ` [PATCH 14/15] nptl: Use exit_lock when accessing TID on pthread_setschedprio Adhemerval Zanella
@ 2021-09-30 20:00 ` Adhemerval Zanella
14 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-09-30 20:00 UTC (permalink / raw)
To: libc-alpha
It is not used anymore.
---
sysdeps/nptl/pthreadP.h | 5 -----
1 file changed, 5 deletions(-)
diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
index 6427a11cd2..2c053428d1 100644
--- a/sysdeps/nptl/pthreadP.h
+++ b/sysdeps/nptl/pthreadP.h
@@ -238,11 +238,6 @@ libc_hidden_proto (__pthread_tpp_change_priority)
extern int __pthread_current_priority (void);
libc_hidden_proto (__pthread_current_priority)
-/* This will not catch all invalid descriptors but is better than
- nothing. And if the test triggers the thread descriptor is
- guaranteed to be invalid. */
-#define INVALID_TD_P(pd) __builtin_expect ((pd)->tid <= 0, 0)
-
extern void __pthread_unwind (__pthread_unwind_buf_t *__buf)
__cleanup_fct_attribute __attribute ((__noreturn__))
#if !defined SHARED && !IS_IN (libpthread)
--
2.30.2
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np
2021-09-30 20:00 ` [PATCH 06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np Adhemerval Zanella
@ 2021-09-30 20:54 ` Florian Weimer
2021-10-04 12:45 ` Cyril Hrubis
0 siblings, 1 reply; 23+ messages in thread
From: Florian Weimer @ 2021-09-30 20:54 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella via Libc-alpha:
> Also return ESRCH if the thread is already terminated at the time of
> the call. This is slight better than returning the calling thread
> affinity (current behaviour), since the thread lifetime is defined.
I still think this should produce an empty affinity set instead of ESRCH
because POSIX means something different for ESRCH (thread ID invalid,
something that we cannot detect in the glibc implementation).
Thanks,
Florian
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np
2021-09-30 20:54 ` Florian Weimer
@ 2021-10-04 12:45 ` Cyril Hrubis
2021-10-04 19:27 ` Florian Weimer
0 siblings, 1 reply; 23+ messages in thread
From: Cyril Hrubis @ 2021-10-04 12:45 UTC (permalink / raw)
To: Florian Weimer; +Cc: Adhemerval Zanella via Libc-alpha
Hi!
> > Also return ESRCH if the thread is already terminated at the time of
> > the call. This is slight better than returning the calling thread
> > affinity (current behaviour), since the thread lifetime is defined.
>
> I still think this should produce an empty affinity set instead of ESRCH
> because POSIX means something different for ESRCH (thread ID invalid,
> something that we cannot detect in the glibc implementation).
The RATIONALE for most pthread_{get,set}sched*() functions has:
If an implementation detects use of a thread ID after the end of its
lifetime, it is recommended that the function should fail and report an
[ESRCH] error.
See for example:
https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np
2021-10-04 12:45 ` Cyril Hrubis
@ 2021-10-04 19:27 ` Florian Weimer
2021-10-04 19:53 ` Adhemerval Zanella
0 siblings, 1 reply; 23+ messages in thread
From: Florian Weimer @ 2021-10-04 19:27 UTC (permalink / raw)
To: Cyril Hrubis; +Cc: Florian Weimer, Adhemerval Zanella via Libc-alpha
* Cyril Hrubis:
> Hi!
>> > Also return ESRCH if the thread is already terminated at the time of
>> > the call. This is slight better than returning the calling thread
>> > affinity (current behaviour), since the thread lifetime is defined.
>>
>> I still think this should produce an empty affinity set instead of ESRCH
>> because POSIX means something different for ESRCH (thread ID invalid,
>> something that we cannot detect in the glibc implementation).
>
> The RATIONALE for most pthread_{get,set}sched*() functions has:
>
> If an implementation detects use of a thread ID after the end of its
> lifetime, it is recommended that the function should fail and report an
> [ESRCH] error.
>
> See for example:
>
> https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/
Exactly. If the thread has exited, but is still joinable, the thread
ID lifetime has not ended, so we should not return ESRCH.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np
2021-10-04 19:27 ` Florian Weimer
@ 2021-10-04 19:53 ` Adhemerval Zanella
2021-10-05 7:35 ` Cyril Hrubis
2021-10-10 14:37 ` Florian Weimer
0 siblings, 2 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-10-04 19:53 UTC (permalink / raw)
To: Florian Weimer, Cyril Hrubis
Cc: Florian Weimer, Adhemerval Zanella via Libc-alpha
On 04/10/2021 16:27, Florian Weimer wrote:
> * Cyril Hrubis:
>
>> Hi!
>>>> Also return ESRCH if the thread is already terminated at the time of
>>>> the call. This is slight better than returning the calling thread
>>>> affinity (current behaviour), since the thread lifetime is defined.
>>>
>>> I still think this should produce an empty affinity set instead of ESRCH
>>> because POSIX means something different for ESRCH (thread ID invalid,
>>> something that we cannot detect in the glibc implementation).
>>
>> The RATIONALE for most pthread_{get,set}sched*() functions has:
>>
>> If an implementation detects use of a thread ID after the end of its
>> lifetime, it is recommended that the function should fail and report an
>> [ESRCH] error.
>>
>> See for example:
>>
>> https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/
>
> Exactly. If the thread has exited, but is still joinable, the thread
> ID lifetime has not ended, so we should not return ESRCH.
>
But in this case the information can not be queried from the kernel,
any information that glibc returns will not make sense (even an
empty affinity mask). We can try to cache the pthread_setaffinity_np()
value, but I really want to avoid such complexity and the extra space
required in struct pthread.
I think this is QoI to indicate the caller such information can not
be obtained, instead of returning an bogus value.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np
2021-10-04 19:53 ` Adhemerval Zanella
@ 2021-10-05 7:35 ` Cyril Hrubis
2021-10-05 13:47 ` Adhemerval Zanella
2021-10-10 14:37 ` Florian Weimer
1 sibling, 1 reply; 23+ messages in thread
From: Cyril Hrubis @ 2021-10-05 7:35 UTC (permalink / raw)
To: Adhemerval Zanella
Cc: Florian Weimer, Florian Weimer, Adhemerval Zanella via Libc-alpha
Hi!
> >> The RATIONALE for most pthread_{get,set}sched*() functions has:
> >>
> >> If an implementation detects use of a thread ID after the end of its
> >> lifetime, it is recommended that the function should fail and report an
> >> [ESRCH] error.
> >>
> >> See for example:
> >>
> >> https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/
> >
> > Exactly. If the thread has exited, but is still joinable, the thread
> > ID lifetime has not ended, so we should not return ESRCH.
> >
>
> But in this case the information can not be queried from the kernel,
> any information that glibc returns will not make sense (even an
> empty affinity mask). We can try to cache the pthread_setaffinity_np()
> value, but I really want to avoid such complexity and the extra space
> required in struct pthread.
>
> I think this is QoI to indicate the caller such information can not
> be obtained, instead of returning an bogus value.
I guess that if we want to return an error it should be something
different from ESRCH in order not to confuse, however there does not
seem to be a good match for this condition as far as I can tell.
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np
2021-10-05 7:35 ` Cyril Hrubis
@ 2021-10-05 13:47 ` Adhemerval Zanella
0 siblings, 0 replies; 23+ messages in thread
From: Adhemerval Zanella @ 2021-10-05 13:47 UTC (permalink / raw)
To: Cyril Hrubis
Cc: Florian Weimer, Florian Weimer, Adhemerval Zanella via Libc-alpha
On 05/10/2021 04:35, Cyril Hrubis wrote:
> Hi!
>>>> The RATIONALE for most pthread_{get,set}sched*() functions has:
>>>>
>>>> If an implementation detects use of a thread ID after the end of its
>>>> lifetime, it is recommended that the function should fail and report an
>>>> [ESRCH] error.
>>>>
>>>> See for example:
>>>>
>>>> https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/
>>>
>>> Exactly. If the thread has exited, but is still joinable, the thread
>>> ID lifetime has not ended, so we should not return ESRCH.
>>>
>>
>> But in this case the information can not be queried from the kernel,
>> any information that glibc returns will not make sense (even an
>> empty affinity mask). We can try to cache the pthread_setaffinity_np()
>> value, but I really want to avoid such complexity and the extra space
>> required in struct pthread.
>>
>> I think this is QoI to indicate the caller such information can not
>> be obtained, instead of returning an bogus value.
>
> I guess that if we want to return an error it should be something
> different from ESRCH in order not to confuse, however there does not
> seem to be a good match for this condition as far as I can tell.
>
Maybe return EINVAL then, but returning an failure would still tie
its semantic to check for pthread_t validity. So the question is
whether would be better to no tie the pthread_setaffinity_np() (or
any other interface that requires the pthread tid valid value)
to check for pthread lifetime and return a bogus value (with possible
side-effects) or return an error to make it clear to caller.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np
2021-10-04 19:53 ` Adhemerval Zanella
2021-10-05 7:35 ` Cyril Hrubis
@ 2021-10-10 14:37 ` Florian Weimer
1 sibling, 0 replies; 23+ messages in thread
From: Florian Weimer @ 2021-10-10 14:37 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: Cyril Hrubis, Adhemerval Zanella via Libc-alpha
* Adhemerval Zanella:
> On 04/10/2021 16:27, Florian Weimer wrote:
>> * Cyril Hrubis:
>>
>>> Hi!
>>>>> Also return ESRCH if the thread is already terminated at the time of
>>>>> the call. This is slight better than returning the calling thread
>>>>> affinity (current behaviour), since the thread lifetime is defined.
>>>>
>>>> I still think this should produce an empty affinity set instead of ESRCH
>>>> because POSIX means something different for ESRCH (thread ID invalid,
>>>> something that we cannot detect in the glibc implementation).
>>>
>>> The RATIONALE for most pthread_{get,set}sched*() functions has:
>>>
>>> If an implementation detects use of a thread ID after the end of its
>>> lifetime, it is recommended that the function should fail and report an
>>> [ESRCH] error.
>>>
>>> See for example:
>>>
>>> https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/
>>
>> Exactly. If the thread has exited, but is still joinable, the thread
>> ID lifetime has not ended, so we should not return ESRCH.
>>
>
> But in this case the information can not be queried from the kernel,
> any information that glibc returns will not make sense (even an
> empty affinity mask). We can try to cache the pthread_setaffinity_np()
> value, but I really want to avoid such complexity and the extra space
> required in struct pthread.
Nah, I agree that caching this information in user space does not make
sense at this time.
I don't see much of a problem with an empty affinity mask.
> I think this is QoI to indicate the caller such information can not
> be obtained, instead of returning an bogus value.
Sure, but ESRCH seems to be the wrong error code, and some language in
POSIX strongly suggests that future POSIX versions are going to be
more strict about the cases in which it can be used. I do not want
applications to check for an error code that is encountered in most
cases only after undefined behavior has occurred.
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2021-10-10 14:37 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-30 20:00 [PATCH 00/15] Fix various NPTL synchronization issues Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 01/15] nptl: Set cancellation type and state on pthread_exit (BZ #28267) Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 02/15] nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST (BZ #28268) Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 03/15] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951) Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 04/15] nptl: Move setxid flag out of cancelhandling Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 05/15] nptl: Replace struct thread cancelhandling field Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np Adhemerval Zanella
2021-09-30 20:54 ` Florian Weimer
2021-10-04 12:45 ` Cyril Hrubis
2021-10-04 19:27 ` Florian Weimer
2021-10-04 19:53 ` Adhemerval Zanella
2021-10-05 7:35 ` Cyril Hrubis
2021-10-05 13:47 ` Adhemerval Zanella
2021-10-10 14:37 ` Florian Weimer
2021-09-30 20:00 ` [PATCH 07/15] nptl: Use exit_lock when accessing TID on pthread_setaffinity Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 08/15] nptl: Use exit_lock when accessing TID on pthread_getcpuclockid Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 09/15] nptl: Use exit_lock when accessing TID on pthread_setschedparam Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 10/15] nptl: Use exit_lock when accessing TID on pthread_getschedparam Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 11/15] nptl: Use exit_lock when accessing TID on pthread_getname_np Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 12/15] nptl: Use exit_lock when accessing TID on pthread_setname_np Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 13/15] nptl: Use exit_lock when accessing TID on pthread_sigqueue Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 14/15] nptl: Use exit_lock when accessing TID on pthread_setschedprio Adhemerval Zanella
2021-09-30 20:00 ` [PATCH 15/15] nptl: Remove INVALID_TD_P Adhemerval Zanella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).