[v2] misc: Set generic pselect as ENOSYS

Message ID 20191114220056.16183-1-adhemerval.zanella@linaro.org
State Dropped
Headers

Commit Message

Adhemerval Zanella Nov. 14, 2019, 10 p.m. UTC
  Changes from previous version:

  - Added some include cleanups.

  - Add the __ASSUME_PSELECT usage on microblaze version.

--

The generic pselect implementation has the very specific race condition
that motived the creation of the pselect syscall (no atomicity in
signal mask set/reset).  Using it as generic implementation is
counterproductive  Also currently only microblaze uses it as fallback
when used on kernel prior 3.15.

This patch moves the generic implementation to a microblaze specific
one, sets the generic internal as a ENOSYS, and cleanups the Linux
generic implementation.

The microblaze implementation mimics the previous Linux generic one,
where it either uses pselect6 directly if __ASSUME_PSELECT or a
first try pselect6 then the fallback otherwise.

Checked on x86_64-linux-gnu and microblaze-linux-gnu.
---
 misc/pselect.c                               | 46 +-----------
 sysdeps/unix/sysv/linux/microblaze/pselect.c | 73 ++++++++++++++++++++
 sysdeps/unix/sysv/linux/pselect.c            | 37 ++--------
 3 files changed, 80 insertions(+), 76 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/microblaze/pselect.c
  

Comments

Adhemerval Zanella Nov. 22, 2019, 2:45 p.m. UTC | #1
I will commit this patch with Lukasz Majewski remark if no one opposes.

On 14/11/2019 19:00, Adhemerval Zanella wrote:
> Changes from previous version:
> 
>   - Added some include cleanups.
> 
>   - Add the __ASSUME_PSELECT usage on microblaze version.
> 
> --
> 
> The generic pselect implementation has the very specific race condition
> that motived the creation of the pselect syscall (no atomicity in
> signal mask set/reset).  Using it as generic implementation is
> counterproductive  Also currently only microblaze uses it as fallback
> when used on kernel prior 3.15.
> 
> This patch moves the generic implementation to a microblaze specific
> one, sets the generic internal as a ENOSYS, and cleanups the Linux
> generic implementation.
> 
> The microblaze implementation mimics the previous Linux generic one,
> where it either uses pselect6 directly if __ASSUME_PSELECT or a
> first try pselect6 then the fallback otherwise.
> 
> Checked on x86_64-linux-gnu and microblaze-linux-gnu.
> ---
>  misc/pselect.c                               | 46 +-----------
>  sysdeps/unix/sysv/linux/microblaze/pselect.c | 73 ++++++++++++++++++++
>  sysdeps/unix/sysv/linux/pselect.c            | 37 ++--------
>  3 files changed, 80 insertions(+), 76 deletions(-)
>  create mode 100644 sysdeps/unix/sysv/linux/microblaze/pselect.c
> 
> diff --git a/misc/pselect.c b/misc/pselect.c
> index 76ded850a5..b53d9cdcc1 100644
> --- a/misc/pselect.c
> +++ b/misc/pselect.c
> @@ -17,12 +17,7 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <errno.h>
> -#include <signal.h>
> -#include <stddef.h>	/* For NULL.  */
> -#include <sys/time.h>
>  #include <sys/select.h>
> -#include <sysdep-cancel.h>
> -
>  
>  /* Check the first NFDS descriptors each in READFDS (if not NULL) for read
>     readiness, in WRITEFDS (if not NULL) for write readiness, and in EXCEPTFDS
> @@ -34,43 +29,8 @@ int
>  __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
>  	   const struct timespec *timeout, const sigset_t *sigmask)
>  {
> -  struct timeval tval;
> -  int retval;
> -  sigset_t savemask;
> -
> -  /* Change nanosecond number to microseconds.  This might mean losing
> -     precision and therefore the `pselect` should be available.  But
> -     for now it is hardly found.  */
> -  if (timeout != NULL)
> -    {
> -      /* Catch bugs which would be hidden by the TIMESPEC_TO_TIMEVAL
> -	 computations.  The division by 1000 truncates values.  */
> -      if (__glibc_unlikely (timeout->tv_nsec < 0))
> -	{
> -	  __set_errno (EINVAL);
> -	  return -1;
> -	}
> -
> -      TIMESPEC_TO_TIMEVAL (&tval, timeout);
> -    }
> -
> -  /* The setting and restoring of the signal mask and the select call
> -     should be an atomic operation.  This can't be done without kernel
> -     help.  */
> -  if (sigmask != NULL)
> -    __sigprocmask (SIG_SETMASK, sigmask, &savemask);
> -
> -  /* Note the pselect() is a cancellation point.  But since we call
> -     select() which itself is a cancellation point we do not have
> -     to do anything here.  */
> -  retval = __select (nfds, readfds, writefds, exceptfds,
> -		     timeout != NULL ? &tval : NULL);
> -
> -  if (sigmask != NULL)
> -    __sigprocmask (SIG_SETMASK, &savemask, NULL);
> -
> -  return retval;
> +  __set_errno (ENOSYS);
> +  return -1;
>  }
> -#ifndef __pselect
>  weak_alias (__pselect, pselect)
> -#endif
> +stub_warning (pselect)
> diff --git a/sysdeps/unix/sysv/linux/microblaze/pselect.c b/sysdeps/unix/sysv/linux/microblaze/pselect.c
> new file mode 100644
> index 0000000000..561ff99fb4
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/microblaze/pselect.c
> @@ -0,0 +1,73 @@
> +/* Synchronous I/O multiplexing.  Linux/microblaze version.
> +   Copyright (C) 2019 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <signal.h>
> +#include <time.h>
> +#include <sys/poll.h>
> +#include <sysdep-cancel.h>
> +
> +#ifndef __ASSUME_PSELECT
> +# define __pselect __pselect_syscall
> +#endif
> +
> +/* If pselect is supported, just use the Linux generic implementation.  */
> +#include <sysdeps/unix/sysv/linux/pselect.c>
> +
> +#ifndef __ASSUME_PSELECT
> +# undef __pselect
> +int
> +__pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
> +	   const struct timespec *timeout, const sigset_t *sigmask)
> +{
> +  int ret = __pselect_syscall (nfds, readfds, writefds, exceptfds, timeout,
> +			       sigmask);
> +  if (ret < 0 && errno != ENOSYS)
> +    return ret;
> +
> +  /* The fallback uses 'select' which shows the race condition regarding
> +     signal mask set/restore, requires two additional syscalls, and has
> +     a worse timeout precision (microseconds instead of nanoseconds).  */
> +
> +  struct timeval tval, *ptval = NULL;
> +  if (timeout != NULL)
> +    {
> +      if (! valid_nanoseconds (timeout->tv_nsec))
> +	{
> +	  __set_errno (EINVAL);
> +	  return -1;
> +	}
> +
> +      TIMESPEC_TO_TIMEVAL (&tval, timeout);
> +      ptval = &tval;
> +    }
> +
> +  sigset_t savemask;
> +  if (sigmask != NULL)
> +    __sigprocmask (SIG_SETMASK, sigmask, &savemask);
> +
> +  /* select itself is a cancellation entrypoint.  */
> +  ret = __select (nfds, readfds, writefds, exceptfds, ptval);
> +
> +  if (sigmask != NULL)
> +    __sigprocmask (SIG_SETMASK, &savemask, NULL);
> +
> +  return ret;
> +}
> +weak_alias (__pselect, pselect)
> +#endif
> diff --git a/sysdeps/unix/sysv/linux/pselect.c b/sysdeps/unix/sysv/linux/pselect.c
> index acda3e0cdd..7a3dc8c4ed 100644
> --- a/sysdeps/unix/sysv/linux/pselect.c
> +++ b/sysdeps/unix/sysv/linux/pselect.c
> @@ -16,23 +16,9 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <errno.h>
> -#include <signal.h>
> -#include <time.h>
> -#include <sys/poll.h>
> -#include <kernel-features.h>
> +#include <sys/select.h>
>  #include <sysdep-cancel.h>
>  
> -
> -#ifdef __NR_pselect6
> -# ifndef __ASSUME_PSELECT
> -static int __generic_pselect (int nfds, fd_set *readfds, fd_set *writefds,
> -			      fd_set *exceptfds,
> -			      const struct timespec *timeout,
> -			      const sigset_t *sigmask);
> -# endif
> -
> -
>  int
>  __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
>  	   const struct timespec *timeout, const sigset_t *sigmask)
> @@ -59,24 +45,9 @@ __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
>    data.ss = (__syscall_ulong_t) (uintptr_t) sigmask;
>    data.ss_len = _NSIG / 8;
>  
> -  int result = SYSCALL_CANCEL (pselect6, nfds, readfds, writefds, exceptfds,
> -                               timeout, &data);
> -
> -# ifndef __ASSUME_PSELECT
> -  if (result == -1 && errno == ENOSYS)
> -    result = __generic_pselect (nfds, readfds, writefds, exceptfds, timeout,
> -				sigmask);
> -# endif
> -
> -  return result;
> +  return SYSCALL_CANCEL (pselect6, nfds, readfds, writefds, exceptfds,
> +                         timeout, &data);
>  }
> +#ifndef __pselect
>  weak_alias (__pselect, pselect)
> -
> -# ifndef __ASSUME_PSELECT
> -#  define __pselect static __generic_pselect
> -# endif
> -#endif
> -
> -#ifndef __ASSUME_PSELECT
> -# include <misc/pselect.c>
>  #endif
>
  

Patch

diff --git a/misc/pselect.c b/misc/pselect.c
index 76ded850a5..b53d9cdcc1 100644
--- a/misc/pselect.c
+++ b/misc/pselect.c
@@ -17,12 +17,7 @@ 
    <https://www.gnu.org/licenses/>.  */
 
 #include <errno.h>
-#include <signal.h>
-#include <stddef.h>	/* For NULL.  */
-#include <sys/time.h>
 #include <sys/select.h>
-#include <sysdep-cancel.h>
-
 
 /* Check the first NFDS descriptors each in READFDS (if not NULL) for read
    readiness, in WRITEFDS (if not NULL) for write readiness, and in EXCEPTFDS
@@ -34,43 +29,8 @@  int
 __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	   const struct timespec *timeout, const sigset_t *sigmask)
 {
-  struct timeval tval;
-  int retval;
-  sigset_t savemask;
-
-  /* Change nanosecond number to microseconds.  This might mean losing
-     precision and therefore the `pselect` should be available.  But
-     for now it is hardly found.  */
-  if (timeout != NULL)
-    {
-      /* Catch bugs which would be hidden by the TIMESPEC_TO_TIMEVAL
-	 computations.  The division by 1000 truncates values.  */
-      if (__glibc_unlikely (timeout->tv_nsec < 0))
-	{
-	  __set_errno (EINVAL);
-	  return -1;
-	}
-
-      TIMESPEC_TO_TIMEVAL (&tval, timeout);
-    }
-
-  /* The setting and restoring of the signal mask and the select call
-     should be an atomic operation.  This can't be done without kernel
-     help.  */
-  if (sigmask != NULL)
-    __sigprocmask (SIG_SETMASK, sigmask, &savemask);
-
-  /* Note the pselect() is a cancellation point.  But since we call
-     select() which itself is a cancellation point we do not have
-     to do anything here.  */
-  retval = __select (nfds, readfds, writefds, exceptfds,
-		     timeout != NULL ? &tval : NULL);
-
-  if (sigmask != NULL)
-    __sigprocmask (SIG_SETMASK, &savemask, NULL);
-
-  return retval;
+  __set_errno (ENOSYS);
+  return -1;
 }
-#ifndef __pselect
 weak_alias (__pselect, pselect)
-#endif
+stub_warning (pselect)
diff --git a/sysdeps/unix/sysv/linux/microblaze/pselect.c b/sysdeps/unix/sysv/linux/microblaze/pselect.c
new file mode 100644
index 0000000000..561ff99fb4
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/microblaze/pselect.c
@@ -0,0 +1,73 @@ 
+/* Synchronous I/O multiplexing.  Linux/microblaze version.
+   Copyright (C) 2019 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <signal.h>
+#include <time.h>
+#include <sys/poll.h>
+#include <sysdep-cancel.h>
+
+#ifndef __ASSUME_PSELECT
+# define __pselect __pselect_syscall
+#endif
+
+/* If pselect is supported, just use the Linux generic implementation.  */
+#include <sysdeps/unix/sysv/linux/pselect.c>
+
+#ifndef __ASSUME_PSELECT
+# undef __pselect
+int
+__pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
+	   const struct timespec *timeout, const sigset_t *sigmask)
+{
+  int ret = __pselect_syscall (nfds, readfds, writefds, exceptfds, timeout,
+			       sigmask);
+  if (ret < 0 && errno != ENOSYS)
+    return ret;
+
+  /* The fallback uses 'select' which shows the race condition regarding
+     signal mask set/restore, requires two additional syscalls, and has
+     a worse timeout precision (microseconds instead of nanoseconds).  */
+
+  struct timeval tval, *ptval = NULL;
+  if (timeout != NULL)
+    {
+      if (! valid_nanoseconds (timeout->tv_nsec))
+	{
+	  __set_errno (EINVAL);
+	  return -1;
+	}
+
+      TIMESPEC_TO_TIMEVAL (&tval, timeout);
+      ptval = &tval;
+    }
+
+  sigset_t savemask;
+  if (sigmask != NULL)
+    __sigprocmask (SIG_SETMASK, sigmask, &savemask);
+
+  /* select itself is a cancellation entrypoint.  */
+  ret = __select (nfds, readfds, writefds, exceptfds, ptval);
+
+  if (sigmask != NULL)
+    __sigprocmask (SIG_SETMASK, &savemask, NULL);
+
+  return ret;
+}
+weak_alias (__pselect, pselect)
+#endif
diff --git a/sysdeps/unix/sysv/linux/pselect.c b/sysdeps/unix/sysv/linux/pselect.c
index acda3e0cdd..7a3dc8c4ed 100644
--- a/sysdeps/unix/sysv/linux/pselect.c
+++ b/sysdeps/unix/sysv/linux/pselect.c
@@ -16,23 +16,9 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <signal.h>
-#include <time.h>
-#include <sys/poll.h>
-#include <kernel-features.h>
+#include <sys/select.h>
 #include <sysdep-cancel.h>
 
-
-#ifdef __NR_pselect6
-# ifndef __ASSUME_PSELECT
-static int __generic_pselect (int nfds, fd_set *readfds, fd_set *writefds,
-			      fd_set *exceptfds,
-			      const struct timespec *timeout,
-			      const sigset_t *sigmask);
-# endif
-
-
 int
 __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	   const struct timespec *timeout, const sigset_t *sigmask)
@@ -59,24 +45,9 @@  __pselect (int nfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
   data.ss = (__syscall_ulong_t) (uintptr_t) sigmask;
   data.ss_len = _NSIG / 8;
 
-  int result = SYSCALL_CANCEL (pselect6, nfds, readfds, writefds, exceptfds,
-                               timeout, &data);
-
-# ifndef __ASSUME_PSELECT
-  if (result == -1 && errno == ENOSYS)
-    result = __generic_pselect (nfds, readfds, writefds, exceptfds, timeout,
-				sigmask);
-# endif
-
-  return result;
+  return SYSCALL_CANCEL (pselect6, nfds, readfds, writefds, exceptfds,
+                         timeout, &data);
 }
+#ifndef __pselect
 weak_alias (__pselect, pselect)
-
-# ifndef __ASSUME_PSELECT
-#  define __pselect static __generic_pselect
-# endif
-#endif
-
-#ifndef __ASSUME_PSELECT
-# include <misc/pselect.c>
 #endif