diff mbox series

[v2,12/19] nptl: Use tidlock when accessing TID on pthread_getcpuclockid

Message ID 20210823195047.543237-13-adhemerval.zanella@linaro.org
State Superseded
Headers
Series Fix various NPTL synchronization issues |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella Netto Aug. 23, 2021, 7:50 p.m. UTC 
  Checked on x86_64-linux-gnu.
---
 nptl/pthread_getcpuclockid.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

Comments

Florian Weimer Aug. 26, 2021, 2:27 p.m. UTC | #1 
* Adhemerval Zanella:

> Checked on x86_64-linux-gnu.
> ---
>  nptl/pthread_getcpuclockid.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/nptl/pthread_getcpuclockid.c b/nptl/pthread_getcpuclockid.c
> index 0a6656ea4c..8c0db0b9ba 100644
> --- a/nptl/pthread_getcpuclockid.c
> +++ b/nptl/pthread_getcpuclockid.c
> @@ -29,16 +29,23 @@ __pthread_getcpuclockid (pthread_t threadid, clockid_t *clockid)
>    struct pthread *pd = (struct pthread *) threadid;
>  
>    /* Make sure the descriptor is valid.  */
> -  if (INVALID_TD_P (pd))
> -    /* Not a valid thread handle.  */
> -    return ESRCH;
> -
> -  /* The clockid_t value is a simple computation from the TID.  */
> -
> -  const clockid_t tidclock = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
> -
> -  *clockid = tidclock;
> -  return 0;
> +  sigset_t oldmask;
> +  __libc_signal_block_all (&oldmask);
> +  lll_lock (pd->tidlock, LLL_PRIVATE);
> +
> +  int res;
> +  if (pd->tid != 0)
> +    {
> +      *clockid = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
> +      res = 0;
> +    }
> +  else
> +    res = -ESRCH;
> +
> +  lll_unlock (pd->tidlock, LLL_PRIVATE);
> +  __libc_signal_restore_set (&oldmask);
> +
> +  return res;

This doesn't really solve the race, does it?  The caller cannot use the
clock ID safely.

Thanks,
Florian
Adhemerval Zanella Netto Aug. 26, 2021, 5:41 p.m. UTC | #2 
On 26/08/2021 11:27, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> Checked on x86_64-linux-gnu.
>> ---
>>  nptl/pthread_getcpuclockid.c | 27 +++++++++++++++++----------
>>  1 file changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/nptl/pthread_getcpuclockid.c b/nptl/pthread_getcpuclockid.c
>> index 0a6656ea4c..8c0db0b9ba 100644
>> --- a/nptl/pthread_getcpuclockid.c
>> +++ b/nptl/pthread_getcpuclockid.c
>> @@ -29,16 +29,23 @@ __pthread_getcpuclockid (pthread_t threadid, clockid_t *clockid)
>>    struct pthread *pd = (struct pthread *) threadid;
>>  
>>    /* Make sure the descriptor is valid.  */
>> -  if (INVALID_TD_P (pd))
>> -    /* Not a valid thread handle.  */
>> -    return ESRCH;
>> -
>> -  /* The clockid_t value is a simple computation from the TID.  */
>> -
>> -  const clockid_t tidclock = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
>> -
>> -  *clockid = tidclock;
>> -  return 0;
>> +  sigset_t oldmask;
>> +  __libc_signal_block_all (&oldmask);
>> +  lll_lock (pd->tidlock, LLL_PRIVATE);
>> +
>> +  int res;
>> +  if (pd->tid != 0)
>> +    {
>> +      *clockid = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
>> +      res = 0;
>> +    }
>> +  else
>> +    res = -ESRCH;
>> +
>> +  lll_unlock (pd->tidlock, LLL_PRIVATE);
>> +  __libc_signal_restore_set (&oldmask);
>> +
>> +  return res;
> 
> This doesn't really solve the race, does it?  The caller cannot use the
> clock ID safely.

Not really if the callee intention is to use along clock_gettime()
or any other routine.  I added this patch more for consistency.

In fact, it makes me wonder if the requested improvement such as
a pthread pid accessor [1] does really make sense, since it is
inherent racy in a way that there is no guarantee that the tid
will be valid when it is actually used.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=27880
Florian Weimer Aug. 30, 2021, 9:34 a.m. UTC | #3 
* Adhemerval Zanella:

> On 26/08/2021 11:27, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> Checked on x86_64-linux-gnu.
>>> ---
>>>  nptl/pthread_getcpuclockid.c | 27 +++++++++++++++++----------
>>>  1 file changed, 17 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/nptl/pthread_getcpuclockid.c b/nptl/pthread_getcpuclockid.c
>>> index 0a6656ea4c..8c0db0b9ba 100644
>>> --- a/nptl/pthread_getcpuclockid.c
>>> +++ b/nptl/pthread_getcpuclockid.c
>>> @@ -29,16 +29,23 @@ __pthread_getcpuclockid (pthread_t threadid, clockid_t *clockid)
>>>    struct pthread *pd = (struct pthread *) threadid;
>>>  
>>>    /* Make sure the descriptor is valid.  */
>>> -  if (INVALID_TD_P (pd))
>>> -    /* Not a valid thread handle.  */
>>> -    return ESRCH;
>>> -
>>> -  /* The clockid_t value is a simple computation from the TID.  */
>>> -
>>> -  const clockid_t tidclock = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
>>> -
>>> -  *clockid = tidclock;
>>> -  return 0;
>>> +  sigset_t oldmask;
>>> +  __libc_signal_block_all (&oldmask);
>>> +  lll_lock (pd->tidlock, LLL_PRIVATE);
>>> +
>>> +  int res;
>>> +  if (pd->tid != 0)
>>> +    {
>>> +      *clockid = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
>>> +      res = 0;
>>> +    }
>>> +  else
>>> +    res = -ESRCH;
>>> +
>>> +  lll_unlock (pd->tidlock, LLL_PRIVATE);
>>> +  __libc_signal_restore_set (&oldmask);
>>> +
>>> +  return res;
>> 
>> This doesn't really solve the race, does it?  The caller cannot use the
>> clock ID safely.
>
> Not really if the callee intention is to use along clock_gettime()
> or any other routine.  I added this patch more for consistency.
>
> In fact, it makes me wonder if the requested improvement such as
> a pthread pid accessor [1] does really make sense, since it is
> inherent racy in a way that there is no guarantee that the tid
> will be valid when it is actually used.
>
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=27880

I think it still makes sense because the caller can ensure that the
thread does not exit, so the TID remains valid while it is used.

The original TID (after the thread has exited) may also be useful for
debug logs.

Thanks,
Florian
diff mbox series

Patch

diff --git a/nptl/pthread_getcpuclockid.c b/nptl/pthread_getcpuclockid.c
index 0a6656ea4c..8c0db0b9ba 100644
--- a/nptl/pthread_getcpuclockid.c
+++ b/nptl/pthread_getcpuclockid.c
@@ -29,16 +29,23 @@  __pthread_getcpuclockid (pthread_t threadid, clockid_t *clockid)
   struct pthread *pd = (struct pthread *) threadid;
 
   /* Make sure the descriptor is valid.  */
-  if (INVALID_TD_P (pd))
-    /* Not a valid thread handle.  */
-    return ESRCH;
-
-  /* The clockid_t value is a simple computation from the TID.  */
-
-  const clockid_t tidclock = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
-
-  *clockid = tidclock;
-  return 0;
+  sigset_t oldmask;
+  __libc_signal_block_all (&oldmask);
+  lll_lock (pd->tidlock, LLL_PRIVATE);
+
+  int res;
+  if (pd->tid != 0)
+    {
+      *clockid = MAKE_THREAD_CPUCLOCK (pd->tid, CPUCLOCK_SCHED);
+      res = 0;
+    }
+  else
+    res = -ESRCH;
+
+  lll_unlock (pd->tidlock, LLL_PRIVATE);
+  __libc_signal_restore_set (&oldmask);
+
+  return res;
 }
 versioned_symbol (libc, __pthread_getcpuclockid, pthread_getcpuclockid,
                   GLIBC_2_34);