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

[Mike Crowe <mac@mcrowe.com>]

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.