diff mbox series

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

Message ID 20201123195256.3336217-9-adhemerval.zanella@linaro.org
State Committed
Headers show
Series [01/13] linux: Remove unused internal futex functions | expand

Commit Message

Adhemerval Zanella Nov. 23, 2020, 7:52 p.m. UTC
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(-)

Comments

Lukasz Majewski Nov. 24, 2020, 9:35 p.m. UTC | #1
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
Mike Crowe Nov. 25, 2020, 3:32 p.m. UTC | #2
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.
Adhemerval Zanella Nov. 25, 2020, 3:40 p.m. UTC | #3
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,
Mike Crowe Nov. 25, 2020, 3:46 p.m. UTC | #4
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.
Adhemerval Zanella Nov. 25, 2020, 5:19 p.m. UTC | #5
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.
Mike Crowe Nov. 25, 2020, 5:37 p.m. UTC | #6
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.
Adhemerval Zanella Nov. 25, 2020, 5:56 p.m. UTC | #7
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.
diff mbox series

Patch

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,