[PATCH v2 1/2] Fix robust mutex daedlock [BZ #20263]

[PATCH v2 1/2] Fix robust mutex daedlock [BZ #20263]

[Jiyoung Yun]

In Linux/ARM environment, a robust mutex can't catch the timeout result
when it is already owned by other thread and requests to try lock with
a specific time value(pthread_mutex_timedlock). The futex already returns
the ETIMEDOUT result but there is no check the return value and it makes
a deadlock.

* nptl/lowlevelrobustlock.c: Implement ETIMEDOUT logic.
---
 ChangeLog                 | 5 +++++
 nptl/lowlevelrobustlock.c | 9 +++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 3da2eca..97461f6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2016-06-28  Jiyoung Yun  <t2wish@gmail.com>
+
+	[BZ #20263]
+	* nptl/lowlevelrobustlock.c: Implement ETIMEDOUT logic.
+
 2016-06-28  Richard Henderson  <rth@redhat.com>
 
 	* elf/elf.h (EM_BPF): New.
diff --git a/nptl/lowlevelrobustlock.c b/nptl/lowlevelrobustlock.c
index 3b988b2..e5d3758 100644
--- a/nptl/lowlevelrobustlock.c
+++ b/nptl/lowlevelrobustlock.c
@@ -113,15 +113,20 @@ __lll_robust_timedlock_wait (int *futex, const struct timespec *abstime,
 	  && atomic_compare_and_exchange_bool_acq (futex, newval, oldval))
 	continue;
 
+      int err;
       /* If *futex == 2, wait until woken or timeout.  */
 #if (!defined __ASSUME_FUTEX_CLOCK_REALTIME \
      || !defined lll_futex_timed_wait_bitset)
-      lll_futex_timed_wait (futex, newval, &rt, private);
+      err = lll_futex_timed_wait (futex, newval, &rt, private);
 #else
-      lll_futex_timed_wait_bitset (futex, newval, abstime,
+      err = lll_futex_timed_wait_bitset (futex, newval, abstime,
 				   FUTEX_CLOCK_REALTIME, private);
 #endif
 
+      /* the futex call time out */
+      if (err == -ETIMEDOUT)
+         return -err;
+
     try:
       ;
     }
-- 
1.9.1

[PATCH v2 0/2] Fix robust mutex daedlock [BZ #20263]

[Jiyoung Yun]

This patch is related to commit 8f86154.
The commit did not support to check the return value for timeout from lll_futex_timed_wait_bitset. It makes a daedlock when a robust mutex is already owned by other thread and requests to try lock it within a specific time.

Andreas Schwab (1):
  Add tst-robust10.c tests

Jiyoung Yun (1):
  Fix robust mutex daedlock [BZ #20263]

 ChangeLog                 |  11 +++++
 nptl/Makefile             |   2 +-
 nptl/lowlevelrobustlock.c |   9 +++-
 nptl/tst-robust10.c       | 110 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 129 insertions(+), 3 deletions(-)
 create mode 100644 nptl/tst-robust10.c

-- 
1.9.1

[PATCH v2 2/2] Add tst-robust10.c tests

[Jiyoung Yun]

From: Andreas Schwab <schwab@suse.de>

[BZ #20263]
* nptl/Makefile: Added new tests.
* nptl/tst-robust10.c: New.
---
 ChangeLog           |   6 +++
 nptl/Makefile       |   2 +-
 nptl/tst-robust10.c | 110 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+), 1 deletion(-)
 create mode 100644 nptl/tst-robust10.c

diff --git a/ChangeLog b/ChangeLog
index 97461f6..84cda4e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2016-06-28  Andreas Schwab <schwab@suse.de>
+
+	[BZ #20263]
+	* nptl/Makefile: Added new tests.
+	* nptl/tst-robust10.c: New.
+
 2016-06-28  Jiyoung Yun  <t2wish@gmail.com>
 
 	[BZ #20263]
diff --git a/nptl/Makefile b/nptl/Makefile
index e0bc1b7..28f6d5a 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -290,7 +290,7 @@ tests = tst-typesizes \
 	tst-initializers1 $(addprefix tst-initializers1-,\
 			    c89 gnu89 c99 gnu99 c11 gnu11) \
 	tst-bad-schedattr \
-	tst-thread_local1 tst-mutex-errorcheck
+	tst-thread_local1 tst-mutex-errorcheck tst-robust10
 xtests = tst-setuid1 tst-setuid1-static tst-setuid2 \
 	tst-mutexpp1 tst-mutexpp6 tst-mutexpp10
 test-srcs = tst-oddstacklimit
diff --git a/nptl/tst-robust10.c b/nptl/tst-robust10.c
new file mode 100644
index 0000000..d510ee1
--- /dev/null
+++ b/nptl/tst-robust10.c
@@ -0,0 +1,110 @@
+/* Test that pthread_mutex_timedlock properly times out.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <pthread.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+
+pthread_mutex_t mutex;
+
+static void *
+thr (void *arg)
+{
+  struct timespec abstime;
+  clock_gettime (CLOCK_REALTIME, &abstime);
+  abstime.tv_sec = 1;
+  int ret = pthread_mutex_timedlock (&mutex, &abstime);
+  if (ret == 0)
+    {
+      puts ("mutex_timedlock didn't fail");
+      exit (1);
+    }
+  if (ret != ETIMEDOUT)
+    {
+      printf ("mutex_timedlock failed: %s\n", strerror (ret));
+      exit (1);
+    }
+
+  return 0;
+}
+
+static int
+do_test (void)
+{
+  pthread_t pt;
+  pthread_mutexattr_t ma;
+
+  if (pthread_mutexattr_init (&ma) != 0)
+    {
+      puts ("mutexattr_init failed");
+      return 0;
+    }
+  if (pthread_mutexattr_setrobust_np (&ma, PTHREAD_MUTEX_ROBUST_NP) != 0)
+    {
+      puts ("mutexattr_setrobust failed");
+      return 1;
+    }
+  if (pthread_mutex_init (&mutex, &ma))
+    {
+      puts ("mutex_init failed");
+      return 1;
+    }
+
+  if (pthread_mutexattr_destroy (&ma))
+    {
+      puts ("mutexattr_destroy failed");
+      return 1;
+    }
+
+  if (pthread_mutex_lock (&mutex))
+    {
+      puts ("mutex_lock failed");
+      return 1;
+    }
+
+  if (pthread_create (&pt, NULL, thr, NULL))
+    {
+      puts ("pthread_create failed");
+      return 1;
+    }
+
+  if (pthread_join (pt, NULL))
+    {
+      puts ("pthread_join failed");
+      return 1;
+    }
+
+  if (pthread_mutex_unlock (&mutex))
+    {
+      puts ("mutex_unlock failed");
+      return 1;
+    }
+
+  if (pthread_mutex_destroy (&mutex))
+    {
+      puts ("mutex_destroy failed");
+      return 1;
+    }
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
-- 
1.9.1

Re: [PATCH v2 1/2] Fix robust mutex daedlock [BZ #20263]

[Andreas Schwab]

Jiyoung Yun <t2wish@gmail.com> writes:

> diff --git a/nptl/lowlevelrobustlock.c b/nptl/lowlevelrobustlock.c
> index 3b988b2..e5d3758 100644
> --- a/nptl/lowlevelrobustlock.c
> +++ b/nptl/lowlevelrobustlock.c
> @@ -113,15 +113,20 @@ __lll_robust_timedlock_wait (int *futex, const struct timespec *abstime,
>  	  && atomic_compare_and_exchange_bool_acq (futex, newval, oldval))
>  	continue;
>  
> +      int err;
>        /* If *futex == 2, wait until woken or timeout.  */
>  #if (!defined __ASSUME_FUTEX_CLOCK_REALTIME \
>       || !defined lll_futex_timed_wait_bitset)
> -      lll_futex_timed_wait (futex, newval, &rt, private);
> +      err = lll_futex_timed_wait (futex, newval, &rt, private);

I don't think this call should be checked for timeout because we want to
check the absolute time, not the relative time we computed here.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v2 1/2] Fix robust mutex daedlock [BZ #20263]

[Andreas Schwab]

Jiyoung Yun <t2wish@gmail.com> writes:

> And the same problem can be occured in case of the relative time too,
> doesn't it?

No, it doesn't, see the top of the loop.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

[PATCH] Fix robust mutex daedlock [BZ #20263]

[Jiyoung Yun]

In Linux/ARM environment, a robust mutex can't catch the timeout result
when it is already owned by other thread and requests to try lock with
a specific time value(pthread_mutex_timedlock). The futex already returns
the ETIMEDOUT result but there is no check the return value and it makes
a deadlock.

* nptl/lowlevelrobustlock.c: Implement ETIMEDOUT logic.
---
 ChangeLog                 | 5 +++++
 nptl/lowlevelrobustlock.c | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index 3da2eca..97461f6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2016-06-28  Jiyoung Yun  <t2wish@gmail.com>
+
+	[BZ #20263]
+	* nptl/lowlevelrobustlock.c: Implement ETIMEDOUT logic.
+
 2016-06-28  Richard Henderson  <rth@redhat.com>
 
 	* elf/elf.h (EM_BPF): New.
diff --git a/nptl/lowlevelrobustlock.c b/nptl/lowlevelrobustlock.c
index 3b988b2..43caae7 100644
--- a/nptl/lowlevelrobustlock.c
+++ b/nptl/lowlevelrobustlock.c
@@ -118,8 +118,11 @@ __lll_robust_timedlock_wait (int *futex, const struct timespec *abstime,
      || !defined lll_futex_timed_wait_bitset)
       lll_futex_timed_wait (futex, newval, &rt, private);
 #else
-      lll_futex_timed_wait_bitset (futex, newval, abstime,
+      int err = lll_futex_timed_wait_bitset (futex, newval, abstime,
 				   FUTEX_CLOCK_REALTIME, private);
+      /* the futex call time out */
+      if (err == -ETIMEDOUT)
+         return -err;
 #endif
 
     try:
-- 
1.9.1

Re: [PATCH] Fix robust mutex daedlock [BZ #20263]

[Andreas Schwab]

I have installed that now.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix robust mutex daedlock [BZ #20263]

[Torvald Riegel]

On Fri, 2016-06-24 at 04:26 +0900, Jiyoung Yun wrote:
> In Linux/ARM environment, a robust mutex can't catch the timeout result
> when it is already owned by other thread and requests to try lock with
> a specific time value(pthread_mutex_timedlock). The futex already returns
> the ETIMEDOUT result but there is no check the return value and it makes
> a deadlock.
> 
> * nptl/lowlevelrobustlock.c: Implement ETIMEDOUT logic.

This touches the same code Andreas Schwab had a patch for (but which
touches the other part of the preprocessor conditional).  Can the two of
you create one patch that fixes this bug?

> Signed-off-by: Jiyoung Yun <t2wish@gmail.com>
> ---
>  ChangeLog                 | 5 +++++
>  nptl/lowlevelrobustlock.c | 9 +++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 4bfee94..7b95767 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,8 @@
> +2016-06-24  Jiyoung Yun  <t2wish@gmail.com>
> +
> +	[BZ #20263]
> +	* nptl/lowlevelrobustlock.c: Implement ETIMEDOUT logic.
> +
>  2016-06-23  Florian Weimer  <fweimer@redhat.com>
>  
>  	* test-skeleton.c (xrealloc): Support deallocation with n == 0.
> diff --git a/nptl/lowlevelrobustlock.c b/nptl/lowlevelrobustlock.c
> index 3b988b2..8842032 100644
> --- a/nptl/lowlevelrobustlock.c
> +++ b/nptl/lowlevelrobustlock.c
> @@ -113,15 +113,20 @@ __lll_robust_timedlock_wait (int *futex, const struct timespec *abstime,
>  	  && atomic_compare_and_exchange_bool_acq (futex, newval, oldval))
>  	continue;
>  
> +      int err;
>        /* If *futex == 2, wait until woken or timeout.  */
>  #if (!defined __ASSUME_FUTEX_CLOCK_REALTIME \
>       || !defined lll_futex_timed_wait_bitset)
> -      lll_futex_timed_wait (futex, newval, &rt, private);
> +      err = lll_futex_timed_wait (futex, newval, &rt, private);
>  #else
> -      lll_futex_timed_wait_bitset (futex, newval, abstime,
> +      err = lll_futex_timed_wait_bitset (futex, newval, abstime,
>  				   FUTEX_CLOCK_REALTIME, private);
>  #endif
>  
> +      /* the futex call time out */
> +      if (err == -ETIMEDOUT)
> +         return ETIMEDOUT;
> +
>      try:
>        ;
>      }

[PATCH] Fix robust mutex daedlock [BZ #20263]

[Jiyoung Yun]

In Linux/ARM environment, a robust mutex can't catch the timeout result
when it is already owned by other thread and requests to try lock with
a specific time value(pthread_mutex_timedlock). The futex already returns
the ETIMEDOUT result but there is no check the return value and it makes
a deadlock.

* nptl/lowlevelrobustlock.c: Implement ETIMEDOUT logic.

Signed-off-by: Jiyoung Yun <t2wish@gmail.com>
---
 ChangeLog                 | 5 +++++
 nptl/lowlevelrobustlock.c | 9 +++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 4bfee94..7b95767 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2016-06-24  Jiyoung Yun  <t2wish@gmail.com>
+
+	[BZ #20263]
+	* nptl/lowlevelrobustlock.c: Implement ETIMEDOUT logic.
+
 2016-06-23  Florian Weimer  <fweimer@redhat.com>
 
 	* test-skeleton.c (xrealloc): Support deallocation with n == 0.
diff --git a/nptl/lowlevelrobustlock.c b/nptl/lowlevelrobustlock.c
index 3b988b2..8842032 100644
--- a/nptl/lowlevelrobustlock.c
+++ b/nptl/lowlevelrobustlock.c
@@ -113,15 +113,20 @@ __lll_robust_timedlock_wait (int *futex, const struct timespec *abstime,
 	  && atomic_compare_and_exchange_bool_acq (futex, newval, oldval))
 	continue;
 
+      int err;
       /* If *futex == 2, wait until woken or timeout.  */
 #if (!defined __ASSUME_FUTEX_CLOCK_REALTIME \
      || !defined lll_futex_timed_wait_bitset)
-      lll_futex_timed_wait (futex, newval, &rt, private);
+      err = lll_futex_timed_wait (futex, newval, &rt, private);
 #else
-      lll_futex_timed_wait_bitset (futex, newval, abstime,
+      err = lll_futex_timed_wait_bitset (futex, newval, abstime,
 				   FUTEX_CLOCK_REALTIME, private);
 #endif
 
+      /* the futex call time out */
+      if (err == -ETIMEDOUT)
+         return ETIMEDOUT;
+
     try:
       ;
     }
-- 
1.9.1