Re: [PATCH 3/3] nptl: Fix race between pthread_kill and thread exit (bug 12889)

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella:

> On 19/08/2021 13:37, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> On 17/08/2021 10:51, Florian Weimer via Libc-alpha wrote:
>>>> Thread exit is delayed until all pending pthread_kill operations
>>>> on the thread have completed.  This avoids sending signals to the
>>>> wrong thread or a spurious ESRCH error.
>>>>
>>>> The test sysdeps/pthread/tst-pthread_cancel-select-loop.c is derived
>>>> from a downstream test originally written by Marek Polacek.
>>>
>>> And I really hope we could first sort out the BZ#19951 and move away
>>> of using pthread::tid as the synchronization member to check thread
>>> termination.  I sent a possible fix [1], but it does not fully handle
>>> the pthread::tid access, it would require to a proper lock to 
>>> synchronize between thread exit and pthread_kill (as Rich has suggested
>>> on BZ#12889).
>>>
>>> I will need to send an updated version of my pthread fixes patchset,
>>> since clone3 changes broke it and INVALID_TD_P needs fixing as well.
>>>
>>> [1] https://patchwork.sourceware.org/project/glibc/patch/20210610193639.3650754-5-adhemerval.zanella@linaro.org/
>> 
>> I think my patch is independent of the other fixes.  Its advantage over
>> other proposals is that it does not add waiting to pthread_kill, so
>> there is no signal-safety issue.
>
> I am not sure, the way I think we should fix to also fix BZ#19951
> would to add a lock to access 'tid' and uses not only on pthread_kill,
> but also on pthread_getcpuclockid, pthread_getschedparam, 
> pthread_setschedparam, and pthread_setschedprio.  The lock also
> simplifies the code, there is no need to special handling of futex
> internal state. I also think pthread_kill would also be simplified.
>
> The lock is just to simplify the code, if we really need to add
> some scalability we might use a more clever code as you are proposing.

I don't think we can wait for the fix for bug 19951 because glibc 2.34
has a real regression in pthread_cancel: it can fail spuriously with
ESRCH.  And it can happen with a single pthread_cancel call in the
program, you don't even need concurrent pthread_cancel to trigger this
bug now.

I have moved the magic no-TID-reuse functionality into a separate
function in the new version, so we can replace it once we have something
better.

Thanks,
Florian