Re: [PATCH v2 04/11] linux: Add clone3 CLONE_CLEAR_SIGHAND optimization to posix_spawn

[Carlos O'Donell via Libc-alpha <libc-alpha@sourceware.org>]

On 11/4/22 15:01, Adhemerval Zanella via Libc-alpha wrote:
> The clone3 flag resets all signal handlers of the child not set to
> SIG_IGN to SIG_DFL.  It allows to skip most of the sigaction calls
> to setup child signal handling, where previously a posix_spawn
> has to issue 2 times NSIG sigaction calls (one to obtain the current
> disposition and another to set either SIG_DFL or SIG_IGN).
> 
> The expection is POSIX_SPAWN_SETSIGDEF the child still setup the
> signal for the case the disposition is SIG_IGN.
> 
> It also need to handle the fallback where clone3 is not available,
> to set the fallback in child.  This is done by splitting of
> __clone_internal_fallback from __clone_internal.
> 

See original v2 review. Please post v4.

> Checked on x86_64-linux-gnu.
> ---
>  include/clone_internal.h                 |   5 +
>  posix/Makefile                           |   3 +-
>  posix/tst-spawn7.c                       | 179 +++++++++++++++++++++++
>  sysdeps/unix/sysv/linux/Makefile         |   3 +-
>  sysdeps/unix/sysv/linux/clone-internal.c |  37 +++--
>  sysdeps/unix/sysv/linux/clone3.h         |   7 +
>  sysdeps/unix/sysv/linux/spawni.c         |  31 ++--
>  7 files changed, 238 insertions(+), 27 deletions(-)
>  create mode 100644 posix/tst-spawn7.c
> 
> diff --git a/include/clone_internal.h b/include/clone_internal.h
> index 5a48c7caa5..81292b3eb1 100644
> --- a/include/clone_internal.h
> +++ b/include/clone_internal.h
> @@ -26,6 +26,11 @@ extern int __clone3 (struct clone_args *__cl_args, size_t __size,
>     fall back to clone or clone2.  */
>  extern int __clone_internal (struct clone_args *__cl_args,
>  			     int (*__func) (void *__arg), void *__arg);
> +/* The fallback code which calls clone/clone2 based on clone3 arguments.  */
> +extern int __clone_internal_fallback (struct clone_args *__cl_args,
> +				      int (*__func) (void *__arg),
> +				      void *__arg)
> +     attribute_hidden;
>  
>  #ifndef _ISOMAC
>  libc_hidden_proto (__clone3)
> diff --git a/posix/Makefile b/posix/Makefile
> index 4d60449426..6e61ce50f2 100644
> --- a/posix/Makefile
> +++ b/posix/Makefile
> @@ -109,7 +109,7 @@ tests		:= test-errno tstgetopt testfnm runtests runptests \
>  		   tst-glob-tilde test-ssize-max tst-spawn4 bug-regex37 \
>  		   bug-regex38 tst-regcomp-truncated tst-spawn-chdir \
>  		   tst-wordexp-nocmd tst-execveat tst-spawn5 \
> -		   tst-sched_getaffinity tst-spawn6
> +		   tst-sched_getaffinity tst-spawn6 tst-spawn7
>  
>  # Test for the glob symbol version that was replaced in glibc 2.27.
>  ifeq ($(have-GLIBC_2.26)$(build-shared),yesyes)
> @@ -291,6 +291,7 @@ tst-spawn-ARGS = -- $(host-test-program-cmd)
>  tst-spawn-static-ARGS = $(tst-spawn-ARGS)
>  tst-spawn5-ARGS = -- $(host-test-program-cmd)
>  tst-spawn6-ARGS = -- $(host-test-program-cmd)
> +tst-spawn7-ARGS = -- $(host-test-program-cmd)
>  tst-dir-ARGS = `pwd` `cd $(common-objdir)/$(subdir); pwd` `cd $(common-objdir); pwd` $(objpfx)tst-dir
>  tst-chmod-ARGS = $(objdir)
>  tst-vfork3-ARGS = --test-dir=$(objpfx)
> diff --git a/posix/tst-spawn7.c b/posix/tst-spawn7.c
> new file mode 100644
> index 0000000000..7cb7347c58
> --- /dev/null
> +++ b/posix/tst-spawn7.c
> @@ -0,0 +1,179 @@
> +/* Tests for posix_spawn signal handling.
> +   Copyright (C) 2022 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 <assert.h>
> +#include <getopt.h>
> +#include <spawn.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <support/check.h>
> +#include <support/xsignal.h>
> +#include <support/xunistd.h>
> +#include <unistd.h>
> +
> +/* Nonzero if the program gets called via `exec'.  */
> +#define CMDLINE_OPTIONS \
> +  { "restart", no_argument, &restart, 1 },
> +static int restart;
> +
> +/* Hold the four initial argument used to respawn the process, plus the extra
> +   '--direct', '--restart', the check type ('SIG_IGN' or 'SIG_DFL'), and a
> +   final NULL.  */
> +static char *spargs[8];
> +static int check_type_argc;
> +
> +/* Called on process re-execution.  */
> +_Noreturn static void
> +handle_restart (int argc, char *argv[])
> +{
> +  assert (argc == 1);
> +
> +  if (strcmp (argv[0], "SIG_DFL") == 0)
> +    {
> +      for (int i = 1; i < NSIG; i++)
> +	{
> +	  struct sigaction sa;
> +	  int r = sigaction (i, NULL, &sa);
> +	  /* Skip internal signals (such as SIGCANCEL).  */
> +	  if (r == -1)
> +	    continue;
> +	  TEST_VERIFY_EXIT (sa.sa_handler == SIG_DFL);
> +	}
> +      exit (EXIT_SUCCESS);
> +    }
> +  else if (strcmp (argv[0], "SIG_IGN") == 0)
> +    {
> +      for (int i = 1; i < NSIG; i++)
> +	{
> +	  struct sigaction sa;
> +	  int r = sigaction (i, NULL, &sa);
> +	  /* Skip internal signals (such as SIGCANCEL).  */
> +	  if (r == -1)
> +	    continue;
> +	  if (i == SIGUSR1 || i == SIGUSR2)
> +	    TEST_VERIFY_EXIT (sa.sa_handler == SIG_IGN);
> +	  else
> +	    TEST_VERIFY_EXIT (sa.sa_handler == SIG_DFL);
> +	}
> +      exit (EXIT_SUCCESS);
> +    }
> +
> +  exit (EXIT_FAILURE);
> +}
> +
> +static void
> +spawn_signal_test (const char *type, const posix_spawnattr_t *attr)
> +{
> +  spargs[check_type_argc] = (char*) type;
> +
> +  pid_t pid;
> +  int status;
> +
> +  TEST_COMPARE (posix_spawn (&pid, spargs[0], NULL, attr, spargs, environ), 0);
> +  TEST_COMPARE (xwaitpid (pid, &status, 0), pid);
> +  TEST_VERIFY (WIFEXITED (status));
> +  TEST_VERIFY (!WIFSIGNALED (status));
> +  TEST_COMPARE (WEXITSTATUS (status), 0);
> +}
> +
> +static void
> +dummy_sa_handler (int)
> +{
> +}
> +
> +static void
> +do_test_signals (void)
> +{
> +  {
> +    /* Check if all signals handler are set to SIG_DFL on spawned process.  */
> +    spawn_signal_test ("SIG_DFL", NULL);
> +  }
> +
> +  {
> +    /* Same as before, but set SIGUSR1 and SIGUSR2 to a handler different than
> +       SIG_IGN or SIG_DFL.  */
> +    struct sigaction sa = { 0 };
> +    sa.sa_handler = dummy_sa_handler;
> +    xsigaction (SIGUSR1, &sa, NULL);
> +    xsigaction (SIGUSR2, &sa, NULL);
> +    spawn_signal_test ("SIG_DFL", NULL);
> +  }
> +
> +  {
> +    /* Check if SIG_IGN is keep as is.  */
> +    struct sigaction sa = { 0 };
> +    sa.sa_handler = SIG_IGN;
> +    xsigaction (SIGUSR1, &sa, NULL);
> +    xsigaction (SIGUSR2, &sa, NULL);
> +    spawn_signal_test ("SIG_IGN", NULL);
> +  }
> +
> +  {
> +    /* Check if SIG_IGN handlers are set to SIG_DFL.  */
> +    posix_spawnattr_t attr;
> +    posix_spawnattr_init (&attr);
> +    sigset_t mask;
> +    sigemptyset (&mask);
> +    sigaddset (&mask, SIGUSR1);
> +    sigaddset (&mask, SIGUSR2);
> +    posix_spawnattr_setsigdefault (&attr, &mask);
> +    posix_spawnattr_setflags (&attr, POSIX_SPAWN_SETSIGDEF);
> +    spawn_signal_test ("SIG_DFL", &attr);
> +    posix_spawnattr_destroy (&attr);
> +  }
> +}
> +
> +static int
> +do_test (int argc, char *argv[])
> +{
> +  /* We must have either:
> +
> +     - one or four parameters if called initially:
> +       + argv[1]: path for ld.so        optional
> +       + argv[2]: "--library-path"      optional
> +       + argv[3]: the library path      optional
> +       + argv[4]: the application name
> +
> +     - six parameters left if called through re-execution:
> +       + argv[1]: the application name
> +       + argv[2]: check SIG_IGN/SIG_DFL.
> +
> +     * When built with --enable-hardcoded-path-in-tests or issued without
> +       using the loader directly.  */
> +
> +  if (restart)
> +    handle_restart (argc - 1, &argv[1]);
> +
> +  TEST_VERIFY_EXIT (argc == 2 || argc == 5);
> +
> +  int i;
> +  for (i = 0; i < argc - 1; i++)
> +    spargs[i] = argv[i + 1];
> +  spargs[i++] = (char *) "--direct";
> +  spargs[i++] = (char *) "--restart";
> +  check_type_argc = i++;
> +  spargs[i] = NULL;
> +
> +
> +  do_test_signals ();
> +
> +  return 0;
> +}
> +
> +#define TEST_FUNCTION_ARGV do_test
> +#include <support/test-driver.c>
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 60e4cb1521..c183ccd4ab 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -175,8 +175,7 @@ tests-clone-internal = \
>    tst-align-clone-internal \
>    tst-clone2-internal \
>    tst-clone3-internal \
> -  tst-getpid1-internal \
> -  tst-misalign-clone-internal
> +  tst-getpid1-internal
>  tests-internal += $(tests-clone-internal)
>  tests-static += $(tests-clone-internal)
>  
> diff --git a/sysdeps/unix/sysv/linux/clone-internal.c b/sysdeps/unix/sysv/linux/clone-internal.c
> index a71effcbd3..7bc991e033 100644
> --- a/sysdeps/unix/sysv/linux/clone-internal.c
> +++ b/sysdeps/unix/sysv/linux/clone-internal.c
> @@ -44,27 +44,15 @@ _Static_assert (sizeof (struct clone_args) == CLONE_ARGS_SIZE_VER2,
>  		"sizeof (struct clone_args) != CLONE_ARGS_SIZE_VER2");
>  
>  int
> -__clone_internal (struct clone_args *cl_args,
> -		  int (*func) (void *arg), void *arg)
> +__clone_internal_fallback (struct clone_args *cl_args,
> +			   int (*func) (void *arg), void *arg)
>  {
> -  int ret;
> -#ifdef HAVE_CLONE3_WRAPPER
> -  /* Try clone3 first.  */
> -  int saved_errno = errno;
> -  ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
> -  if (ret != -1 || errno != ENOSYS)
> -    return ret;
> -
> -  /* NB: Restore errno since errno may be checked against non-zero
> -     return value.  */
> -  __set_errno (saved_errno);
> -#endif
> -
>    /* Map clone3 arguments to clone arguments.  NB: No need to check
>       invalid clone3 specific bits in flags nor exit_signal since this
>       is an internal function.  */
>    int flags = cl_args->flags | cl_args->exit_signal;
>    void *stack = cast_to_pointer (cl_args->stack);
> +  int ret;
>  
>  #ifdef __ia64__
>    ret = __clone2 (func, stack, cl_args->stack_size,
> @@ -88,4 +76,23 @@ __clone_internal (struct clone_args *cl_args,
>    return ret;
>  }
>  
> +
> +int
> +__clone_internal (struct clone_args *cl_args,
> +		  int (*func) (void *arg), void *arg)
> +{
> +#ifdef HAVE_CLONE3_WRAPPER
> +  int saved_errno = errno;
> +  int ret = __clone3 (cl_args, sizeof (*cl_args), func, arg);
> +  if (ret != -1 || errno != ENOSYS)
> +    return ret;
> +
> +  /* NB: Restore errno since errno may be checked against non-zero
> +     return value.  */
> +  __set_errno (saved_errno);
> +#endif
> +
> +  return __clone_internal_fallback (cl_args, func, arg);
> +}
> +
>  libc_hidden_def (__clone_internal)
> diff --git a/sysdeps/unix/sysv/linux/clone3.h b/sysdeps/unix/sysv/linux/clone3.h
> index 7c7e2cdcb0..7dd42ec2d2 100644
> --- a/sysdeps/unix/sysv/linux/clone3.h
> +++ b/sysdeps/unix/sysv/linux/clone3.h
> @@ -23,6 +23,13 @@
>  #include <stddef.h>
>  #include <bits/types.h>
>  
> +
> +/* Flags for the clone3 syscall.  */
> +#define CLONE_CLEAR_SIGHAND 0x100000000ULL /* Clear any signal handler and
> +					      reset to SIG_DFL.  */
> +#define CLONE_INTO_CGROUP 0x200000000ULL /* Clone into a specific cgroup given
> +					    the right permissions.  */
> +
>  /* The unsigned 64-bit and 8-byte aligned integer type.  */
>  typedef __U64_TYPE __aligned_uint64_t __attribute__ ((__aligned__ (8)));
>  
> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index 65ee03c804..ebc3c3e521 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c
> @@ -66,6 +66,7 @@ struct posix_spawn_args
>    ptrdiff_t argc;
>    char *const *envp;
>    int xflags;
> +  bool use_clone3;
>    int err;
>  };
>  
> @@ -104,12 +105,11 @@ __spawni_child (void *arguments)
>    const posix_spawnattr_t *restrict attr = args->attr;
>    const posix_spawn_file_actions_t *file_actions = args->fa;
>  
> -  /* The child must ensure that no signal handler are enabled because it shared
> -     memory with parent, so the signal disposition must be either SIG_DFL or
> -     SIG_IGN.  It does by iterating over all signals and although it could
> -     possibly be more optimized (by tracking which signal potentially have a
> -     signal handler), it might requires system specific solutions (since the
> -     sigset_t data type can be very different on different architectures).  */
> +  /* The child must ensure that no signal handler is enabled because it
> +     shares memory with parent, so all signal dispositions must be either
> +     SIG_DFL or SIG_IGN.  If clone3/CLONE_CLEAR_SIGHAND is used, there is
> +     only need to set the defined signals POSIX_SPAWN_SETSIGDEF to SIG_DFL;
> +     otherwise, the code iterates over all signals.  */
>    struct sigaction sa;
>    memset (&sa, '\0', sizeof (sa));
>  
> @@ -122,7 +122,7 @@ __spawni_child (void *arguments)
>  	{
>  	  sa.sa_handler = SIG_DFL;
>  	}
> -      else if (__sigismember (&hset, sig))
> +      else if (!args->use_clone3 && __sigismember (&hset, sig))
>  	{
>  	  if (is_internal_signal (sig))
>  	    sa.sa_handler = SIG_IGN;
> @@ -382,12 +382,25 @@ __spawnix (pid_t * pid, const char *file,
>       for instance).  */
>    struct clone_args clone_args =
>      {
> -      .flags = CLONE_VM | CLONE_VFORK,
> +      /* Unsupported flags like CLONE_CLEAR_SIGHAND will be cleared up by
> +	 __clone_internal_fallback.  */
> +      .flags = CLONE_CLEAR_SIGHAND | CLONE_VM | CLONE_VFORK,
>        .exit_signal = SIGCHLD,
>        .stack = (uintptr_t) stack,
>        .stack_size = stack_size,
>      };
> -  new_pid = __clone_internal (&clone_args, __spawni_child, &args);
> +#ifdef HAVE_CLONE3_WRAPPER
> +  args.use_clone3 = true;
> +  new_pid = __clone3 (&clone_args, sizeof (clone_args), __spawni_child,
> +		      &args);
> +  /* clone3 was added on 5.3 and CLONE_CLEAR_SIGHAND on 5.5.  */
> +  if (new_pid == -1 && (errno == ENOSYS || errno == EINVAL))
> +#endif
> +    {
> +      args.use_clone3 = false;
> +      new_pid = __clone_internal_fallback (&clone_args, __spawni_child,
> +					   &args);
> +    }
>  
>    /* It needs to collect the case where the auxiliary process was created
>       but failed to execute the file (due either any preparation step or

-- 
Cheers,
Carlos.