[09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h

  The idea is to make NPTL implementation to use on the functions
provided by futex-internal.h.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 nptl/lowlevellock.c                 | 6 +++---
 nptl/pthread_mutex_lock.c           | 9 +++++----
 nptl/pthread_mutex_setprioceiling.c | 5 +++--
 nptl/pthread_mutex_timedlock.c      | 6 +++---
 4 files changed, 14 insertions(+), 12 deletions(-)
  

Hi Adhemerval,

> The idea is to make NPTL implementation to use on the functions
> provided by futex-internal.h.
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
>  nptl/lowlevellock.c                 | 6 +++---
>  nptl/pthread_mutex_lock.c           | 9 +++++----
>  nptl/pthread_mutex_setprioceiling.c | 5 +++--
>  nptl/pthread_mutex_timedlock.c      | 6 +++---
>  4 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/nptl/lowlevellock.c b/nptl/lowlevellock.c
> index f69547a235..973df4d03a 100644
> --- a/nptl/lowlevellock.c
> +++ b/nptl/lowlevellock.c
> @@ -18,7 +18,7 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <sysdep.h>
> -#include <lowlevellock.h>
> +#include <futex-internal.h>
>  #include <atomic.h>
>  #include <stap-probe.h>
>  
> @@ -32,7 +32,7 @@ __lll_lock_wait_private (int *futex)
>      {
>      futex:
>        LIBC_PROBE (lll_lock_wait_private, 1, futex);
> -      lll_futex_wait (futex, 2, LLL_PRIVATE); /* Wait if *futex ==
> 2.  */
> +      futex_wait ((unsigned int *) futex, 2, LLL_PRIVATE); /* Wait
> if *futex == 2.  */ }
>  }
>  
> @@ -49,7 +49,7 @@ __lll_lock_wait (int *futex, int private)
>      {
>      futex:
>        LIBC_PROBE (lll_lock_wait, 1, futex);
> -      lll_futex_wait (futex, 2, private); /* Wait if *futex == 2.  */
> +      futex_wait ((unsigned int *) futex, 2, private); /* Wait if
> *futex == 2.  */ }
>  }
>  #endif
> diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
> index 1f137f6201..965c5a3f4a 100644
> --- a/nptl/pthread_mutex_lock.c
> +++ b/nptl/pthread_mutex_lock.c
> @@ -307,8 +307,8 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
>  	  assume_other_futex_waiters |= FUTEX_WAITERS;
>  
>  	  /* Block using the futex and reload current lock value.  */
> -	  lll_futex_wait (&mutex->__data.__lock, oldval,
> -			  PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
> +	  futex_wait ((unsigned int *) &mutex->__data.__lock, oldval,
> +		      PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
>  	  oldval = mutex->__data.__lock;
>  	}
>  
> @@ -568,8 +568,9 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
>  		  break;
>  
>  		if (oldval != ceilval)
> -		  lll_futex_wait (&mutex->__data.__lock, ceilval | 2,
> -				  PTHREAD_MUTEX_PSHARED (mutex));
> +		  futex_wait ((unsigned int * )
> &mutex->__data.__lock,
> +			      ceilval | 2,
> +			      PTHREAD_MUTEX_PSHARED (mutex));
>  	      }
>  	    while (atomic_compare_and_exchange_val_acq
> (&mutex->__data.__lock, ceilval | 2, ceilval)
> diff --git a/nptl/pthread_mutex_setprioceiling.c
> b/nptl/pthread_mutex_setprioceiling.c index 521da72622..cbef202579
> 100644 --- a/nptl/pthread_mutex_setprioceiling.c
> +++ b/nptl/pthread_mutex_setprioceiling.c
> @@ -21,6 +21,7 @@
>  #include <errno.h>
>  #include <pthreadP.h>
>  #include <atomic.h>
> +#include <futex-internal.h>
>  
>  
>  int
> @@ -84,8 +85,8 @@ pthread_mutex_setprioceiling (pthread_mutex_t
> *mutex, int prioceiling, break;
>  
>  	    if (oldval != ceilval)
> -	      lll_futex_wait (&mutex->__data.__lock, ceilval | 2,
> -			      PTHREAD_MUTEX_PSHARED (mutex));
> +	      futex_wait ((unsigned int *) &mutex->__data.__lock,
> ceilval | 2,
> +			  PTHREAD_MUTEX_PSHARED (mutex));
>  	  }
>  	while (atomic_compare_and_exchange_val_acq
> (&mutex->__data.__lock, ceilval | 2, ceilval)
> diff --git a/nptl/pthread_mutex_timedlock.c
> b/nptl/pthread_mutex_timedlock.c index e643eab258..343acf6107 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t
> *mutex, goto failpp;
>  		      }
>  
> -		    lll_futex_timed_wait (&mutex->__data.__lock,
> -					  ceilval | 2, &rt,
> -					  PTHREAD_MUTEX_PSHARED
> (mutex));
> +		    __futex_abstimed_wait64 (
> +		      (unsigned int *) &mutex->__data.__lock,
> clockid,
> +		      ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED
> (mutex)); }
>  	      }
>  	    while (atomic_compare_and_exchange_val_acq
> (&mutex->__data.__lock,

Reviewed-by: Lukasz Majewski <lukma@denx.de>


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
> The idea is to make NPTL implementation to use on the functions
> provided by futex-internal.h.
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
>  nptl/lowlevellock.c                 | 6 +++---
>  nptl/pthread_mutex_lock.c           | 9 +++++----
>  nptl/pthread_mutex_setprioceiling.c | 5 +++--
>  nptl/pthread_mutex_timedlock.c      | 6 +++---
>  4 files changed, 14 insertions(+), 12 deletions(-)

[snip]

> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> index e643eab258..343acf6107 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>  			goto failpp;
>  		      }
>  
> -		    lll_futex_timed_wait (&mutex->__data.__lock,
> -					  ceilval | 2, &rt,
> -					  PTHREAD_MUTEX_PSHARED (mutex));
> +		    __futex_abstimed_wait64 (
> +		      (unsigned int *) &mutex->__data.__lock, clockid,
> +		      ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));

I think you've replaced the lll_futex_timed_wait call that expects a
relative timeout with a __futex_abstimed_wait64 call that expects an
absolute timeout, yet you still appear to be passing the relative timeout.

However, it turns out that the implementation for the
PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
completely broken with clockid != CLOCK_REALTIME ever since I added it in
9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
time this was a less obvious __gettimeofday call.)

I'll work on writing some test cases for the those types of mutex in the
hope of catching both flaws before fixing them.

Mike.
  

On 25/11/2020 12:32, Mike Crowe wrote:
> On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
>> The idea is to make NPTL implementation to use on the functions
>> provided by futex-internal.h.
>>
>> Checked on x86_64-linux-gnu and i686-linux-gnu.
>> ---
>>  nptl/lowlevellock.c                 | 6 +++---
>>  nptl/pthread_mutex_lock.c           | 9 +++++----
>>  nptl/pthread_mutex_setprioceiling.c | 5 +++--
>>  nptl/pthread_mutex_timedlock.c      | 6 +++---
>>  4 files changed, 14 insertions(+), 12 deletions(-)
> 
> [snip]
> 
>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>> index e643eab258..343acf6107 100644
>> --- a/nptl/pthread_mutex_timedlock.c
>> +++ b/nptl/pthread_mutex_timedlock.c
>> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>  			goto failpp;
>>  		      }
>>  
>> -		    lll_futex_timed_wait (&mutex->__data.__lock,
>> -					  ceilval | 2, &rt,
>> -					  PTHREAD_MUTEX_PSHARED (mutex));
>> +		    __futex_abstimed_wait64 (
>> +		      (unsigned int *) &mutex->__data.__lock, clockid,
>> +		      ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
> 
> I think you've replaced the lll_futex_timed_wait call that expects a
> relative timeout with a __futex_abstimed_wait64 call that expects an
> absolute timeout, yet you still appear to be passing the relative timeout.
> 
> However, it turns out that the implementation for the
> PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
> completely broken with clockid != CLOCK_REALTIME ever since I added it in
> 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
> is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
> time this was a less obvious __gettimeofday call.)
> 
> I'll work on writing some test cases for the those types of mutex in the
> hope of catching both flaws before fixing them.

Indeed, there is no need to calculate the relative timeout anymore. I think
the fix below should pass the absolute timeout directly.   I will check
a possible regression tests as well.


diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
index aaaafa21ce..86c5f4446e 100644
--- a/nptl/pthread_mutex_timedlock.c
+++ b/nptl/pthread_mutex_timedlock.c
@@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 	    if (__pthread_current_priority () > ceiling)
 	      {
 		result = EINVAL;
-	      failpp:
 		if (oldprio != -1)
 		  __pthread_tpp_change_priority (oldprio, -1);
 		return result;
@@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 
 		if (oldval != ceilval)
 		  {
-		    /* Reject invalid timeouts.  */
-		    if (! valid_nanoseconds (abstime->tv_nsec))
-		      {
-			result = EINVAL;
-			goto failpp;
-		      }
-
-		    struct __timespec64 rt;
-
-		    /* Get the current time.  */
-		    __clock_gettime64 (CLOCK_REALTIME, &rt);
-
-		    /* Compute relative timeout.  */
-		    rt.tv_sec = abstime->tv_sec - rt.tv_sec;
-		    rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
-		    if (rt.tv_nsec < 0)
-		      {
-			rt.tv_nsec += 1000000000;
-			--rt.tv_sec;
-		      }
-
-		    /* Already timed out?  */
-		    if (rt.tv_sec < 0)
-		      {
-			result = ETIMEDOUT;
-			goto failpp;
-		      }
-
 		    __futex_abstimed_wait64 (
 		      (unsigned int *) &mutex->__data.__lock, clockid,
-		      ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
+		      ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex));
 		  }
 	      }
 	    while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,
  

On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote:
> 
> 
> On 25/11/2020 12:32, Mike Crowe wrote:
> > On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
> >> The idea is to make NPTL implementation to use on the functions
> >> provided by futex-internal.h.
> >>
> >> Checked on x86_64-linux-gnu and i686-linux-gnu.
> >> ---
> >>  nptl/lowlevellock.c                 | 6 +++---
> >>  nptl/pthread_mutex_lock.c           | 9 +++++----
> >>  nptl/pthread_mutex_setprioceiling.c | 5 +++--
> >>  nptl/pthread_mutex_timedlock.c      | 6 +++---
> >>  4 files changed, 14 insertions(+), 12 deletions(-)
> > 
> > [snip]
> > 
> >> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> >> index e643eab258..343acf6107 100644
> >> --- a/nptl/pthread_mutex_timedlock.c
> >> +++ b/nptl/pthread_mutex_timedlock.c
> >> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
> >>  			goto failpp;
> >>  		      }
> >>  
> >> -		    lll_futex_timed_wait (&mutex->__data.__lock,
> >> -					  ceilval | 2, &rt,
> >> -					  PTHREAD_MUTEX_PSHARED (mutex));
> >> +		    __futex_abstimed_wait64 (
> >> +		      (unsigned int *) &mutex->__data.__lock, clockid,
> >> +		      ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
> > 
> > I think you've replaced the lll_futex_timed_wait call that expects a
> > relative timeout with a __futex_abstimed_wait64 call that expects an
> > absolute timeout, yet you still appear to be passing the relative timeout.
> > 
> > However, it turns out that the implementation for the
> > PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
> > completely broken with clockid != CLOCK_REALTIME ever since I added it in
> > 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
> > is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
> > time this was a less obvious __gettimeofday call.)
> > 
> > I'll work on writing some test cases for the those types of mutex in the
> > hope of catching both flaws before fixing them.
> 
> Indeed, there is no need to calculate the relative timeout anymore. I think
> the fix below should pass the absolute timeout directly.   I will check
> a possible regression tests as well.

OK. I won't then. Thanks.

> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> index aaaafa21ce..86c5f4446e 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>  	    if (__pthread_current_priority () > ceiling)
>  	      {
>  		result = EINVAL;
> -	      failpp:
>  		if (oldprio != -1)
>  		  __pthread_tpp_change_priority (oldprio, -1);
>  		return result;
> @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>  
>  		if (oldval != ceilval)
>  		  {
> -		    /* Reject invalid timeouts.  */
> -		    if (! valid_nanoseconds (abstime->tv_nsec))
> -		      {
> -			result = EINVAL;
> -			goto failpp;
> -		      }

If this is removed then is there a risk of getting into a busy loop if
someone passes a bogus timespec? (Regardless of the answer, it makes sense
to ensure that is tested somehow.)

> -
> -		    struct __timespec64 rt;
> -
> -		    /* Get the current time.  */
> -		    __clock_gettime64 (CLOCK_REALTIME, &rt);
> -
> -		    /* Compute relative timeout.  */
> -		    rt.tv_sec = abstime->tv_sec - rt.tv_sec;
> -		    rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
> -		    if (rt.tv_nsec < 0)
> -		      {
> -			rt.tv_nsec += 1000000000;
> -			--rt.tv_sec;
> -		      }
> -
> -		    /* Already timed out?  */
> -		    if (rt.tv_sec < 0)
> -		      {
> -			result = ETIMEDOUT;
> -			goto failpp;
> -		      }
> -
>  		    __futex_abstimed_wait64 (
>  		      (unsigned int *) &mutex->__data.__lock, clockid,
> -		      ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
> +		      ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex));
>  		  }
>  	      }
>  	    while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,

LGTM.

Thanks.

Mike.
  

On 25/11/2020 12:46, Mike Crowe wrote:
> On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote:
>>
>>
>> On 25/11/2020 12:32, Mike Crowe wrote:
>>> On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
>>>> The idea is to make NPTL implementation to use on the functions
>>>> provided by futex-internal.h.
>>>>
>>>> Checked on x86_64-linux-gnu and i686-linux-gnu.
>>>> ---
>>>>  nptl/lowlevellock.c                 | 6 +++---
>>>>  nptl/pthread_mutex_lock.c           | 9 +++++----
>>>>  nptl/pthread_mutex_setprioceiling.c | 5 +++--
>>>>  nptl/pthread_mutex_timedlock.c      | 6 +++---
>>>>  4 files changed, 14 insertions(+), 12 deletions(-)
>>>
>>> [snip]
>>>
>>>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>>>> index e643eab258..343acf6107 100644
>>>> --- a/nptl/pthread_mutex_timedlock.c
>>>> +++ b/nptl/pthread_mutex_timedlock.c
>>>> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>>>  			goto failpp;
>>>>  		      }
>>>>  
>>>> -		    lll_futex_timed_wait (&mutex->__data.__lock,
>>>> -					  ceilval | 2, &rt,
>>>> -					  PTHREAD_MUTEX_PSHARED (mutex));
>>>> +		    __futex_abstimed_wait64 (
>>>> +		      (unsigned int *) &mutex->__data.__lock, clockid,
>>>> +		      ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
>>>
>>> I think you've replaced the lll_futex_timed_wait call that expects a
>>> relative timeout with a __futex_abstimed_wait64 call that expects an
>>> absolute timeout, yet you still appear to be passing the relative timeout.
>>>
>>> However, it turns out that the implementation for the
>>> PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
>>> completely broken with clockid != CLOCK_REALTIME ever since I added it in
>>> 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
>>> is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
>>> time this was a less obvious __gettimeofday call.)
>>>
>>> I'll work on writing some test cases for the those types of mutex in the
>>> hope of catching both flaws before fixing them.
>>
>> Indeed, there is no need to calculate the relative timeout anymore. I think
>> the fix below should pass the absolute timeout directly.   I will check
>> a possible regression tests as well.
> 
> OK. I won't then. Thanks.
> 
>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>> index aaaafa21ce..86c5f4446e 100644
>> --- a/nptl/pthread_mutex_timedlock.c
>> +++ b/nptl/pthread_mutex_timedlock.c
>> @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>  	    if (__pthread_current_priority () > ceiling)
>>  	      {
>>  		result = EINVAL;
>> -	      failpp:
>>  		if (oldprio != -1)
>>  		  __pthread_tpp_change_priority (oldprio, -1);
>>  		return result;
>> @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>  
>>  		if (oldval != ceilval)
>>  		  {
>> -		    /* Reject invalid timeouts.  */
>> -		    if (! valid_nanoseconds (abstime->tv_nsec))
>> -		      {
>> -			result = EINVAL;
>> -			goto failpp;
>> -		      }
> 
> If this is removed then is there a risk of getting into a busy loop if
> someone passes a bogus timespec? (Regardless of the answer, it makes sense
> to ensure that is tested somehow.)

The minimum supported kernel already does the same check on the futex call
(source for Linux 3.2):

2690 SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
2691                 struct timespec __user *, utime, u32 __user *, uaddr2,
2692                 u32, val3)
2693 {
2694         struct timespec ts;
2695         ktime_t t, *tp = NULL;
2696         u32 val2 = 0;
2697         int cmd = op & FUTEX_CMD_MASK;
2698 
2699         if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
2700                       cmd == FUTEX_WAIT_BITSET ||
2701                       cmd == FUTEX_WAIT_REQUEUE_PI)) {
2702                 if (copy_from_user(&ts, utime, sizeof(ts)) != 0)
2703                         return -EFAULT;
2704                 if (!timespec_valid(&ts))
2705                         return -EINVAL;
2706 
2707                 t = timespec_to_ktime(ts);
2708                 if (cmd == FUTEX_WAIT)
2709                         t = ktime_add_safe(ktime_get(), t);
2710                 tp = &t;
2711         }

113 #define timespec_valid(ts) \
114         (((ts)->tv_sec >= 0) && (((unsigned long) (ts)->tv_nsec) < NSEC_PER_SEC))

So it will return EINVAL for bogus timespec.
  

On Wednesday 25 November 2020 at 14:19:37 -0300, Adhemerval Zanella wrote:
> On 25/11/2020 12:46, Mike Crowe wrote:
> > On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote:
> >>
> >>
> >> On 25/11/2020 12:32, Mike Crowe wrote:
> >>> On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
> >>>> The idea is to make NPTL implementation to use on the functions
> >>>> provided by futex-internal.h.
> >>>>
> >>>> Checked on x86_64-linux-gnu and i686-linux-gnu.
> >>>> ---
> >>>>  nptl/lowlevellock.c                 | 6 +++---
> >>>>  nptl/pthread_mutex_lock.c           | 9 +++++----
> >>>>  nptl/pthread_mutex_setprioceiling.c | 5 +++--
> >>>>  nptl/pthread_mutex_timedlock.c      | 6 +++---
> >>>>  4 files changed, 14 insertions(+), 12 deletions(-)
> >>>
> >>> [snip]
> >>>
> >>>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> >>>> index e643eab258..343acf6107 100644
> >>>> --- a/nptl/pthread_mutex_timedlock.c
> >>>> +++ b/nptl/pthread_mutex_timedlock.c
> >>>> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
> >>>>  			goto failpp;
> >>>>  		      }
> >>>>  
> >>>> -		    lll_futex_timed_wait (&mutex->__data.__lock,
> >>>> -					  ceilval | 2, &rt,
> >>>> -					  PTHREAD_MUTEX_PSHARED (mutex));
> >>>> +		    __futex_abstimed_wait64 (
> >>>> +		      (unsigned int *) &mutex->__data.__lock, clockid,
> >>>> +		      ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
> >>>
> >>> I think you've replaced the lll_futex_timed_wait call that expects a
> >>> relative timeout with a __futex_abstimed_wait64 call that expects an
> >>> absolute timeout, yet you still appear to be passing the relative timeout.
> >>>
> >>> However, it turns out that the implementation for the
> >>> PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
> >>> completely broken with clockid != CLOCK_REALTIME ever since I added it in
> >>> 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
> >>> is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
> >>> time this was a less obvious __gettimeofday call.)
> >>>
> >>> I'll work on writing some test cases for the those types of mutex in the
> >>> hope of catching both flaws before fixing them.
> >>
> >> Indeed, there is no need to calculate the relative timeout anymore. I think
> >> the fix below should pass the absolute timeout directly.   I will check
> >> a possible regression tests as well.
> > 
> > OK. I won't then. Thanks.
> > 
> >> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> >> index aaaafa21ce..86c5f4446e 100644
> >> --- a/nptl/pthread_mutex_timedlock.c
> >> +++ b/nptl/pthread_mutex_timedlock.c
> >> @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
> >>  	    if (__pthread_current_priority () > ceiling)
> >>  	      {
> >>  		result = EINVAL;
> >> -	      failpp:
> >>  		if (oldprio != -1)
> >>  		  __pthread_tpp_change_priority (oldprio, -1);
> >>  		return result;
> >> @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
> >>  
> >>  		if (oldval != ceilval)
> >>  		  {
> >> -		    /* Reject invalid timeouts.  */
> >> -		    if (! valid_nanoseconds (abstime->tv_nsec))
> >> -		      {
> >> -			result = EINVAL;
> >> -			goto failpp;
> >> -		      }
> > 
> > If this is removed then is there a risk of getting into a busy loop if
> > someone passes a bogus timespec? (Regardless of the answer, it makes sense
> > to ensure that is tested somehow.)
> 
> The minimum supported kernel already does the same check on the futex call
> (source for Linux 3.2):
> 
> 2690 SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
> 2691                 struct timespec __user *, utime, u32 __user *, uaddr2,
> 2692                 u32, val3)
> 2693 {
> 2694         struct timespec ts;
> 2695         ktime_t t, *tp = NULL;
> 2696         u32 val2 = 0;
> 2697         int cmd = op & FUTEX_CMD_MASK;
> 2698 
> 2699         if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
> 2700                       cmd == FUTEX_WAIT_BITSET ||
> 2701                       cmd == FUTEX_WAIT_REQUEUE_PI)) {
> 2702                 if (copy_from_user(&ts, utime, sizeof(ts)) != 0)
> 2703                         return -EFAULT;
> 2704                 if (!timespec_valid(&ts))
> 2705                         return -EINVAL;
> 2706 
> 2707                 t = timespec_to_ktime(ts);
> 2708                 if (cmd == FUTEX_WAIT)
> 2709                         t = ktime_add_safe(ktime_get(), t);
> 2710                 tp = &t;
> 2711         }
> 
> 113 #define timespec_valid(ts) \
> 114         (((ts)->tv_sec >= 0) && (((unsigned long) (ts)->tv_nsec) < NSEC_PER_SEC))
> 
> So it will return EINVAL for bogus timespec.

Yes, but here:

>                     __futex_abstimed_wait64 (
>                       (unsigned int *) &mutex->__data.__lock, clockid,
> -                     ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
> +                     ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex));

the return value of __futex_abstimed_wait64 is not checked, so the loop
might just spin around busily until the timeout expires. Perhaps the return
value needs checking too?

Thanks.

Mike.
  

On 25/11/2020 14:37, Mike Crowe wrote:
> On Wednesday 25 November 2020 at 14:19:37 -0300, Adhemerval Zanella wrote:
>> On 25/11/2020 12:46, Mike Crowe wrote:
>>> On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote:
>>>>
>>>>
>>>> On 25/11/2020 12:32, Mike Crowe wrote:
>>>>> On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
>>>>>> The idea is to make NPTL implementation to use on the functions
>>>>>> provided by futex-internal.h.
>>>>>>
>>>>>> Checked on x86_64-linux-gnu and i686-linux-gnu.
>>>>>> ---
>>>>>>  nptl/lowlevellock.c                 | 6 +++---
>>>>>>  nptl/pthread_mutex_lock.c           | 9 +++++----
>>>>>>  nptl/pthread_mutex_setprioceiling.c | 5 +++--
>>>>>>  nptl/pthread_mutex_timedlock.c      | 6 +++---
>>>>>>  4 files changed, 14 insertions(+), 12 deletions(-)
>>>>>
>>>>> [snip]
>>>>>
>>>>>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>>>>>> index e643eab258..343acf6107 100644
>>>>>> --- a/nptl/pthread_mutex_timedlock.c
>>>>>> +++ b/nptl/pthread_mutex_timedlock.c
>>>>>> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>>>>>  			goto failpp;
>>>>>>  		      }
>>>>>>  
>>>>>> -		    lll_futex_timed_wait (&mutex->__data.__lock,
>>>>>> -					  ceilval | 2, &rt,
>>>>>> -					  PTHREAD_MUTEX_PSHARED (mutex));
>>>>>> +		    __futex_abstimed_wait64 (
>>>>>> +		      (unsigned int *) &mutex->__data.__lock, clockid,
>>>>>> +		      ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
>>>>>
>>>>> I think you've replaced the lll_futex_timed_wait call that expects a
>>>>> relative timeout with a __futex_abstimed_wait64 call that expects an
>>>>> absolute timeout, yet you still appear to be passing the relative timeout.
>>>>>
>>>>> However, it turns out that the implementation for the
>>>>> PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
>>>>> completely broken with clockid != CLOCK_REALTIME ever since I added it in
>>>>> 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
>>>>> is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
>>>>> time this was a less obvious __gettimeofday call.)
>>>>>
>>>>> I'll work on writing some test cases for the those types of mutex in the
>>>>> hope of catching both flaws before fixing them.
>>>>
>>>> Indeed, there is no need to calculate the relative timeout anymore. I think
>>>> the fix below should pass the absolute timeout directly.   I will check
>>>> a possible regression tests as well.
>>>
>>> OK. I won't then. Thanks.
>>>
>>>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>>>> index aaaafa21ce..86c5f4446e 100644
>>>> --- a/nptl/pthread_mutex_timedlock.c
>>>> +++ b/nptl/pthread_mutex_timedlock.c
>>>> @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>>>  	    if (__pthread_current_priority () > ceiling)
>>>>  	      {
>>>>  		result = EINVAL;
>>>> -	      failpp:
>>>>  		if (oldprio != -1)
>>>>  		  __pthread_tpp_change_priority (oldprio, -1);
>>>>  		return result;
>>>> @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>>>  
>>>>  		if (oldval != ceilval)
>>>>  		  {
>>>> -		    /* Reject invalid timeouts.  */
>>>> -		    if (! valid_nanoseconds (abstime->tv_nsec))
>>>> -		      {
>>>> -			result = EINVAL;
>>>> -			goto failpp;
>>>> -		      }
>>>
>>> If this is removed then is there a risk of getting into a busy loop if
>>> someone passes a bogus timespec? (Regardless of the answer, it makes sense
>>> to ensure that is tested somehow.)
>>
>> The minimum supported kernel already does the same check on the futex call
>> (source for Linux 3.2):
>>
>> 2690 SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
>> 2691                 struct timespec __user *, utime, u32 __user *, uaddr2,
>> 2692                 u32, val3)
>> 2693 {
>> 2694         struct timespec ts;
>> 2695         ktime_t t, *tp = NULL;
>> 2696         u32 val2 = 0;
>> 2697         int cmd = op & FUTEX_CMD_MASK;
>> 2698 
>> 2699         if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
>> 2700                       cmd == FUTEX_WAIT_BITSET ||
>> 2701                       cmd == FUTEX_WAIT_REQUEUE_PI)) {
>> 2702                 if (copy_from_user(&ts, utime, sizeof(ts)) != 0)
>> 2703                         return -EFAULT;
>> 2704                 if (!timespec_valid(&ts))
>> 2705                         return -EINVAL;
>> 2706 
>> 2707                 t = timespec_to_ktime(ts);
>> 2708                 if (cmd == FUTEX_WAIT)
>> 2709                         t = ktime_add_safe(ktime_get(), t);
>> 2710                 tp = &t;
>> 2711         }
>>
>> 113 #define timespec_valid(ts) \
>> 114         (((ts)->tv_sec >= 0) && (((unsigned long) (ts)->tv_nsec) < NSEC_PER_SEC))
>>
>> So it will return EINVAL for bogus timespec.
> 
> Yes, but here:
> 
>>                     __futex_abstimed_wait64 (
>>                       (unsigned int *) &mutex->__data.__lock, clockid,
>> -                     ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
>> +                     ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex));
> 
> the return value of __futex_abstimed_wait64 is not checked, so the loop
> might just spin around busily until the timeout expires. Perhaps the return
> value needs checking too?

Indeed, we need to check for ETIMEDOUT/EOVERFLOW.