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

[Adhemerval Zanella]

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.