Re: [PATCH v2] misc: Set generic pselect as ENOSYS

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

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
>