Re: [PATCH] nptl: Fix PTHREAD_PRIO_PROTECT timed lock

[Mike Crowe <mac@mcrowe.com>]

On Wednesday 25 November 2020 at 17:27:23 -0300, Adhemerval Zanella wrote:
> The 878fe624d4 changed lll_futex_timed_wait, which expects a relative
> timeout, with a __futex_abstimed_wait64, which expects an absolute
> timeout.  However the code still passes a relative timeout.
> 
> Also, the PTHREAD_PRIO_PROTECT support for clocks different than
> CLOCK_REALTIME was broken since the inclusion of
> pthread_mutex_clocklock (9d20e22e46) since lll_futex_timed_wait
> always use CLOCK_REALTIME.
> 
> This patch fixes by removing the relative time calculation.  It
> also adds some xtests that tests both thread and inter-process
> usage.
> 
> Checked on x86_64-linux-gnu.
> ---
>  nptl/Makefile                  |  3 ++-
>  nptl/pthread_mutex_timedlock.c | 29 +++++------------------------
>  nptl/tst-mutexpp5.c            |  2 ++
>  nptl/tst-mutexpp9.c            |  2 ++
>  sysdeps/pthread/tst-mutex5.c   | 12 +++++++++++-
>  sysdeps/pthread/tst-mutex9.c   | 13 ++++++++++++-
>  6 files changed, 34 insertions(+), 27 deletions(-)
>  create mode 100644 nptl/tst-mutexpp5.c
>  create mode 100644 nptl/tst-mutexpp9.c
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index a48426a396..94d805f0d4 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -309,7 +309,8 @@ tests-internal := tst-robustpi8 tst-rwlock19 tst-rwlock20 \
>  		  tst-setgetname \
>  
>  xtests = tst-setuid1 tst-setuid1-static tst-setuid2 \
> -	tst-mutexpp1 tst-mutexpp6 tst-mutexpp10 tst-setgroups
> +	tst-mutexpp1 tst-mutexpp6 tst-mutexpp10 tst-setgroups \
> +	tst-mutexpp5 tst-mutexpp9
>  
>  # This test can run into task limits because of a linux kernel bug
>  # and then cause the make process to fail too, see bug 24537.
> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> index aaaafa21ce..74adffe790 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -547,30 +547,11 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>  			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));
> +		    int e = __futex_abstimed_wait64 (
> +		      (unsigned int *) &mutex->__data.__lock, ceilval | 2,
> +		      clockid, abstime, PTHREAD_MUTEX_PSHARED (mutex));
> +		    if (e == ETIMEDOUT)
> +		      return ETIMEDOUT;

I'm worried that futex could return other errors here which would cause a
busy infinite loop. However, my attempts to provoke EINVAL have failed
since the validity of abstime.tv_nsec is checked earlier. Presumably we
should never get this far if EOVERFLOW could be returned? (If there is a
problem here, then it also affects other mutex types which have similar
code.)

>  		  }
>  	      }
>  	    while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,
> diff --git a/nptl/tst-mutexpp5.c b/nptl/tst-mutexpp5.c
> new file mode 100644
> index 0000000000..a864a390ca
> --- /dev/null
> +++ b/nptl/tst-mutexpp5.c
> @@ -0,0 +1,2 @@
> +#define ENABLE_PP 1
> +#include "tst-mutex5.c"
> diff --git a/nptl/tst-mutexpp9.c b/nptl/tst-mutexpp9.c
> new file mode 100644
> index 0000000000..c848c74c7e
> --- /dev/null
> +++ b/nptl/tst-mutexpp9.c
> @@ -0,0 +1,2 @@
> +#define ENABLE_PP 1
> +#include "tst-mutex9.c"
> diff --git a/sysdeps/pthread/tst-mutex5.c b/sysdeps/pthread/tst-mutex5.c
> index bfe1a79fa4..dbd2c3c15f 100644
> --- a/sysdeps/pthread/tst-mutex5.c
> +++ b/sysdeps/pthread/tst-mutex5.c
> @@ -27,6 +27,9 @@
>  #include <support/check.h>
>  #include <support/timespec.h>
>  
> +#ifdef ENABLE_PP
> +#include "tst-tpp.h"
> +#endif
>  
>  #ifndef TYPE
>  # define TYPE PTHREAD_MUTEX_NORMAL
> @@ -47,8 +50,11 @@ do_test_clock (clockid_t clockid, const char *fnname)
>    TEST_COMPARE (pthread_mutexattr_init (&a), 0);
>    TEST_COMPARE (pthread_mutexattr_settype (&a, TYPE), 0);
>  
> -#ifdef ENABLE_PI
> +#if defined ENABLE_PI
>    TEST_COMPARE (pthread_mutexattr_setprotocol (&a, PTHREAD_PRIO_INHERIT), 0);
> +#elif defined ENABLE_PP
> +  TEST_COMPARE (pthread_mutexattr_setprotocol (&a, PTHREAD_PRIO_PROTECT), 0);
> +  TEST_COMPARE (pthread_mutexattr_setprioceiling (&a, 6), 0);
>  #endif
>  
>    int err = pthread_mutex_init (&m, &a);
> @@ -110,6 +116,10 @@ do_test_clock (clockid_t clockid, const char *fnname)
>  
>  static int do_test (void)
>  {
> +#ifdef ENABLE_PP
> +  init_tpp_test ();
> +#endif
> +
>    do_test_clock (CLOCK_USE_TIMEDLOCK, "timedlock");
>    do_test_clock (CLOCK_REALTIME, "clocklock(realtime)");
>  #ifndef ENABLE_PI
> diff --git a/sysdeps/pthread/tst-mutex9.c b/sysdeps/pthread/tst-mutex9.c
> index bfc01f8c75..081aeff0f6 100644
> --- a/sysdeps/pthread/tst-mutex9.c
> +++ b/sysdeps/pthread/tst-mutex9.c
> @@ -30,6 +30,10 @@
>  #include <support/timespec.h>
>  #include <support/xunistd.h>
>  
> +#ifdef ENABLE_PP
> +#include "tst-tpp.h"
> +#endif
> +
>  
>  /* A bogus clock value that tells run_test to use pthread_mutex_timedlock
>     rather than pthread_mutex_clocklock.  */
> @@ -73,8 +77,11 @@ do_test_clock (clockid_t clockid)
>  
>    TEST_COMPARE (pthread_mutexattr_settype (&a, PTHREAD_MUTEX_RECURSIVE), 0);
>  
> -#ifdef ENABLE_PI
> +#if defined ENABLE_PI
>    TEST_COMPARE (pthread_mutexattr_setprotocol (&a, PTHREAD_PRIO_INHERIT), 0);
> +#elif defined ENABLE_PP
> +  TEST_COMPARE (pthread_mutexattr_setprotocol (&a, PTHREAD_PRIO_PROTECT), 0);
> +  TEST_COMPARE (pthread_mutexattr_setprioceiling (&a, 6), 0);
>  #endif
>  
>    int e;
> @@ -131,6 +138,10 @@ do_test_clock (clockid_t clockid)
>  static int
>  do_test (void)
>  {
> +#ifdef ENABLE_PP
> +  init_tpp_test ();
> +#endif
> +
>    do_test_clock (CLOCK_USE_TIMEDLOCK);
>    do_test_clock (CLOCK_REALTIME);
>  #ifndef ENABLE_PI

LGTM. I have some extra timespec tests that I'll propose in a separate
patch once this lands.

Thanks.

Mike.