[06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np

Message ID 20210930200051.1017457-7-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • Fix various NPTL synchronization issues
Related show

Commit Message

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

-- 
2.30.2

Comments

* 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
Cyril Hrubis Oct. 4, 2021, 12:45 p.m. | #2
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
Florian Weimer Oct. 4, 2021, 7:27 p.m. | #3
* 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.
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.
Cyril Hrubis Oct. 5, 2021, 7:35 a.m. | #5
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
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.
Florian Weimer Oct. 10, 2021, 2:37 p.m. | #7
* 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.

Patch

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>