* [PATCH v3 01/12] nptl: Set cancellation type and state on pthread_exit (BZ #28267)
2022-05-31 17:52 [PATCH v3 00/12] Fix various NPTL synchronization issues Adhemerval Zanella
@ 2022-05-31 17:52 ` Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 02/12] nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST (BZ #28268) Adhemerval Zanella
` (10 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2022-05-31 17:52 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 | 31 +++++++--
nptl/pthread_cancel.c | 1 -
nptl/pthread_exit.c | 4 +-
nptl/pthread_testcancel.c | 5 +-
nptl/tst-cleanup5.c | 129 ++++++++++++++++++++++++++++++++++++++
sysdeps/nptl/pthreadP.h | 14 ++---
7 files changed, 164 insertions(+), 23 deletions(-)
create mode 100644 nptl/tst-cleanup5.c
diff --git a/nptl/Makefile b/nptl/Makefile
index b585663974..fa551798d0 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -312,7 +312,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 f4b08902b2..fef2e9a614 100644
--- a/nptl/cancellation.c
+++ b/nptl/cancellation.c
@@ -43,10 +43,7 @@ __pthread_enable_asynccancel (void)
&oldval, newval))
{
if (cancel_enabled_and_canceled_and_async (newval))
- {
- self->result = PTHREAD_CANCELED;
- __do_cancel ();
- }
+ __do_cancel ();
break;
}
@@ -89,3 +86,29 @@ __pthread_disable_asynccancel (int oldtype)
}
}
libc_hidden_def (__pthread_disable_asynccancel)
+
+void
+__exit_thread (void *value)
+{
+ struct pthread *self = THREAD_SELF;
+
+ THREAD_SETMEM (self, result, value);
+
+ int oldval = atomic_load_relaxed (&self->cancelhandling);
+ int newval;
+ do
+ {
+ /* 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. */
+ newval = oldval | CANCELSTATE_BITMASK | EXITING_BITMASK;
+ newval = newval & ~CANCELTYPE_BITMASK;
+ if (oldval == newval)
+ break;
+ }
+ while (!atomic_compare_exchange_weak_acquire (&self->cancelhandling,
+ &oldval, newval));
+
+ __pthread_unwind ((__pthread_unwind_buf_t *)
+ THREAD_GETMEM (self, cleanup_jmp_buf));
+}
diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index e67b2df5cc..c4e571ec21 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -143,7 +143,6 @@ __pthread_cancel (pthread_t th)
pthread_create, so the signal handler may not have been
set up for a self-cancel. */
{
- pd->result = PTHREAD_CANCELED;
if ((newval & CANCELTYPE_BITMASK) != 0)
__do_cancel ();
}
diff --git a/nptl/pthread_exit.c b/nptl/pthread_exit.c
index 3e20c03b52..8ff8a4fd25 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 b81928c000..c1fd1e0e3a 100644
--- a/nptl/pthread_testcancel.c
+++ b/nptl/pthread_testcancel.c
@@ -25,10 +25,7 @@ ___pthread_testcancel (void)
struct pthread *self = THREAD_SELF;
int cancelhandling = atomic_load_relaxed (&self->cancelhandling);
if (cancel_enabled_and_canceled (cancelhandling))
- {
- self->result = PTHREAD_CANCELED;
- __do_cancel ();
- }
+ __do_cancel ();
}
versioned_symbol (libc, ___pthread_testcancel, pthread_testcancel, GLIBC_2_34);
libc_hidden_ver (___pthread_testcancel, __pthread_testcancel)
diff --git a/nptl/tst-cleanup5.c b/nptl/tst-cleanup5.c
new file mode 100644
index 0000000000..726b651c46
--- /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) 2022 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <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 39af275c25..e6e7430dfe 100644
--- a/sysdeps/nptl/pthreadP.h
+++ b/sysdeps/nptl/pthreadP.h
@@ -269,20 +269,14 @@ extern void __pthread_unregister_cancel (__pthread_unwind_buf_t *__buf)
libc_hidden_proto (__pthread_unregister_cancel)
/* Called when a thread reacts on a cancellation request. */
-static inline void
-__attribute ((noreturn, always_inline))
+extern _Noreturn void __exit_thread (void *value) attribute_hidden;
+
+static _Noreturn __always_inline void
__do_cancel (void)
{
- struct pthread *self = THREAD_SELF;
-
- /* Make sure we get no more cancellations. */
- atomic_bit_set (&self->cancelhandling, EXITING_BIT);
-
- __pthread_unwind ((__pthread_unwind_buf_t *)
- THREAD_GETMEM (self, cleanup_jmp_buf));
+ __exit_thread (PTHREAD_CANCELED);
}
-
/* Internal prototypes. */
/* Deallocate a thread's stack after optionally making sure the thread
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 02/12] nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST (BZ #28268)
2022-05-31 17:52 [PATCH v3 00/12] Fix various NPTL synchronization issues Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 01/12] nptl: Set cancellation type and state on pthread_exit (BZ #28267) Adhemerval Zanella
@ 2022-05-31 17:52 ` Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 03/12] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951) Adhemerval Zanella
` (9 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2022-05-31 17:52 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 e7a099acb7..a6200cf9e5 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -515,35 +515,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.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 03/12] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951)
2022-05-31 17:52 [PATCH v3 00/12] Fix various NPTL synchronization issues Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 01/12] nptl: Set cancellation type and state on pthread_exit (BZ #28267) Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 02/12] nptl: Handle robust PI mutexes for !__ASSUME_SET_ROBUST_LIST (BZ #28268) Adhemerval Zanella
@ 2022-05-31 17:52 ` Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 04/12] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np Adhemerval Zanella
` (8 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2022-05-31 17:52 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', used too describe the thread
state and to synchronize the calls of pthread_join, pthread_detach,
pthread_exit, and normal thread exit.
Any state change potentially requires to check for both field
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 is not defined. The test is changed so the sleep is done
by the thread itself, so the creating thread will try to join a valid
thread identificator.
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 | 44 +++++++---
nptl/pthread_detach.c | 40 ++++-----
nptl/pthread_getattr_np.c | 2 +-
nptl/pthread_join.c | 2 +-
nptl/pthread_join_common.c | 124 ++++++++++------------------
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, 142 insertions(+), 153 deletions(-)
diff --git a/nptl/descr.h b/nptl/descr.h
index bb46b5958e..d22eb75dd3 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -126,6 +126,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
{
@@ -168,8 +180,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. */
@@ -346,15 +357,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 5a90752e37..a62d09498a 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 c4e571ec21..1cdc9ef079 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -72,7 +72,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 c299551423..d306d13d3d 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 a6200cf9e5..89976901d6 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -287,7 +287,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,
@@ -352,12 +352,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;
}
@@ -489,6 +491,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);
@@ -567,17 +586,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.
rseq TLS is still registered at this point. Rely on implicit
unregistration performed by the kernel on thread teardown. This is not a
@@ -689,7 +708,9 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
/* 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;
@@ -848,10 +869,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 b83ec314fd..ad0ca129e9 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 9c5b73b452..ab01c9a6b0 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 491de1dbd2..f98d571687 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 ca3245b0af..9637f2ffd5 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -22,110 +22,70 @@
#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
+ & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
+ | TERMINATED_BITMASK)) == 0))
+ && !cancel_enabled_and_canceled (self->cancelhandling));
}
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
- & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
- | TERMINATED_BITMASK)) == 0))
- && !cancel_enabled_and_canceled (self->cancelhandling))
- /* This is a deadlock situation. The threads are waiting for each
- other to finish. Note that this is a "may" error. To be 100%
- sure we catch this error we would have to lock the data
- 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 96fd54dc84..dbe848bac3 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 1ceb1c4e8f..b45addec82 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 a blocking
+ 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 53fba774a5..75afe4bad4 100644
--- a/sysdeps/nptl/dl-tls_init_tp.c
+++ b/sysdeps/nptl/dl-tls_init_tp.c
@@ -72,7 +72,7 @@ __tls_init_tp (void)
list_add (&pd->list, &GL (dl_stack_user));
/* Early initialization of the TCB. */
- 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);
@@ -128,4 +128,6 @@ __tls_init_tp (void)
It will be bigger than it actually is, but for unwind.c/pt-longjmp.c
purposes this is good enough. */
THREAD_SETMEM (pd, stackblock_size, (size_t) __libc_stack_end);
+
+ THREAD_SETMEM (pd, joinstate, THREAD_STATE_JOINABLE);
}
diff --git a/sysdeps/nptl/libc_start_call_main.h b/sysdeps/nptl/libc_start_call_main.h
index a9e85f2b09..5fcaedbfac 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 e6e7430dfe..96d2015902 100644
--- a/sysdeps/nptl/pthreadP.h
+++ b/sysdeps/nptl/pthreadP.h
@@ -243,7 +243,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__))
@@ -521,7 +520,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 d4800d7ee4..89f02c64a4 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.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 04/12] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np
2022-05-31 17:52 [PATCH v3 00/12] Fix various NPTL synchronization issues Adhemerval Zanella
` (2 preceding siblings ...)
2022-05-31 17:52 ` [PATCH v3 03/12] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951) Adhemerval Zanella
@ 2022-05-31 17:52 ` Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 05/12] nptl: Use exit_lock when accessing TID on pthread_getcpuclockid Adhemerval Zanella
` (7 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2022-05-31 17:52 UTC (permalink / raw)
To: libc-alpha
Also return EINVAL 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 | 30 ++++++++++------
sysdeps/pthread/Makefile | 1 +
sysdeps/pthread/tst-pthread-exited.c | 51 ++++++++++++++++++++++++++++
3 files changed, 71 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 c91fb37343..79f346326d 100644
--- a/nptl/pthread_getaffinity.c
+++ b/nptl/pthread_getaffinity.c
@@ -15,25 +15,33 @@
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= 0;
+ if (pd->tid > 0)
+ res = INTERNAL_SYSCALL_CALL (sched_getaffinity, pd->tid,
+ MIN (INT_MAX, cpusetsize), cpuset);
+ else
+ res = -EINVAL;
+
+ __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 8cebe7a784..6a1debaf30 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -126,6 +126,7 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
tst-pthread-raise-blocked-self \
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..6fca37d7c2
--- /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, EINVAL);
+ }
+
+ xpthread_join (thr);
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 05/12] nptl: Use exit_lock when accessing TID on pthread_getcpuclockid
2022-05-31 17:52 [PATCH v3 00/12] Fix various NPTL synchronization issues Adhemerval Zanella
` (3 preceding siblings ...)
2022-05-31 17:52 ` [PATCH v3 04/12] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np Adhemerval Zanella
@ 2022-05-31 17:52 ` Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 06/12] nptl: Use exit_lock when accessing TID on pthread_setschedparam Adhemerval Zanella
` (6 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2022-05-31 17:52 UTC (permalink / raw)
To: libc-alpha
Also return EINVAL if the thread is already terminated at the time
of the call.
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 344bd6560e..9cae35f2df 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 )
+ *clockid = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
+ else
+ res = EINVAL;
- 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 6fca37d7c2..d350369563 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)
@@ -43,6 +44,12 @@ do_test (void)
TEST_COMPARE (r, EINVAL);
}
+ {
+ clockid_t clkid;
+ int r = pthread_getcpuclockid (thr, &clkid);
+ TEST_COMPARE (r, EINVAL);
+ }
+
xpthread_join (thr);
return 0;
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 06/12] nptl: Use exit_lock when accessing TID on pthread_setschedparam
2022-05-31 17:52 [PATCH v3 00/12] Fix various NPTL synchronization issues Adhemerval Zanella
` (4 preceding siblings ...)
2022-05-31 17:52 ` [PATCH v3 05/12] nptl: Use exit_lock when accessing TID on pthread_getcpuclockid Adhemerval Zanella
@ 2022-05-31 17:52 ` Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 07/12] nptl: Use exit_lock when accessing TID on pthread_getschedparam Adhemerval Zanella
` (5 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2022-05-31 17:52 UTC (permalink / raw)
To: libc-alpha
Also return EINVAL if the thread is already terminated at the time
of the call.
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 5568ff140c..58511fcf94 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) : EINVAL;
+
+ __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 d350369563..94c5bc2bea 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -50,6 +50,12 @@ do_test (void)
TEST_COMPARE (r, EINVAL);
}
+ {
+ struct sched_param sch = { 0 };
+ int r = pthread_setschedparam (thr, SCHED_FIFO, &sch);
+ TEST_COMPARE (r, EINVAL);
+ }
+
xpthread_join (thr);
return 0;
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 07/12] nptl: Use exit_lock when accessing TID on pthread_getschedparam
2022-05-31 17:52 [PATCH v3 00/12] Fix various NPTL synchronization issues Adhemerval Zanella
` (5 preceding siblings ...)
2022-05-31 17:52 ` [PATCH v3 06/12] nptl: Use exit_lock when accessing TID on pthread_setschedparam Adhemerval Zanella
@ 2022-05-31 17:52 ` Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 08/12] nptl: Use exit_lock when accessing TID on pthread_getname_np Adhemerval Zanella
` (4 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2022-05-31 17:52 UTC (permalink / raw)
To: libc-alpha
The sched_getparam call is also replaced with a INTERNAL_SYSCALL_CALL
to avoid clobbering errno.
Also return EINVAL if the thread is already terminated at the time
of the call.
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 11a34e4af7..bd0264f0fa 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) : EINVAL;
+
+ __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 94c5bc2bea..9fdbdfddc7 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -56,6 +56,13 @@ do_test (void)
TEST_COMPARE (r, EINVAL);
}
+ {
+ struct sched_param sch;
+ int policy;
+ int r = pthread_getschedparam (thr, &policy, &sch);
+ TEST_COMPARE (r, EINVAL);
+ }
+
xpthread_join (thr);
return 0;
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 08/12] nptl: Use exit_lock when accessing TID on pthread_getname_np
2022-05-31 17:52 [PATCH v3 00/12] Fix various NPTL synchronization issues Adhemerval Zanella
` (6 preceding siblings ...)
2022-05-31 17:52 ` [PATCH v3 07/12] nptl: Use exit_lock when accessing TID on pthread_getschedparam Adhemerval Zanella
@ 2022-05-31 17:52 ` Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 09/12] nptl: Use exit_lock when accessing TID on pthread_setname_np Adhemerval Zanella
` (3 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2022-05-31 17:52 UTC (permalink / raw)
To: libc-alpha
Also return EINVAL if the thread is already terminated at the time
of the call.
Checked on x86_64-linux-gnu.
---
nptl/pthread_getname.c | 65 +++++++++++++++++-----------
sysdeps/pthread/tst-pthread-exited.c | 6 +++
2 files changed, 45 insertions(+), 26 deletions(-)
diff --git a/nptl/pthread_getname.c b/nptl/pthread_getname.c
index ebec06e23f..a0c22ba561 100644
--- a/nptl/pthread_getname.c
+++ b/nptl/pthread_getname.c
@@ -16,43 +16,27 @@
License along with the GNU C Library; see the file COPYING.LIB. If
not, see <https://www.gnu.org/licenses/>. */
-#include <errno.h>
-#include <fcntl.h>
+#include <intprops.h>
+#include <libc-lock.h>
+#include <not-cancel.h>
#include <pthreadP.h>
+#include <shlib-compat.h>
#include <stdio.h>
-#include <string.h>
-#include <unistd.h>
#include <sys/prctl.h>
-#include <not-cancel.h>
-#include <shlib-compat.h>
-int
-__pthread_getname_np (pthread_t th, char *buf, size_t len)
+static int
+getname (pid_t tid, char *buf, size_t len)
{
- const struct pthread *pd = (const struct pthread *) th;
-
- /* Unfortunately the kernel headers do not export the TASK_COMM_LEN
- macro. So we have to define it here. */
-#define TASK_COMM_LEN 16
- if (len < TASK_COMM_LEN)
- return ERANGE;
-
- 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);
+ char fname[sizeof ("/proc/self/task//comm") + INT_BUFSIZE_BOUND (pid_t)];
+ __snprintf (fname, sizeof (fname), "/proc/self/task/%d/comm", tid);
int fd = __open64_nocancel (fname, O_RDONLY);
if (fd == -1)
- return errno;
+ return EINVAL;
int res = 0;
ssize_t n = TEMP_FAILURE_RETRY (__read_nocancel (fd, buf, len));
- if (n < 0)
- res = errno;
- else
+ if (n > 0)
{
if (buf[n - 1] == '\n')
buf[n - 1] = '\0';
@@ -61,11 +45,40 @@ __pthread_getname_np (pthread_t th, char *buf, size_t len)
else
buf[n] = '\0';
}
+ else
+ res = errno == ENOENT ? EINVAL : errno;;
__close_nocancel_nostatus (fd);
return res;
}
+
+int
+__pthread_getname_np (pthread_t th, char *buf, size_t len)
+{
+ 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. */
+#define TASK_COMM_LEN 16
+ if (len < TASK_COMM_LEN)
+ return ERANGE;
+
+ if (pd == THREAD_SELF)
+ return __prctl (PR_GET_NAME, buf) ? errno : 0;
+
+ /* 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 ? getname (pd->tid, buf, len) : EINVAL;
+
+ __libc_lock_unlock (pd->exit_lock);
+ __libc_signal_restore_set (&old_mask);
+
+ return res;
+}
versioned_symbol (libc, __pthread_getname_np, pthread_getname_np,
GLIBC_2_34);
diff --git a/sysdeps/pthread/tst-pthread-exited.c b/sysdeps/pthread/tst-pthread-exited.c
index 9fdbdfddc7..15d57a0440 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -63,6 +63,12 @@ do_test (void)
TEST_COMPARE (r, EINVAL);
}
+ {
+ char thread_name[16];
+ int r = pthread_getname_np (thr, thread_name, sizeof (thread_name));
+ TEST_COMPARE (r, EINVAL);
+ }
+
xpthread_join (thr);
return 0;
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 09/12] nptl: Use exit_lock when accessing TID on pthread_setname_np
2022-05-31 17:52 [PATCH v3 00/12] Fix various NPTL synchronization issues Adhemerval Zanella
` (7 preceding siblings ...)
2022-05-31 17:52 ` [PATCH v3 08/12] nptl: Use exit_lock when accessing TID on pthread_getname_np Adhemerval Zanella
@ 2022-05-31 17:52 ` Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 10/12] nptl: Use exit_lock when accessing TID on pthread_sigqueue Adhemerval Zanella
` (2 subsequent siblings)
11 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2022-05-31 17:52 UTC (permalink / raw)
To: libc-alpha
Also return EINVAL if the thread is already terminated at the time
of the call.
Checked on x86_64-linux-gnu.
---
nptl/pthread_setname.c | 52 +++++++++++++++++-----------
sysdeps/pthread/tst-pthread-exited.c | 6 ++++
2 files changed, 38 insertions(+), 20 deletions(-)
diff --git a/nptl/pthread_setname.c b/nptl/pthread_setname.c
index f24560db47..1dbbdcf63b 100644
--- a/nptl/pthread_setname.c
+++ b/nptl/pthread_setname.c
@@ -16,21 +16,40 @@
License along with the GNU C Library; see the file COPYING.LIB. If
not, see <https://www.gnu.org/licenses/>. */
-#include <errno.h>
-#include <fcntl.h>
+#include <intprops.h>
+#include <libc-lock.h>
+#include <not-cancel.h>
#include <pthreadP.h>
+#include <shlib-compat.h>
#include <stdio.h>
-#include <string.h>
-#include <unistd.h>
#include <sys/prctl.h>
-#include <not-cancel.h>
+static int
+setname (pid_t tid, const char *name, size_t name_len)
+{
+ char fname[sizeof ("/proc/self/task//comm") + INT_BUFSIZE_BOUND (pid_t)];
+ __snprintf (fname, sizeof (fname), "/proc/self/task/%d/comm", tid);
+ int fd = __open64_nocancel (fname, O_RDONLY);
+ if (fd == -1)
+ return EINVAL;
+
+ 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;
+
+ __close_nocancel_nostatus (fd);
+
+ return res;
+}
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 +61,15 @@ __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;
-
- 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 res = pd->tid > 0 ? setname (pd->tid, name, name_len) : EINVAL;
- __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 15d57a0440..7f5f339b3e 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -69,6 +69,12 @@ do_test (void)
TEST_COMPARE (r, EINVAL);
}
+ {
+ char thread_name[] = "test";
+ int r = pthread_setname_np (thr, thread_name);
+ TEST_COMPARE (r, EINVAL);
+ }
+
xpthread_join (thr);
return 0;
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 10/12] nptl: Use exit_lock when accessing TID on pthread_sigqueue
2022-05-31 17:52 [PATCH v3 00/12] Fix various NPTL synchronization issues Adhemerval Zanella
` (8 preceding siblings ...)
2022-05-31 17:52 ` [PATCH v3 09/12] nptl: Use exit_lock when accessing TID on pthread_setname_np Adhemerval Zanella
@ 2022-05-31 17:52 ` Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 11/12] nptl: Use exit_lock when accessing TID on pthread_setschedprio Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 12/12] nptl: Remove INVALID_TD_P Adhemerval Zanella
11 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2022-05-31 17:52 UTC (permalink / raw)
To: libc-alpha
Also return EINVAL if the thread is already terminated at the time
of the call.
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 48dc3ca4ee..e13cf9fe12 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 = EINVAL;
+
+ __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 7f5f339b3e..c4d8492f8c 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -75,6 +75,12 @@ do_test (void)
TEST_COMPARE (r, EINVAL);
}
+ {
+ union sigval value = { 0 };
+ int r = pthread_sigqueue (thr, SIGUSR1, value);
+ TEST_COMPARE (r, EINVAL);
+ }
+
xpthread_join (thr);
return 0;
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 11/12] nptl: Use exit_lock when accessing TID on pthread_setschedprio
2022-05-31 17:52 [PATCH v3 00/12] Fix various NPTL synchronization issues Adhemerval Zanella
` (9 preceding siblings ...)
2022-05-31 17:52 ` [PATCH v3 10/12] nptl: Use exit_lock when accessing TID on pthread_sigqueue Adhemerval Zanella
@ 2022-05-31 17:52 ` Adhemerval Zanella
2022-05-31 17:52 ` [PATCH v3 12/12] nptl: Remove INVALID_TD_P Adhemerval Zanella
11 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2022-05-31 17:52 UTC (permalink / raw)
To: libc-alpha
Also return EINVAL if the thread is already terminated at the time
of the call.
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 85f68d6f2d..8603624d80 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) : EINVAL;
+
+ __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 c4d8492f8c..ed4006c829 100644
--- a/sysdeps/pthread/tst-pthread-exited.c
+++ b/sysdeps/pthread/tst-pthread-exited.c
@@ -81,6 +81,11 @@ do_test (void)
TEST_COMPARE (r, EINVAL);
}
+ {
+ int r = pthread_setschedprio (thr, 0);
+ TEST_COMPARE (r, EINVAL);
+ }
+
xpthread_join (thr);
return 0;
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v3 12/12] nptl: Remove INVALID_TD_P
2022-05-31 17:52 [PATCH v3 00/12] Fix various NPTL synchronization issues Adhemerval Zanella
` (10 preceding siblings ...)
2022-05-31 17:52 ` [PATCH v3 11/12] nptl: Use exit_lock when accessing TID on pthread_setschedprio Adhemerval Zanella
@ 2022-05-31 17:52 ` Adhemerval Zanella
11 siblings, 0 replies; 13+ messages in thread
From: Adhemerval Zanella @ 2022-05-31 17:52 UTC (permalink / raw)
To: libc-alpha
Since 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 96d2015902..732e7c9874 100644
--- a/sysdeps/nptl/pthreadP.h
+++ b/sysdeps/nptl/pthreadP.h
@@ -239,11 +239,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.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread