This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] [BZ #21778] Fix oversight in robust mutex lock acquisition.
- From: Siddhesh Poyarekar <siddhesh at gotplt dot org>
- To: Torvald Riegel <triegel at redhat dot com>, GLIBC Devel <libc-alpha at sourceware dot org>
- Cc: Carlos O'Donell <codonell at redhat dot com>
- Date: Fri, 28 Jul 2017 18:11:11 +0530
- Subject: Re: [PATCH] [BZ #21778] Fix oversight in robust mutex lock acquisition.
- Authentication-results: sourceware.org; auth=none
- References: <1501018343.30433.50.camel@redhat.com>
On Wednesday 26 July 2017 03:02 AM, Torvald Riegel wrote:
> 65810f0ef05e8c9e333f17a44e77808b163ca298 fixed a robust mutex bug but
> introduced BZ 21778: if the CAS used to try to acquire a lock fails, the
> expected value is not updated, which breaks other cases in the lock
> acquisition loop. The fix is to simply update the expected value with
> the value returned by the CAS, which ensures that behavior is as if the
> first case with the CAS never happened (if the CAS fails).
>
> This is a regression introduced in the last release, so it would be good
> to get this included in this release. I'll likely be AFK on Thursday,
> so please just commit this once it has been approved. Tested on
> x86_64-linux.
>
>
> [BZ 21778]
> * nptl/pthread_mutex_timedlock.c (__pthread_mutex_timedlock): Update
> oldval if the CAS fails.
> * nptl/pthread_mutex_lock.c (__pthread_mutex_lock_full): Likewise.
> * nptl/tst-mutex7.c (ROBUST, DELAY_NSEC, ROUNDS, N): New.
> (tf, do_test): Use them.
> * nptl/tst-mutex7robust.c: New file.
> * nptl/Makefile (tests): Add new test.
>
Looks good to me, but I think Carlos is also reviewing this, so please
wait for his confirmation before you commit.
Siddhesh
>
> robust-mutex-21778.patch
>
>
> commit 77e7d626fa6d7f8800c9658e65e43d13c5cdc81d
> Author: Torvald Riegel <triegel@redhat.com>
> Date: Mon Jul 24 22:53:38 2017 +0200
>
> Fix oversight in robust mutex lock acquisition.
>
> 65810f0ef05e8c9e333f17a44e77808b163ca298 fixed a robust mutex bug but
> introduced BZ 21778: if the CAS used to try to acquire a lock fails,
> the expected value is not updated, which breaks other cases in the lock
> acquisition loop. The fix is to simply update the expected value with
> the value returned by the CAS, which ensures that behavior is as if the
> first case with the CAS never happened.
>
> 2017-07-25 Torvald Riegel <triegel@redhat.com>
>
> [BZ 21778]
> * nptl/pthread_mutex_timedlock.c (__pthread_mutex_timedlock): Update
> oldval if the CAS fails.
> * nptl/pthread_mutex_lock.c (__pthread_mutex_lock_full): Likewise.
> * nptl/tst-mutex7.c (ROBUST, DELAY_NSEC, ROUNDS, N): New.
> (tf, do_test): Use them.
> * nptl/tst-mutex7robust.c: New file.
> * nptl/Makefile (tests): Add new test.
>
> diff --git a/nptl/Makefile b/nptl/Makefile
> index dd01994..bca09bf 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -230,7 +230,7 @@ LDLIBS-tst-thread_local1 = -lstdc++
>
> tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
> tst-mutex1 tst-mutex2 tst-mutex3 tst-mutex4 tst-mutex5 tst-mutex6 \
> - tst-mutex7 tst-mutex9 tst-mutex5a tst-mutex7a \
> + tst-mutex7 tst-mutex9 tst-mutex5a tst-mutex7a tst-mutex7robust \
> tst-mutexpi1 tst-mutexpi2 tst-mutexpi3 tst-mutexpi4 tst-mutexpi5 \
> tst-mutexpi5a tst-mutexpi6 tst-mutexpi7 tst-mutexpi7a \
> tst-mutexpi9 \
> diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
> index b76475b..8c48503 100644
> --- a/nptl/pthread_mutex_lock.c
> +++ b/nptl/pthread_mutex_lock.c
> @@ -197,11 +197,14 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
> {
> /* Try to acquire the lock through a CAS from 0 (not acquired) to
> our TID | assume_other_futex_waiters. */
> - if (__glibc_likely ((oldval == 0)
> - && (atomic_compare_and_exchange_bool_acq
> - (&mutex->__data.__lock,
> - id | assume_other_futex_waiters, 0) == 0)))
> - break;
> + if (__glibc_likely (oldval == 0))
> + {
> + oldval
> + = atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,
> + id | assume_other_futex_waiters, 0);
> + if (__glibc_likely (oldval == 0))
> + break;
> + }
>
> if ((oldval & FUTEX_OWNER_DIED) != 0)
> {
> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> index be53381..d5ec314 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -154,11 +154,14 @@ __pthread_mutex_timedlock (pthread_mutex_t *mutex,
> {
> /* Try to acquire the lock through a CAS from 0 (not acquired) to
> our TID | assume_other_futex_waiters. */
> - if (__glibc_likely ((oldval == 0)
> - && (atomic_compare_and_exchange_bool_acq
> - (&mutex->__data.__lock,
> - id | assume_other_futex_waiters, 0) == 0)))
> - break;
> + if (__glibc_likely (oldval == 0))
> + {
> + oldval
> + = atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,
> + id | assume_other_futex_waiters, 0);
> + if (__glibc_likely (oldval == 0))
> + break;
> + }
>
> if ((oldval & FUTEX_OWNER_DIED) != 0)
> {
> diff --git a/nptl/tst-mutex7.c b/nptl/tst-mutex7.c
> index a11afdb..7990409 100644
> --- a/nptl/tst-mutex7.c
> +++ b/nptl/tst-mutex7.c
> @@ -26,13 +26,22 @@
> #ifndef TYPE
> # define TYPE PTHREAD_MUTEX_DEFAULT
> #endif
> -
> +#ifndef ROBUST
> +# define ROBUST PTHREAD_MUTEX_STALLED
> +#endif
> +#ifndef DELAY_NSEC
> +# define DELAY_NSEC 11000
> +#endif
> +#ifndef ROUNDS
> +# define ROUNDS 1000
> +#endif
> +#ifndef N
> +# define N 100
> +#endif
>
> static pthread_mutex_t lock;
>
>
> -#define ROUNDS 1000
> -#define N 100
>
>
> static void *
> @@ -40,7 +49,7 @@ tf (void *arg)
> {
> int nr = (long int) arg;
> int cnt;
> - struct timespec ts = { .tv_sec = 0, .tv_nsec = 11000 };
> + struct timespec ts = { .tv_sec = 0, .tv_nsec = DELAY_NSEC };
>
> for (cnt = 0; cnt < ROUNDS; ++cnt)
> {
> @@ -56,7 +65,8 @@ tf (void *arg)
> return (void *) 1l;
> }
>
> - nanosleep (&ts, NULL);
> + if ((ts.tv_sec > 0) || (ts.tv_nsec > 0))
> + nanosleep (&ts, NULL);
> }
>
> return NULL;
> @@ -80,6 +90,12 @@ do_test (void)
> exit (1);
> }
>
> + if (pthread_mutexattr_setrobust (&a, ROBUST) != 0)
> + {
> + puts ("mutexattr_setrobust failed");
> + exit (1);
> + }
> +
> #ifdef ENABLE_PI
> if (pthread_mutexattr_setprotocol (&a, PTHREAD_PRIO_INHERIT) != 0)
> {
> diff --git a/nptl/tst-mutex7robust.c b/nptl/tst-mutex7robust.c
> new file mode 100644
> index 0000000..73eb66d
> --- /dev/null
> +++ b/nptl/tst-mutex7robust.c
> @@ -0,0 +1,6 @@
> +#define TYPE PTHREAD_MUTEX_NORMAL
> +#define ROBUST PTHREAD_MUTEX_ROBUST
> +#define DELAY_NSEC 0
> +#define ROUNDS 1000
> +#define N 32
> +#include "tst-mutex7.c"
>