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

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

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.