Re: [PATCH v2] nptl: Handle spurious EINTR when thread cancellation is disabled (BZ#29029)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 19/04/2022 09:10, Szabolcs Nagy wrote:
> The 04/14/2022 12:49, Adhemerval Zanella via Libc-alpha wrote:
>> --- a/nptl/pthread_cancel.c
>> +++ b/nptl/pthread_cancel.c
> ...
>> +  /* Some syscalls are never restarted after being interrupted by a signal
>> +     handler, regardless of the use of SA_RESTART (they always fail with
>> +     EINTR).  So pthread_cancel cannot send SIGCANCEL unless the cancellation
>> +     is enabled and set as asynchronous (in this case the cancellation will
>> +     be acted in the cancellation handler instead by the syscall wrapper).
>> +     Otherwise the target thread is set as 'cancelling' (CANCELING_BITMASK)
>> +     by atomically setting 'cancelhandling' and the cancelation will be acted
>> +     upon on next cancellation entrypoing in the target thread.
>> +
>> +     It also requires to atomically check if cancellation is enabled and
>> +     asynchronous, so both cancellation state and type are tracked on
>> +     'cancelhandling'.  */
>> +
>> +  int result = 0;
>> +  int oldval = atomic_load_relaxed (&pd->cancelhandling);
>> +  int newval;
>> +  do
>>      {
>> -      /* A single-threaded process should be able to kill itself, since there
>> -	 is nothing in the POSIX specification that says that it cannot.  So
>> -	 we set multiple_threads to true so that cancellation points get
>> -	 executed.  */
>> -      THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
>> +      newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
>> +      if (oldval == newval)
>> +	break;
>> +
>> +      /* If the cancellation is handled asynchronously just send a
>> +	 signal.  We avoid this if possible since it's more
>> +	 expensive.  */
>> +      if (cancel_enabled_and_canceled_and_async (newval))
>> +	{
>> +	  /* Mark the cancellation as "in progress".  */
>> +	  int newval2 = oldval | CANCELING_BITMASK;
>> +	  if (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling,
>> +						     &oldval, newval2))
>> +	    continue;
> 
> this continue looks wrong, the cas can fail spuriously
> (cancelhandling == oldval) and then continue jumps to...
> 
>> +
>> +	  if (pd == THREAD_SELF)
>> +	    /* This is not merely an optimization: An application may
>> +	       call pthread_cancel (pthread_self ()) without calling
>> +	       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 ();
>> +	    }
>> +	  else
>> +	    /* The cancellation handler will take care of marking the
>> +	       thread as canceled.  */
>> +	    result = __pthread_kill_internal (th, SIGCANCEL);
>> +
>> +	  break;
>> +	}
>> +
>> +	/* A single-threaded process should be able to kill itself, since
>> +	   there is nothing in the POSIX specification that says that it
>> +	   cannot.  So we set multiple_threads to true so that cancellation
>> +	   points get executed.  */
>> +	THREAD_SETMEM (THREAD_SELF, header.multiple_threads, 1);
>>  #ifndef TLS_MULTIPLE_THREADS_IN_TCB
>>        __libc_multiple_threads = 1;
>>  #endif
>> -
>> -      THREAD_SETMEM (pd, result, PTHREAD_CANCELED);
>> -      if (pd->cancelstate == PTHREAD_CANCEL_ENABLE
>> -	  && pd->canceltype == PTHREAD_CANCEL_ASYNCHRONOUS)
>> -	__do_cancel ();
>> -      return 0;
>>      }
>> +  while (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling, &oldval,
>> +						newval));
> 
> ...here and this cas updates cancelhandling to newval without
> actually doing any cancellation.

Yeah, it looks wrong indeed thanks for catching it.  Old code has a goto that
I tried to remove (wrongly):

  do
    {
    again:
      oldval = pd->cancelhandling;
      newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
      [...]
      if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
        {
          if (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling,
						    oldval | CANCELING_BITMASK,
						    oldval);
	    goto again;
          [...]	
        }
       [...]
     }
    while (atomic_compare_and_exchange_bool_acq (&pd->cancelhandling, newval,
						 oldval);

Do this fix the intermittent issue you are seeing:

diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
index c76882e279..e67b2df5cc 100644
--- a/nptl/pthread_cancel.c
+++ b/nptl/pthread_cancel.c
@@ -121,6 +121,7 @@ __pthread_cancel (pthread_t th)
   int newval;
   do
     {
+    again:
       newval = oldval | CANCELING_BITMASK | CANCELED_BITMASK;
       if (oldval == newval)
        break;
@@ -134,7 +135,7 @@ __pthread_cancel (pthread_t th)
          int newval2 = oldval | CANCELING_BITMASK;
          if (!atomic_compare_exchange_weak_acquire (&pd->cancelhandling,
                                                     &oldval, newval2))
-           continue;
+           goto again;
 
          if (pd == THREAD_SELF)
            /* This is not merely an optimization: An application may