Re: [PATCH v2 05/19] nptl: Move setxid flag out of cancelhandling

[Florian Weimer via Libc-alpha <libc-alpha@sourceware.org>]

* Adhemerval Zanella:

> On 26/08/2021 08:34, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> Now that the thread state is tracked by 'joinstate' field, there is no
>>> need to keep the setxid flag within the 'cancelhandling'.  It simplifies
>>> the atomic code to set and reset it (since there is no need to handle
>>> the thread state concurrent update).
>> 
>> Some of this functionality reimplements the tidlock.
>> 
>> There is some functionality around thread creation, but I do not know if
>> the synchronization is entirely correct.  This makes it rather difficult
>> to review changes.
>> 
>> It's also not clear to me if we need to do anything to prevent early
>> setxid calls:
>> 
>>>  	  /* Don't allow setxid until cloned.  */
>> 
>> I suspect this should just look at the TID instead.  We create the
>> thread with all signals blocked, so once there is a TID (and the thread
>> has not exited), it is safe to send the signal.
>
> I agree that we might improve the setxid code, however what I intended
> with this changes is to keep its semantic as-is and just decouple it
> from cancelhandling.  I might revise it on a later patch to follow your
> suggestions.

I wonder if there is a new race here:

@@ -109,41 +100,23 @@ setxid_mark_thread (struct xid_command *cmdp, struct pthread *t)
   /* Don't let the thread exit before the setxid handler runs.  */
   t->setxid_futex = 0;
 
+  /* If thread is exiting right now, ignore it.  */
+  if (atomic_load_acquire (&t->joinstate) == THREAD_STATE_EXITING)
+    return;
 
+  /* Release the futex if there is no other setxid in progress.  */
+  if (atomic_exchange_acquire (&t->setxid_flag, 1) == 0)
+    {
+      t->setxid_futex = 1;
+      futex_wake (&t->setxid_futex, 1, FUTEX_PRIVATE);
     }

That is, if the setxid-sending thread can now wait for a futex wake that
never comes.

There's also a use of atomic_exchange_release whose result is not used:

+  atomic_exchange_release (&t->setxid_flag, 0);

That should probably be store.

Thanks,
Florian