[PATCH v6 2/4] nptl: Do not use pthread set_tid_address as state synchronization (BZ #19951)

[Florian Weimer]

* Adhemerval Zanella:

> @@ -496,6 +498,19 @@ start_thread (void *arg)
>       the breakpoint reports TD_THR_RUN state rather than TD_THR_ZOMBIE.  */
>    atomic_fetch_or_relaxed (&pd->cancelhandling, EXITING_BITMASK);
>  
> +  /* CONCURRENCY NOTES:
> +
> +     Concurrent pthread_detach() either sets the state to
> +     THREAD_STATE_DETACHED or waits for the thread to terminate.  The
> +     THREAD_STATE_EXITING state set here ensures that pthread_join() waits
> +     until all required cleanup steps are complete.
> +
> +     The 'prevstate' variable field will be used to determine who is
> +     responsible for calling __nptl_free_tcb below.  */
> +
> +  unsigned int prevstate = atomic_exchange_acquire (&pd->joinstate,
> +						    THREAD_STATE_EXITING);

The acquire seems unnecessary because it does not synchronize with
anything.

> @@ -865,10 +883,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_relaxed (&pd->joinstate))
> +                 != THREAD_STATE_EXITED)
> +	    __futex_abstimed_wait_cancelable64 (&pd->joinstate, state, 0,
> +                                                NULL, LLL_SHARED);
>          }

Shouldn't use a cancelable wait because pthread_create is declared
_THROWNL.  Maybe file a separate bug if you don't want to change this
now.

> diff --git a/nptl/pthread_detach.c b/nptl/pthread_detach.c
> index 3bbc037bdc..9b776ba9b0 100644
> --- a/nptl/pthread_detach.c
> +++ b/nptl/pthread_detach.c
> @@ -25,32 +25,32 @@ ___pthread_detach (pthread_t th)
>  {
>    struct pthread *pd = (struct pthread *) th;
>  
> +  /* CONCURRENCY NOTES:
>  
> +     Concurrent pthread_detach will return EINVAL for the case where 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.
>  
> +     In the case the thread is being terminated (THREAD_STATE_EXITING),
> +     pthread_detach will be responsible for cleaning up the stack.  */
> +
> +  unsigned int prevstate = atomic_load_relaxed (&pd->joinstate);
> +  do
>      {
> +      if (prevstate != THREAD_STATE_JOINABLE)
> +	{
> +	  if (prevstate == THREAD_STATE_DETACHED)
> +	    return EINVAL;
> +	  /* pthread_detach is declared _THROW so it need to call a
> +	     pthread_join variant that is not an cancellation entrypoint.  */

typo: not a[] cancellation entrypoint

> diff --git a/nptl/pthread_getattr_np.c b/nptl/pthread_getattr_np.c
> index 43dd16d59c..f6d7526041 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;

Should use relaxed, really.

> diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c
> index 54b528fd19..ff5f1e7faa 100644
> --- a/nptl/pthread_tryjoin.c
> +++ b/nptl/pthread_tryjoin.c
> @@ -21,15 +21,24 @@
>  int
>  __pthread_tryjoin_np (pthread_t threadid, void **thread_return)
>  {
> +  /* The joinable state (THREAD_STATE_JOINABLE) is straightforward: the thread
> +     hasn't finished yet, so trying to join might block.
>  
> +     The exiting thread (THREAD_STATE_EXITING) also might result in a blocking
> +     call: a detached thread might change its state to exiting, and an exiting
> +     thread might take some time to exit (and thus let the kernel set the
> +     state to THREAD_STATE_EXITED).
> +
> +     The ‘joinstate’ does not change during the thread lifetime once the
> +     kernel sets it to THREAD_STATE_EXITED.  The __pthread_clockjoin_ex will
> +     only call the cancellable futex if state is not THREAD_STATE_EXITED, so
> +     calling it should be safe wrt not making pthread_tryjoin_np a
> +     cancellable entrypoint (since it is marked as __THROW).  */
> +
> +  struct pthread *pd = (struct pthread *) threadid;
> +  return atomic_load_acquire (&pd->joinstate) != THREAD_STATE_EXITED
> +	 ? EBUSY
> +	 : __pthread_clockjoin_ex (threadid, thread_return, 0, NULL, false);
>  }
>  versioned_symbol (libc, __pthread_tryjoin_np, pthread_tryjoin_np, GLIBC_2_34);

Huh.  Isn't that a bug on its own because pthread_join is required to be
a cancellation point, so it needs to act on the cancellation whether or
not it blocks or not?

> diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h
> index 881a37cedd..88dea3c1e2 100644
> --- a/sysdeps/nptl/pthreadP.h
> +++ b/sysdeps/nptl/pthreadP.h

> @@ -518,8 +517,10 @@ extern int __pthread_setcanceltype (int type, int *oldtype);
>  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 *,
> +				   bool)
>    attribute_hidden;

Spurious change?

>  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 966e7c1289..fa8d4181f3 100644
> --- a/sysdeps/pthread/tst-thrd-detach.c
> +++ b/sysdeps/pthread/tst-thrd-detach.c

> @@ -43,6 +46,7 @@ do_test (void)
>    /* Give some time so the thread can finish.  */
>    thrd_sleep (&(struct timespec) {.tv_sec = 2}, NULL);
>  
> +  /* Calling thrd_join on a detached thread is UB... */
>    if (thrd_join (id, NULL) == thrd_success)
>      FAIL_EXIT1 ("thrd_join succeed where it should fail");

typo: succeed[ed]

Thanks,
Florian