[v2,2/9] nptl: Deallocate the thread stack on setup failure (BZ #19511)

Message ID 20210527172823.3461314-3-adhemerval.zanella@linaro.org
State New
Headers show
Series
  • nptl: pthread cancellation refactor
Related show

Commit Message

Adhemerval Zanella via Libc-alpha May 27, 2021, 5:28 p.m.
To setup either the thread scheduling parameters or affinity,
pthread_create enforce synchronization on created thread wait until
its parent either unlock the 'struct pthread' or send a cancellation
signal if a failure occurs.

However, cancelling the thread does not deallocate the newly created
stack since cancellation expects that a pthread_join to deallocate any
allocated thread resouces (threads stack or TLS).

I used a different strategy than the one proposed on BZ#19511
comment #4.  Making the parent responsible for the cleanup requires
additional synchronization similar to what pthread_join does.  Instead,
this patch reassigns an unused 'struct pthread' member,
parent_cancelhandling_unsed, to indicate whether the setup has failed
and set the thread itself to deallocate the allocated resouces (similar
to what detached mode does).

This strategy also simplifies by not using thread cancellation and
thus not running libgcc_so load in the signal handler (which is
avoided in thread cancellation since 'pthread_cancel' is the one
responsible to dlopen libgcc_s).

Checked on x86_64-linux-gnu and aarch64-linux-gnu.
---
 nptl/allocatestack.c  |  1 +
 nptl/descr.h          |  5 ++--
 nptl/pthread_create.c | 70 ++++++++++++++++++++-----------------------
 3 files changed, 36 insertions(+), 40 deletions(-)

-- 
2.30.2

Comments

Adhemerval Zanella via Libc-alpha June 1, 2021, 8:32 a.m. | #1
* Adhemerval Zanella via Libc-alpha:

> @@ -424,17 +409,22 @@ start_thread (void *arg)

>  	 have ownership (see CONCURRENCY NOTES above).  */

>        if (__glibc_unlikely (pd->stopped_start))

>  	{

>  	  /* Get the lock the parent locked to force synchronization.  */

>  	  lll_lock (pd->lock, LLL_PRIVATE);

>  

>  	  /* We have ownership of PD now.  */

> +	  if (pd->setup_failed == 1)

> +	    {

> +	      /* In the case of a failed setup, the created thread will be

> +		 responsible to free up the allocated resources.  The

> +		 detached mode ensure it won't be joined and it will trigger

> +		 the required cleanup.  */

> +	      pd->joinid = pd;

> +	      __do_cancel ();

> +	    }

>  

>  	  /* And give it up right away.  */

>  	  lll_unlock (pd->lock, LLL_PRIVATE);

>  	}


I don't think this is quite right.  The failed thread should not linger
around after pthread_create has reported failure to the caller.  It's
better than what we had before, so maybe we should use this version and
clean it up further later.

At the point of the __do_cancel call, unwinding may not yet initialized,
so the implied dlopen happens on a potentially very small stack.  It
should be possible to replace it with a goto to the cleanup code.  Or
maybe put the cleanup code into a separate function and call it here and
on the regular cleanup path.

Apart from that, it looks fine to me.

Thanks,
Florian
Adhemerval Zanella via Libc-alpha June 1, 2021, 1:08 p.m. | #2
On 01/06/2021 05:32, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:

> 

>> @@ -424,17 +409,22 @@ start_thread (void *arg)

>>  	 have ownership (see CONCURRENCY NOTES above).  */

>>        if (__glibc_unlikely (pd->stopped_start))

>>  	{

>>  	  /* Get the lock the parent locked to force synchronization.  */

>>  	  lll_lock (pd->lock, LLL_PRIVATE);

>>  

>>  	  /* We have ownership of PD now.  */

>> +	  if (pd->setup_failed == 1)

>> +	    {

>> +	      /* In the case of a failed setup, the created thread will be

>> +		 responsible to free up the allocated resources.  The

>> +		 detached mode ensure it won't be joined and it will trigger

>> +		 the required cleanup.  */

>> +	      pd->joinid = pd;

>> +	      __do_cancel ();

>> +	    }

>>  

>>  	  /* And give it up right away.  */

>>  	  lll_unlock (pd->lock, LLL_PRIVATE);

>>  	}

> 

> I don't think this is quite right.  The failed thread should not linger

> around after pthread_create has reported failure to the caller.  It's

> better than what we had before, so maybe we should use this version and

> clean it up further later.


Agreed, but I think this is really an improvement over current strategy
as I commented earlier [1].  I think to proper fix would move the
lock prior any initialization so we don't need to run any unwinding
operations, skip directly to INTERNAL_SYSCALL_CALL (exit, 0), and let the
parent join the thread and do the resource deallocation.

> 

> At the point of the __do_cancel call, unwinding may not yet initialized,

> so the implied dlopen happens on a potentially very small stack.  It

> should be possible to replace it with a goto to the cleanup code.  Or

> maybe put the cleanup code into a separate function and call it here and

> on the regular cleanup path.

> 

> Apart from that, it looks fine to me.


The __do_cancel is not done really in asynchronous mode, so it will use
the allocated thread stack for the dlopen.  But I think with a better
strategy to move the 'struct thread' lock earlier than any initialization
will avoid such issue.

> 

> Thanks,

> Florian

> 


[1] https://sourceware.org/pipermail/libc-alpha/2021-May/126879.html
Adhemerval Zanella via Libc-alpha June 1, 2021, 1:55 p.m. | #3
On 01/06/2021 10:08, Adhemerval Zanella wrote:
> 

> 

> On 01/06/2021 05:32, Florian Weimer wrote:

>> * Adhemerval Zanella via Libc-alpha:

>>

>>> @@ -424,17 +409,22 @@ start_thread (void *arg)

>>>  	 have ownership (see CONCURRENCY NOTES above).  */

>>>        if (__glibc_unlikely (pd->stopped_start))

>>>  	{

>>>  	  /* Get the lock the parent locked to force synchronization.  */

>>>  	  lll_lock (pd->lock, LLL_PRIVATE);

>>>  

>>>  	  /* We have ownership of PD now.  */

>>> +	  if (pd->setup_failed == 1)

>>> +	    {

>>> +	      /* In the case of a failed setup, the created thread will be

>>> +		 responsible to free up the allocated resources.  The

>>> +		 detached mode ensure it won't be joined and it will trigger

>>> +		 the required cleanup.  */

>>> +	      pd->joinid = pd;

>>> +	      __do_cancel ();

>>> +	    }

>>>  

>>>  	  /* And give it up right away.  */

>>>  	  lll_unlock (pd->lock, LLL_PRIVATE);

>>>  	}

>>

>> I don't think this is quite right.  The failed thread should not linger

>> around after pthread_create has reported failure to the caller.  It's

>> better than what we had before, so maybe we should use this version and

>> clean it up further later.

> 

> Agreed, but I think this is really an improvement over current strategy

> as I commented earlier [1].  I think to proper fix would move the

> lock prior any initialization so we don't need to run any unwinding

> operations, skip directly to INTERNAL_SYSCALL_CALL (exit, 0), and let the

> parent join the thread and do the resource deallocation.

> 

>>

>> At the point of the __do_cancel call, unwinding may not yet initialized,

>> so the implied dlopen happens on a potentially very small stack.  It

>> should be possible to replace it with a goto to the cleanup code.  Or

>> maybe put the cleanup code into a separate function and call it here and

>> on the regular cleanup path.

>>

>> Apart from that, it looks fine to me.

> 

> The __do_cancel is not done really in asynchronous mode, so it will use

> the allocated thread stack for the dlopen.  But I think with a better

> strategy to move the 'struct thread' lock earlier than any initialization

> will avoid such issue.

> 

>>

>> Thanks,

>> Florian

>>

> 

> [1] https://sourceware.org/pipermail/libc-alpha/2021-May/126879.html

> 


The patch below on top of this patch implements the synchronous thread
join in case of setup failure.  It shows no regressions on testcases,
and it is slight better because it avoid calling the __pthread_unwind for
such cases (and issuing the libgcc dlopen)

In fact since there is no registered user case that might required
calling either cancellation cleanup handler or thread destructors,
I think there is no need to actually issue a pthread_exit like
termination.

--

diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 018af30c85..7a4c742416 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -346,6 +346,23 @@ start_thread (void *arg)
 {
   struct pthread *pd = arg;
 
+  /* We are either in (a) or (b), and in either case we either own PD already
+     (2) or are about to own PD (1), and so our only restriction would be that
+     we can't free PD until we know we have ownership (see CONCURRENCY NOTES
+     above).  */
+  if (__glibc_unlikely (pd->stopped_start))
+    {
+      /* Get the lock the parent locked to force synchronization.  */
+      lll_lock (pd->lock, LLL_PRIVATE);
+
+      /* We have ownership of PD now.  */
+      if (pd->setup_failed == 1)
+	goto out;
+
+      /* And give it up right away.  */
+      lll_unlock (pd->lock, LLL_PRIVATE);
+     }
+
   /* Initialize resolver state pointer.  */
   __resp = &pd->res;
 
@@ -403,30 +420,6 @@ start_thread (void *arg)
       /* Store the new cleanup handler info.  */
       THREAD_SETMEM (pd, cleanup_jmp_buf, &unwind_buf);
 
-      /* We are either in (a) or (b), and in either case we either own
-         PD already (2) or are about to own PD (1), and so our only
-	 restriction would be that we can't free PD until we know we
-	 have ownership (see CONCURRENCY NOTES above).  */
-      if (__glibc_unlikely (pd->stopped_start))
-	{
-	  /* Get the lock the parent locked to force synchronization.  */
-	  lll_lock (pd->lock, LLL_PRIVATE);
-
-	  /* We have ownership of PD now.  */
-	  if (pd->setup_failed == 1)
-	    {
-	      /* In the case of a failed setup, the created thread will be
-		 responsible to free up the allocated resources.  The
-		 detached mode ensure it won't be joined and it will trigger
-		 the required cleanup.  */
-	      pd->joinid = pd;
-	      __do_cancel ();
-	    }
-
-	  /* And give it up right away.  */
-	  lll_unlock (pd->lock, LLL_PRIVATE);
-	}
-
       LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
 
       /* Run the code the user provided.  */
@@ -556,6 +549,7 @@ start_thread (void *arg)
     /* Free the TCB.  */
     __nptl_free_tcb (pd);
 
+out:
   /* We cannot call '_exit' here.  '_exit' will terminate the process.
 
      The 'exit' implementation in the kernel will signal when the
@@ -802,6 +796,8 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
      thread.  */
   __libc_signal_restore_set (&original_sigmask);
 
+  int setup_failed = 0;
+
   if (__glibc_unlikely (retval != 0))
     {
       if (thread_ran)
@@ -814,7 +810,7 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
   	  /* State (a), we own PD. The thread blocked on this lock will
 	     finish due setup_failed being set.  The thread will be
 	     responsible to cleanup any allocated resource.  */
-	  pd->setup_failed = 1;
+	  setup_failed = pd->setup_failed = 1;
 	  lll_unlock (pd->lock, LLL_PRIVATE);
         }
       else
@@ -857,6 +853,9 @@ __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
       THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
     }
 
+  if (setup_failed == 1)
+    __pthread_join (*newthread, NULL);
+
  out:
   if (destroy_default_attr)
     __pthread_attr_destroy (&default_attr.external);

Patch

diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index dc81a2ca73..2114bd2e27 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -161,6 +161,7 @@  get_cached_stack (size_t *sizep, void **memp)
   /* Cancellation handling is back to the default.  */
   result->cancelhandling = 0;
   result->cleanup = NULL;
+  result->setup_failed = 0;
 
   /* No pending event.  */
   result->nextevent = NULL;
diff --git a/nptl/descr.h b/nptl/descr.h
index 3de9535449..9d8297b45f 100644
--- a/nptl/descr.h
+++ b/nptl/descr.h
@@ -340,8 +340,9 @@  struct pthread
   /* True if thread must stop at startup time.  */
   bool stopped_start;
 
-  /* Formerly used for dealing with cancellation.  */
-  int parent_cancelhandling_unsed;
+  /* Indicate that a thread creation setup has failed (for instance the
+     scheduler or affinity).  */
+  int setup_failed;
 
   /* Lock to synchronize access to the descriptor.  */
   int lock;
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index aacbba1cac..018af30c85 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -169,17 +169,15 @@  late_init (void)
 
    (c) If the detached thread setup failed and THREAD_RAN is true, then
        the creating thread releases ownership to the new thread by
-       sending a cancellation signal.  All threads set THREAD_RAN to
-       true as quickly as possible after returning from the OS kernel's
-       thread creation routine.
+       setting the PD->setup_failed 1 and releasing PD->lock. All threads
+       set THREAD_RAN to true as quickly as possible after returning from
+       the OS kernel's thread creation routine.
 
    (d) If the joinable thread setup failed and THREAD_RAN is true, then
        then the creating thread retains ownership of PD and must cleanup
        state.  Ownership cannot be released to the process via the
        return of pthread_create since a non-zero result entails PD is
        undefined and therefore cannot be joined to free the resources.
-       We privately call pthread_join on the thread to finish handling
-       the resource shutdown (Or at least we should, see bug 19511).
 
    (e) If the thread creation failed and THREAD_RAN is false, then the
        creating thread retains ownership of PD and must cleanup state.
@@ -239,8 +237,8 @@  late_init (void)
    The return value is zero for success or an errno code for failure.
    If the return value is ENOMEM, that will be translated to EAGAIN,
    so create_thread need not do that.  On failure, *THREAD_RAN should
-   be set to true iff the thread actually started up and then got
-   canceled before calling user code (*PD->start_routine).  */
+   be set to true iff the thread actually started up but before calling
+   the user code (*PD->start_routine).  */
 
 static int _Noreturn start_thread (void *arg);
 
@@ -308,35 +306,23 @@  static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
 			== -1))
     return errno;
 
-  /* It's started now, so if we fail below, we'll have to cancel it
-     and let it clean itself up.  */
+  /* It's started now, so if we fail below, we'll have to let it clean itself
+     up.  */
   *thread_ran = true;
 
   /* Now we have the possibility to set scheduling parameters etc.  */
   if (attr != NULL)
     {
-      int res;
-
       /* Set the affinity mask if necessary.  */
       if (need_setaffinity)
 	{
 	  assert (*stopped_start);
 
-	  res = INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid,
-				       attr->extension->cpusetsize,
-				       attr->extension->cpuset);
-
+	  int res = INTERNAL_SYSCALL_CALL (sched_setaffinity, pd->tid,
+					   attr->extension->cpusetsize,
+					   attr->extension->cpuset);
 	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
-	  err_out:
-	    {
-	      /* The operation failed.  We have to kill the thread.
-		 We let the normal cancellation mechanism do the work.  */
-
-	      pid_t pid = __getpid ();
-	      INTERNAL_SYSCALL_CALL (tgkill, pid, pd->tid, SIGCANCEL);
-
-	      return INTERNAL_SYSCALL_ERRNO (res);
-	    }
+            return INTERNAL_SYSCALL_ERRNO (res);
 	}
 
       /* Set the scheduling parameters.  */
@@ -344,11 +330,10 @@  static int create_thread (struct pthread *pd, const struct pthread_attr *attr,
 	{
 	  assert (*stopped_start);
 
-	  res = INTERNAL_SYSCALL_CALL (sched_setscheduler, pd->tid,
-				       pd->schedpolicy, &pd->schedparam);
-
+	  int res = INTERNAL_SYSCALL_CALL (sched_setscheduler, pd->tid,
+					   pd->schedpolicy, &pd->schedparam);
 	  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (res)))
-	    goto err_out;
+            return INTERNAL_SYSCALL_ERRNO (res);
 	}
     }
 
@@ -424,17 +409,22 @@  start_thread (void *arg)
 	 have ownership (see CONCURRENCY NOTES above).  */
       if (__glibc_unlikely (pd->stopped_start))
 	{
-	  int oldtype = LIBC_CANCEL_ASYNC ();
-
 	  /* Get the lock the parent locked to force synchronization.  */
 	  lll_lock (pd->lock, LLL_PRIVATE);
 
 	  /* We have ownership of PD now.  */
+	  if (pd->setup_failed == 1)
+	    {
+	      /* In the case of a failed setup, the created thread will be
+		 responsible to free up the allocated resources.  The
+		 detached mode ensure it won't be joined and it will trigger
+		 the required cleanup.  */
+	      pd->joinid = pd;
+	      __do_cancel ();
+	    }
 
 	  /* And give it up right away.  */
 	  lll_unlock (pd->lock, LLL_PRIVATE);
-
-	  LIBC_CANCEL_RESET (oldtype);
 	}
 
       LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
@@ -561,7 +551,7 @@  start_thread (void *arg)
       pd->setxid_futex = 0;
     }
 
-  /* If the thread is detached free the TCB.  */
+  /* If the thread is detached or an setup error occurred, free the TCB.  */
   if (IS_DETACHED (pd))
     /* Free the TCB.  */
     __nptl_free_tcb (pd);
@@ -761,7 +751,6 @@  __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
 	 signal mask of this thread, so save it in the startup
 	 information.  */
       pd->sigmask = original_sigmask;
-
       /* Reset the cancellation signal mask in case this thread is
 	 running cancellation.  */
       __sigdelset (&pd->sigmask, SIGCANCEL);
@@ -820,9 +809,14 @@  __pthread_create_2_1 (pthread_t *newthread, const pthread_attr_t *attr,
 	   CONCURRENCY NOTES above).  We can assert that STOPPED_START
 	   must have been true because thread creation didn't fail, but
 	   thread attribute setting did.  */
-	/* See bug 19511 which explains why doing nothing here is a
-	   resource leak for a joinable thread.  */
-	assert (stopped_start);
+        {
+	  assert (stopped_start);
+  	  /* State (a), we own PD. The thread blocked on this lock will
+	     finish due setup_failed being set.  The thread will be
+	     responsible to cleanup any allocated resource.  */
+	  pd->setup_failed = 1;
+	  lll_unlock (pd->lock, LLL_PRIVATE);
+        }
       else
 	{
 	  /* State (e) and we have ownership of PD (see CONCURRENCY