[PATCH v2 05/19] nptl: Move setxid flag out of cancelhandling
Florian Weimer
fweimer@redhat.com
Thu Aug 26 15:21:03 GMT 2021
* 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
More information about the Libc-alpha
mailing list