[PATCH][BZ 19944] nptl/pthread_mutex*lock: don't ignore unknown error codes

[PATCH][BZ 19944] nptl/pthread_mutex*lock: don't ignore unknown error codes

[Sebastian Andrzej Siewior]

pthread_mutex_lock(), pthread_mutex_trylock() and pthread_mutex_unlock()
ignore the return code from the futex syscall unless it is something
the function expects.
This patch forwards the error code to the user instead of simply
ignoring it.

2016-04-13  Sebastian Andrzej Siewior  <bigeasy@linutronix.de>
	[BZ #19944]
	* nptl/pthread_mutex_lock.c: return unhandled sys_futex error.
	* nptl/pthread_mutex_trylock.c: return unhandled sys_futex
	error.
	* nptl/pthread_mutex_unlock.c: return unhandled sys_futex error.
---
 nptl/pthread_mutex_lock.c    | 3 ++-
 nptl/pthread_mutex_trylock.c | 2 ++
 nptl/pthread_mutex_unlock.c  | 5 ++++-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
index bdfa529f639b..f1b9aa1b9f84 100644
--- a/nptl/pthread_mutex_lock.c
+++ b/nptl/pthread_mutex_lock.c
@@ -354,7 +354,8 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
 		/* Delay the thread indefinitely.  */
 		while (1)
 		  pause_not_cancel ();
-	      }
+	      } else if (INTERNAL_SYSCALL_ERROR_P (e, __err))
+		      return INTERNAL_SYSCALL_ERRNO (e, __err);
 
 	    oldval = mutex->__data.__lock;
 
diff --git a/nptl/pthread_mutex_trylock.c b/nptl/pthread_mutex_trylock.c
index 48c7865702ba..45c416b75cf2 100644
--- a/nptl/pthread_mutex_trylock.c
+++ b/nptl/pthread_mutex_trylock.c
@@ -265,6 +265,8 @@ __pthread_mutex_trylock (pthread_mutex_t *mutex)
 		THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
 
 		return EBUSY;
+	      } else if (INTERNAL_SYSCALL_ERROR_P (e, __err)) {
+		      return INTERNAL_SYSCALL_ERRNO (e, __err);
 	      }
 
 	    oldval = mutex->__data.__lock;
diff --git a/nptl/pthread_mutex_unlock.c b/nptl/pthread_mutex_unlock.c
index 334ce383420e..787fb3017c44 100644
--- a/nptl/pthread_mutex_unlock.c
+++ b/nptl/pthread_mutex_unlock.c
@@ -243,8 +243,11 @@ __pthread_mutex_unlock_full (pthread_mutex_t *mutex, int decr)
 								  tid)))
 	{
 	  INTERNAL_SYSCALL_DECL (__err);
-	  INTERNAL_SYSCALL (futex, __err, 2, &mutex->__data.__lock,
+	  int e = INTERNAL_SYSCALL (futex, __err, 2, &mutex->__data.__lock,
 			    __lll_private_flag (FUTEX_UNLOCK_PI, private));
+
+	  if (INTERNAL_SYSCALL_ERROR_P (e, __err))
+		  return INTERNAL_SYSCALL_ERRNO (e, __err);
 	}
 
       THREAD_SETMEM (THREAD_SELF, robust_head.list_op_pending, NULL);
-- 
2.8.0.rc3

Re: [PATCH][BZ 19944] nptl/pthread_mutex*lock: don't ignore unknown error codes

[Roland McGrath]

I won't bother with the several style problems in your patch.  Please see
previous discussions about the futex error cases.  Torvald was leading the
charge on converting things to use the new internal layers that handle
futex error checking.  The only changes we want are in that direction.


Thanks,
Roland

Re: [PATCH][BZ 19944] nptl/pthread_mutex*lock: don't ignore unknown error codes

[Sebastian Andrzej Siewior]

On 2016-04-13 13:15:14 [-0700], Roland McGrath wrote:
> I won't bother with the several style problems in your patch.  Please see
> previous discussions about the futex error cases.  Torvald was leading the
> charge on converting things to use the new internal layers that handle
> futex error checking.  The only changes we want are in that direction.

Are we talking about this thread:
  https://sourceware.org/ml/libc-alpha/2014-09/msg00381.html

Subject: Futex error handling
Date: Tue, 16 Sep 2014 17:36:25 +0200

> Thanks,
> Roland

Sebastian

Re: [PATCH][BZ 19944] nptl/pthread_mutex*lock: don't ignore unknown error codes

[Torvald Riegel]

On Wed, 2016-04-13 at 22:52 +0200, Sebastian Andrzej Siewior wrote:
> On 2016-04-13 13:15:14 [-0700], Roland McGrath wrote:
> > I won't bother with the several style problems in your patch.  Please see
> > previous discussions about the futex error cases.  Torvald was leading the
> > charge on converting things to use the new internal layers that handle
> > futex error checking.  The only changes we want are in that direction.
> 
> Are we talking about this thread:
>   https://sourceware.org/ml/libc-alpha/2014-09/msg00381.html
> 
> Subject: Futex error handling
> Date: Tue, 16 Sep 2014 17:36:25 +0200

That is the start of it.  The result is that we have a new
glibc-internal futex interface that does error checking, see
sysdeps/unix/sysv/linux/futex-internal.h and
sysdeps/nptl/futex-internal.h for details.

The way we handle futex errors should be consistent with what we do in
there; if you spot missing errors we do not test for explicitly (ie,
which are listed in the updated futex manpage but not part of the glibc
code), please fix this in the wrappers.

Regarding the mutex changes, what we should do there eventually is use
the new futex interface.  We cannot just forward errors to the callers
of POSIX functions because POSIX might not specify the same error codes
as futexes do.

[PATCH RFC][BZ 19944] ntpl: pthread_mutex_lock() use a wrapper for error checking

[Sebastian Andrzej Siewior]

Implement a wrapper around kernel's FUTEX_LOCK_PI opcode / syscall to
filter error codes. Returned to caller:
- EDEADLOCK: detected a deadlock
- EINVAL: if uaddr's address is not a multiple of four.
- ENOMEM: failed to allocate memory for PI state.
          I am not sure if it is better to return it to the caller or
	  retry again. A sleep() between invocation is probably a bad
	  idea if the process is RT. Trying again is probably a bad idea
	  if the process is RT with the highest priority.
- EAGAIN: The owner is about to terminate. Same as with ENOMEM (not sure
          what is the best action here).
- ESRCH: Happens also for non-robust futex if the owner exits. The lock
         value becomes FUTEX_OWNER_DIED set. Returned as EOWNERDEAD.

Error codes which should not happen invoke futex_fatal_error().
2016-04-13  Sebastian Andrzej Siewior  <bigeasy@linutronix.de>
	[BZ #19944] (partly)
	* sysdeps/nptl/futex-internal.h: futex_lock_pi () prototype
	* sysdeps/unix/sysv/linux/futex-internal.h: implement futex_lock_pi()
	* nptl/pthread_mutex_lock.c: use new wrapper futex_lock_pi ()
---

Compile tested only.
Could we extend futex_fatal_error() a little to pass the opcode + error
code? If it is not fully reproducible it would be nice to know what led
to it.

 nptl/pthread_mutex_lock.c                | 31 ++++++---------------------
 sysdeps/nptl/futex-internal.h            |  3 +++
 sysdeps/unix/sysv/linux/futex-internal.h | 36 ++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 25 deletions(-)

diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
index bdfa529f639b..4ecd2d8a91ec 100644
--- a/nptl/pthread_mutex_lock.c
+++ b/nptl/pthread_mutex_lock.c
@@ -26,6 +26,7 @@
 #include <atomic.h>
 #include <lowlevellock.h>
 #include <stap-probe.h>
+#include <futex-internal.h>
 
 #ifndef lll_lock_elision
 #define lll_lock_elision(lock, try_lock, private)	({ \
@@ -332,36 +333,16 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
 	  {
 	    /* The mutex is locked.  The kernel will now take care of
 	       everything.  */
-	    int private = (robust
-			   ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
-			   : PTHREAD_MUTEX_PSHARED (mutex));
-	    INTERNAL_SYSCALL_DECL (__err);
-	    int e = INTERNAL_SYSCALL (futex, __err, 4, &mutex->__data.__lock,
-				      __lll_private_flag (FUTEX_LOCK_PI,
-							  private), 1, 0);
+	    int ret;
 
-	    if (INTERNAL_SYSCALL_ERROR_P (e, __err)
-		&& (INTERNAL_SYSCALL_ERRNO (e, __err) == ESRCH
-		    || INTERNAL_SYSCALL_ERRNO (e, __err) == EDEADLK))
-	      {
-		assert (INTERNAL_SYSCALL_ERRNO (e, __err) != EDEADLK
-			|| (kind != PTHREAD_MUTEX_ERRORCHECK_NP
-			    && kind != PTHREAD_MUTEX_RECURSIVE_NP));
-		/* ESRCH can happen only for non-robust PI mutexes where
-		   the owner of the lock died.  */
-		assert (INTERNAL_SYSCALL_ERRNO (e, __err) != ESRCH || !robust);
-
-		/* Delay the thread indefinitely.  */
-		while (1)
-		  pause_not_cancel ();
-	      }
+	    ret = futex_lock_pi(mutex);
+	    if (ret)
+		    return ret;
 
 	    oldval = mutex->__data.__lock;
-
-	    assert (robust || (oldval & FUTEX_OWNER_DIED) == 0);
 	  }
 
-	if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED))
+	if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED) && robust)
 	  {
 	    atomic_and (&mutex->__data.__lock, ~FUTEX_OWNER_DIED);
 
diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
index d798b6970893..3c376788437a 100644
--- a/sysdeps/nptl/futex-internal.h
+++ b/sysdeps/nptl/futex-internal.h
@@ -192,6 +192,9 @@ futex_abstimed_wait_cancelable (unsigned int* futex_word,
 static __always_inline void
 futex_wake (unsigned int* futex_word, int processes_to_wake, int private);
 
+/* Invoke kernel's FUTEX_LOCK_PI syscall. */
+static __always_inline int futex_lock_pi(pthread_mutex_t *mutex);
+
 /* Calls __libc_fatal with an error message.  Convenience function for
    concrete implementations of the futex interface.  */
 static __always_inline __attribute__ ((__noreturn__)) void
diff --git a/sysdeps/unix/sysv/linux/futex-internal.h b/sysdeps/unix/sysv/linux/futex-internal.h
index 1add836ebc2f..6b571ae82e33 100644
--- a/sysdeps/unix/sysv/linux/futex-internal.h
+++ b/sysdeps/unix/sysv/linux/futex-internal.h
@@ -248,4 +248,40 @@ futex_wake (unsigned int *futex_word, int processes_to_wake, int private)
     }
 }
 
+static __always_inline int futex_lock_pi(pthread_mutex_t *mutex)
+{
+  INTERNAL_SYSCALL_DECL (err);
+  int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP;
+  int private;
+  int ret;
+
+  private = (robust
+	     ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
+	     : PTHREAD_MUTEX_PSHARED (mutex));
+
+  ret = INTERNAL_SYSCALL (futex, err, 4, &mutex->__data.__lock,
+		    __lll_private_flag (FUTEX_LOCK_PI,
+					private), 1, 0);
+  if (!INTERNAL_SYSCALL_ERROR_P(ret, err))
+    return 0;
+  ret = INTERNAL_SYSCALL_ERRNO (ret, err);
+  switch (ret)
+    {
+    case EDEADLOCK:	/* dead lock detected */
+    case EINVAL:	/* the __lock variable is not properly aligned */
+    case ENOMEM:	/* internal memory allocation failed */
+    case EAGAIN:	/* lock owner is terminating */
+      return ret;
+    case ESRCH:		/* The process holding the lock is gone */
+      return EOWNERDEAD;
+    default:
+    case EPERM:		/* not possible (unlock) or PID of owner belongs to a
+			   kernel thread */
+    case EFAULT:	/* we dereferenced the pointer, kernel should be able to
+			   do, too */
+    case ETIMEDOUT:	/* didn't ask for timeout */
+      futex_fatal_error ();
+    }
+}
+
 #endif  /* futex-internal.h */
-- 
2.8.1

Re: [PATCH RFC][BZ 19944] ntpl: pthread_mutex_lock() use a wrapper for error checking

[Torvald Riegel]

On Tue, 2016-05-31 at 18:40 +0200, Sebastian Andrzej Siewior wrote:
> Implement a wrapper around kernel's FUTEX_LOCK_PI opcode / syscall to
> filter error codes. Returned to caller:
> - EDEADLOCK: detected a deadlock
> - EINVAL: if uaddr's address is not a multiple of four.
> - ENOMEM: failed to allocate memory for PI state.
>           I am not sure if it is better to return it to the caller or
> 	  retry again. A sleep() between invocation is probably a bad
> 	  idea if the process is RT. Trying again is probably a bad idea
> 	  if the process is RT with the highest priority.
> - EAGAIN: The owner is about to terminate. Same as with ENOMEM (not sure
>           what is the best action here).
> - ESRCH: Happens also for non-robust futex if the owner exits. The lock
>          value becomes FUTEX_OWNER_DIED set. Returned as EOWNERDEAD.
> 
> Error codes which should not happen invoke futex_fatal_error().

Thanks for trying to improve how we handle errors.

There are two levels of interfaces we need to consider.  First, the
futex interface in futex-internal.h should return errors that a caller
of a futex operation would have to deal with; thus, this is basically
the errors the kernel can return, with the exception that we abort early
on errors that can only arise due to bugs in the kernel or glibc.

Thus, adding a futex_lock_pi to futex-internal.h seems fine, but then
this should be an abstraction for the FUTEX_LOCK_PI operation: It should
not need to be aware of pthread_mutex_t internals, and it should not
convert error codes.

The patch should also add the matching FUTEX_UNLOCK_PI operations.  

Furthermore, it should define these operations for nacl too, I believe,
but just abort when they are called as they aren't supported there, and
should never be called in the first place.  Another option would be to
leave them simply declared but not defined.

The second interface is pthread mutexes.  The error codes we can return
for these should follow what's defined by POSIX (although we already
deviate from that sometimes); if this doesn't work in practice, this
should be discussed with the Austin Group.  If we deviate, we need to
document it.
The pthread mutex code would then call the futex interface, and
transform errors as required or abort on others that we cannot return.

More detailed comments below.

> 2016-04-13  Sebastian Andrzej Siewior  <bigeasy@linutronix.de>
> 	[BZ #19944] (partly)
> 	* sysdeps/nptl/futex-internal.h: futex_lock_pi () prototype
> 	* sysdeps/unix/sysv/linux/futex-internal.h: implement futex_lock_pi()
> 	* nptl/pthread_mutex_lock.c: use new wrapper futex_lock_pi ()
> ---
> 
> Compile tested only.

Tests for this would be good.

> Could we extend futex_fatal_error() a little to pass the opcode + error
> code? If it is not fully reproducible it would be nice to know what led
> to it.
> 
>  nptl/pthread_mutex_lock.c                | 31 ++++++---------------------
>  sysdeps/nptl/futex-internal.h            |  3 +++
>  sysdeps/unix/sysv/linux/futex-internal.h | 36 ++++++++++++++++++++++++++++++++
>  3 files changed, 45 insertions(+), 25 deletions(-)
> 
> diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
> index bdfa529f639b..4ecd2d8a91ec 100644
> --- a/nptl/pthread_mutex_lock.c
> +++ b/nptl/pthread_mutex_lock.c
> @@ -26,6 +26,7 @@
>  #include <atomic.h>
>  #include <lowlevellock.h>
>  #include <stap-probe.h>
> +#include <futex-internal.h>
>  
>  #ifndef lll_lock_elision
>  #define lll_lock_elision(lock, try_lock, private)	({ \
> @@ -332,36 +333,16 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
>  	  {
>  	    /* The mutex is locked.  The kernel will now take care of
>  	       everything.  */
> -	    int private = (robust
> -			   ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
> -			   : PTHREAD_MUTEX_PSHARED (mutex));
> -	    INTERNAL_SYSCALL_DECL (__err);
> -	    int e = INTERNAL_SYSCALL (futex, __err, 4, &mutex->__data.__lock,
> -				      __lll_private_flag (FUTEX_LOCK_PI,
> -							  private), 1, 0);
> +	    int ret;
>  
> -	    if (INTERNAL_SYSCALL_ERROR_P (e, __err)
> -		&& (INTERNAL_SYSCALL_ERRNO (e, __err) == ESRCH
> -		    || INTERNAL_SYSCALL_ERRNO (e, __err) == EDEADLK))
> -	      {
> -		assert (INTERNAL_SYSCALL_ERRNO (e, __err) != EDEADLK
> -			|| (kind != PTHREAD_MUTEX_ERRORCHECK_NP
> -			    && kind != PTHREAD_MUTEX_RECURSIVE_NP));
> -		/* ESRCH can happen only for non-robust PI mutexes where
> -		   the owner of the lock died.  */
> -		assert (INTERNAL_SYSCALL_ERRNO (e, __err) != ESRCH || !robust);
> -
> -		/* Delay the thread indefinitely.  */
> -		while (1)
> -		  pause_not_cancel ();
> -	      }
> +	    ret = futex_lock_pi(mutex);
> +	    if (ret)
> +		    return ret;
>  
>  	    oldval = mutex->__data.__lock;
> -
> -	    assert (robust || (oldval & FUTEX_OWNER_DIED) == 0);
>  	  }
>  
> -	if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED))
> +	if (__glibc_unlikely (oldval & FUTEX_OWNER_DIED) && robust)
>  	  {
>  	    atomic_and (&mutex->__data.__lock, ~FUTEX_OWNER_DIED);
>  
> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
> index d798b6970893..3c376788437a 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -192,6 +192,9 @@ futex_abstimed_wait_cancelable (unsigned int* futex_word,
>  static __always_inline void
>  futex_wake (unsigned int* futex_word, int processes_to_wake, int private);
>  
> +/* Invoke kernel's FUTEX_LOCK_PI syscall. */

This needs the same level of detail in the documentation as we have for
other futex operations.  Also, two spaces behind the . please.

> +static __always_inline int futex_lock_pi(pthread_mutex_t *mutex);
> +
>  /* Calls __libc_fatal with an error message.  Convenience function for
>     concrete implementations of the futex interface.  */
>  static __always_inline __attribute__ ((__noreturn__)) void
> diff --git a/sysdeps/unix/sysv/linux/futex-internal.h b/sysdeps/unix/sysv/linux/futex-internal.h
> index 1add836ebc2f..6b571ae82e33 100644
> --- a/sysdeps/unix/sysv/linux/futex-internal.h
> +++ b/sysdeps/unix/sysv/linux/futex-internal.h
> @@ -248,4 +248,40 @@ futex_wake (unsigned int *futex_word, int processes_to_wake, int private)
>      }
>  }
>  
> +static __always_inline int futex_lock_pi(pthread_mutex_t *mutex)

See above.

> +{
> +  INTERNAL_SYSCALL_DECL (err);
> +  int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP;
> +  int private;
> +  int ret;
> +
> +  private = (robust
> +	     ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
> +	     : PTHREAD_MUTEX_PSHARED (mutex));

This belongs into the mutex code, not the futex code.

> +
> +  ret = INTERNAL_SYSCALL (futex, err, 4, &mutex->__data.__lock,
> +		    __lll_private_flag (FUTEX_LOCK_PI,
> +					private), 1, 0);
> +  if (!INTERNAL_SYSCALL_ERROR_P(ret, err))
> +    return 0;
> +  ret = INTERNAL_SYSCALL_ERRNO (ret, err);
> +  switch (ret)
> +    {
> +    case EDEADLOCK:	/* dead lock detected */
> +    case EINVAL:	/* the __lock variable is not properly aligned */
> +    case ENOMEM:	/* internal memory allocation failed */
> +    case EAGAIN:	/* lock owner is terminating */
> +      return ret;
> +    case ESRCH:		/* The process holding the lock is gone */
> +      return EOWNERDEAD;
> +    default:
> +    case EPERM:		/* not possible (unlock) or PID of owner belongs to a
> +			   kernel thread */
> +    case EFAULT:	/* we dereferenced the pointer, kernel should be able to
> +			   do, too */
> +    case ETIMEDOUT:	/* didn't ask for timeout */

Formatting of comments as elsewhere in the file, please.  Some of these
also seem to need more detail.