* H. J. Lu:
> diff --git a/include/clone_internal.h b/include/clone_internal.h
> new file mode 100644
> index 0000000000..124f7ba169
> --- /dev/null
> +++ b/include/clone_internal.h
> @@ -0,0 +1,14 @@
> +#ifndef _CLONE3_H
> +#include_next <clone3.h>
> +
> +extern __typeof (clone3) __clone3;
> +
> +/* The internal wrapper of clone and clone3. */
> +extern __typeof (clone3) __clone_internal;
Maybe mention fallback explicitly?
> diff --git a/include/libc-pointer-arith.h b/include/libc-pointer-arith.h
> index 72e722c5aa..04ba537617 100644
> --- a/include/libc-pointer-arith.h
> +++ b/include/libc-pointer-arith.h
> @@ -37,6 +37,9 @@
> /* Cast an integer or a pointer VAL to integer with proper type. */
> # define cast_to_integer(val) ((__integer_if_pointer_type (val)) (val))
>
> +/* Cast an integer VAL to void * pointer. */
> +# define cast_to_pointer(val) ((void *) (uintptr_t) (val))
> +
> /* Align a value by rounding down to closest size.
> e.g. Using size of 4096, we get this behavior:
> {4095, 4096, 4097} = {0, 4096, 4096}. */
As a regular backporter, I'd like to see this in a separate commit if
possible.
> diff --git a/sysdeps/unix/sysv/linux/clone-internal.c b/sysdeps/unix/sysv/linux/clone-internal.c
> new file mode 100644
> index 0000000000..c357b0ac14
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/clone-internal.c
> +#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
> +#define offsetofend(TYPE, MEMBER) \
> + (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
Missing after sizeof/offsetof/sizeof_field. And __alignof below.
> +int
> +__clone_internal (struct clone_args *cl_args,
> + int (*func) (void *arg), void *arg)
> +{
> + int ret;
> +#ifdef HAVE_CLONE3_WAPPER
> + /* Try clone3 first. */
> + int saved_errno = errno;
> + ret = __clone3 (cl_args, func, arg);
> + if (ret != -1 || errno != ENOSYS)
> + return ret;
*sigh* This will cause breakage in containers again. Like faccessat2.
I think this is technically the right thing to do.
> + /* NB: Restore errno since errno may be checked against non-zero
> + return value. */
> + __set_errno (saved_errno);
> +#else
> + /* Check invalid arguments. */
> + if (cl_args == NULL || func == NULL)
> + {
> + __set_errno (EINVAL);
> + return -1;
> + }
> +#endif
> +
> + /* Map clone3 arguments to clone arguments. NB: No need to check
> + invalid clone3 specific bits since this is an internal function. */
This comment contradicts with the check above under the #else.
Maybe the public clone3 wrapper should not have emulation. This would
push the EPERM problem to callers. (But it doesn't solve EPERM from
pthread_create of course.)
> diff --git a/sysdeps/unix/sysv/linux/clone3.c b/sysdeps/unix/sysv/linux/clone3.c
> new file mode 100644
> index 0000000000..de963ef89d
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/clone3.c
> @@ -0,0 +1 @@
> +/* An empty placeholder. */
> diff --git a/sysdeps/unix/sysv/linux/clone3.h b/sysdeps/unix/sysv/linux/clone3.h
> new file mode 100644
> index 0000000000..a222948d55
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/clone3.h
> +struct clone_args
> +{
> + uint64_t flags; /* Flags bit mask. */
> + uint64_t pidfd; /* Where to store PID file descriptor
> + (pid_t *). */
> + uint64_t child_tid; /* Where to store child TID, in child's memory
> + (pid_t *). */
> + uint64_t parent_tid; /* Where to store child TID, in parent's memory
> + (int *). */
> + uint64_t exit_signal; /* Signal to deliver to parent on child
> + termination */
> + uint64_t stack; /* The lowest address of stack. */
> + uint64_t stack_size; /* Size of stack. */
> + uint64_t tls; /* Location of new TLS. */
> + uint64_t set_tid; /* Pointer to a pid_t array
> + (since Linux 5.5). */
> + uint64_t set_tid_size; /* Number of elements in set_tid
> + (since Linux 5.5). */
> + uint64_t cgroup; /* File descriptor for target cgroup
> + of child (since Linux 5.7). */
> +} __attribute__ ((aligned (8)));
Usually, this kind of use of an ABI-changing attribute would not be
okay, but there is an expectation that the struct will be extended with
future fields in the future, so
I know that this is not an installed header yet. But would you please
add a comment to the end of the struct that new fields will be added in
the future, and that this struct should only be used in an argument to
clone3 (along with its size arguments) and not in a way that defines
some external ABI?
> +/* The wrapper of clone3. */
> +extern int clone3 (struct clone_args *__cl_args,
> + int (*__func) (void *__arg), void *__arg);
Sorry, the public clone3 system call wrapper will have to retain its
size argument. I didn't realize things were moeving in that direction.
I think __clone_internal should still avoid the size argument, but
__clone3 should have it (to align with public clone3).
Rest looks okay to me, thanks.
Florian
On Thu, May 20, 2021 at 7:46 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > diff --git a/include/clone_internal.h b/include/clone_internal.h
> > new file mode 100644
> > index 0000000000..124f7ba169
> > --- /dev/null
> > +++ b/include/clone_internal.h
> > @@ -0,0 +1,14 @@
> > +#ifndef _CLONE3_H
> > +#include_next <clone3.h>
> > +
> > +extern __typeof (clone3) __clone3;
> > +
> > +/* The internal wrapper of clone and clone3. */
> > +extern __typeof (clone3) __clone_internal;
>
> Maybe mention fallback explicitly?
Done.
> > diff --git a/include/libc-pointer-arith.h b/include/libc-pointer-arith.h
> > index 72e722c5aa..04ba537617 100644
> > --- a/include/libc-pointer-arith.h
> > +++ b/include/libc-pointer-arith.h
> > @@ -37,6 +37,9 @@
> > /* Cast an integer or a pointer VAL to integer with proper type. */
> > # define cast_to_integer(val) ((__integer_if_pointer_type (val)) (val))
> >
> > +/* Cast an integer VAL to void * pointer. */
> > +# define cast_to_pointer(val) ((void *) (uintptr_t) (val))
> > +
> > /* Align a value by rounding down to closest size.
> > e.g. Using size of 4096, we get this behavior:
> > {4095, 4096, 4097} = {0, 4096, 4096}. */
>
> As a regular backporter, I'd like to see this in a separate commit if
> possible.
Done.
> > diff --git a/sysdeps/unix/sysv/linux/clone-internal.c b/sysdeps/unix/sysv/linux/clone-internal.c
> > new file mode 100644
> > index 0000000000..c357b0ac14
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/clone-internal.c
>
> > +#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
> > +#define offsetofend(TYPE, MEMBER) \
> > + (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
>
> Missing after sizeof/offsetof/sizeof_field. And __alignof below.
Fixed.
> > +int
> > +__clone_internal (struct clone_args *cl_args,
> > + int (*func) (void *arg), void *arg)
> > +{
> > + int ret;
> > +#ifdef HAVE_CLONE3_WAPPER
> > + /* Try clone3 first. */
> > + int saved_errno = errno;
> > + ret = __clone3 (cl_args, func, arg);
> > + if (ret != -1 || errno != ENOSYS)
> > + return ret;
>
> *sigh* This will cause breakage in containers again. Like faccessat2.
>
> I think this is technically the right thing to do.
>
> > + /* NB: Restore errno since errno may be checked against non-zero
> > + return value. */
> > + __set_errno (saved_errno);
> > +#else
> > + /* Check invalid arguments. */
> > + if (cl_args == NULL || func == NULL)
> > + {
> > + __set_errno (EINVAL);
> > + return -1;
> > + }
This block is removed.
> > +#endif
> > +
> > + /* Map clone3 arguments to clone arguments. NB: No need to check
> > + invalid clone3 specific bits since this is an internal function. */
>
> This comment contradicts with the check above under the #else.
Fixed.
> Maybe the public clone3 wrapper should not have emulation. This would
> push the EPERM problem to callers. (But it doesn't solve EPERM from
> pthread_create of course.)
I don't think the public clone3 wrapper should have emulation.
> > diff --git a/sysdeps/unix/sysv/linux/clone3.c b/sysdeps/unix/sysv/linux/clone3.c
> > new file mode 100644
> > index 0000000000..de963ef89d
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/clone3.c
> > @@ -0,0 +1 @@
> > +/* An empty placeholder. */
> > diff --git a/sysdeps/unix/sysv/linux/clone3.h b/sysdeps/unix/sysv/linux/clone3.h
> > new file mode 100644
> > index 0000000000..a222948d55
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/clone3.h
>
> > +struct clone_args
> > +{
> > + uint64_t flags; /* Flags bit mask. */
> > + uint64_t pidfd; /* Where to store PID file descriptor
> > + (pid_t *). */
> > + uint64_t child_tid; /* Where to store child TID, in child's memory
> > + (pid_t *). */
> > + uint64_t parent_tid; /* Where to store child TID, in parent's memory
> > + (int *). */
> > + uint64_t exit_signal; /* Signal to deliver to parent on child
> > + termination */
> > + uint64_t stack; /* The lowest address of stack. */
> > + uint64_t stack_size; /* Size of stack. */
> > + uint64_t tls; /* Location of new TLS. */
> > + uint64_t set_tid; /* Pointer to a pid_t array
> > + (since Linux 5.5). */
> > + uint64_t set_tid_size; /* Number of elements in set_tid
> > + (since Linux 5.5). */
> > + uint64_t cgroup; /* File descriptor for target cgroup
> > + of child (since Linux 5.7). */
> > +} __attribute__ ((aligned (8)));
>
> Usually, this kind of use of an ABI-changing attribute would not be
> okay, but there is an expectation that the struct will be extended with
> future fields in the future, so
>
> I know that this is not an installed header yet. But would you please
> add a comment to the end of the struct that new fields will be added in
> the future, and that this struct should only be used in an argument to
> clone3 (along with its size arguments) and not in a way that defines
> some external ABI?
I added:
/* This struct should only be used in an argument to the clone3 system
call (along with its size argument). It may be extended with new
fields in the future. */
struct clone_args
{
...
> > +/* The wrapper of clone3. */
> > +extern int clone3 (struct clone_args *__cl_args,
> > + int (*__func) (void *__arg), void *__arg);
>
> Sorry, the public clone3 system call wrapper will have to retain its
> size argument. I didn't realize things were moeving in that direction.
> I think __clone_internal should still avoid the size argument, but
> __clone3 should have it (to align with public clone3).
I added the size argument to the clone3 wrapper.
> Rest looks okay to me, thanks.
>
> Florian
>
Thanks.
new file mode 100644
@@ -0,0 +1,14 @@
+#ifndef _CLONE3_H
+#include_next <clone3.h>
+
+extern __typeof (clone3) __clone3;
+
+/* The internal wrapper of clone and clone3. */
+extern __typeof (clone3) __clone_internal;
+
+#ifndef _ISOMAC
+libc_hidden_proto (__clone3)
+libc_hidden_proto (__clone_internal)
+#endif
+
+#endif
@@ -37,6 +37,9 @@
/* Cast an integer or a pointer VAL to integer with proper type. */
# define cast_to_integer(val) ((__integer_if_pointer_type (val)) (val))
+/* Cast an integer VAL to void * pointer. */
+# define cast_to_pointer(val) ((void *) (uintptr_t) (val))
+
/* Align a value by rounding down to closest size.
e.g. Using size of 4096, we get this behavior:
{4095, 4096, 4097} = {0, 4096, 4096}. */
@@ -54,6 +54,8 @@ CFLAGS-malloc.c += -DMORECORE_CLEARS=2
endif
ifeq ($(subdir),misc)
+gen-as-const-headers += clone-offsets.sym
+
sysdep_routines += adjtimex clone umount umount2 readahead sysctl \
setfsuid setfsgid epoll_pwait signalfd \
eventfd eventfd_read eventfd_write prlimit \
@@ -64,7 +66,7 @@ sysdep_routines += adjtimex clone umount umount2 readahead sysctl \
time64-support pselect32 \
xstat fxstat lxstat xstat64 fxstat64 lxstat64 \
fxstatat fxstatat64 \
- xmknod xmknodat
+ xmknod xmknodat clone3 clone-internal
CFLAGS-gethostid.c = -fexceptions
CFLAGS-tee.c = -fexceptions -fasynchronous-unwind-tables
new file mode 100644
@@ -0,0 +1,97 @@
+/* The internal wrapper of clone and clone3.
+ Copyright (C) 2021 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 <sysdep.h>
+#include <stddef.h>
+#include <errno.h>
+#include <sched.h>
+#include <clone_internal.h>
+#include <libc-pointer-arith.h> /* For cast_to_pointer. */
+#include <stackinfo.h> /* For _STACK_GROWS_{UP,DOWN}. */
+
+#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
+#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */
+#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
+
+#define sizeof_field(TYPE, MEMBER) sizeof((((TYPE *)0)->MEMBER))
+#define offsetofend(TYPE, MEMBER) \
+ (offsetof(TYPE, MEMBER) + sizeof_field(TYPE, MEMBER))
+
+_Static_assert (__alignof(struct clone_args) == 8,
+ "__alignof(struct clone_args) != 8");
+_Static_assert (offsetofend(struct clone_args, tls) == CLONE_ARGS_SIZE_VER0,
+ "offsetofend(struct clone_args, tls) != CLONE_ARGS_SIZE_VER0");
+_Static_assert (offsetofend(struct clone_args, set_tid_size) == CLONE_ARGS_SIZE_VER1,
+ "offsetofend(struct clone_args, set_tid_size) != CLONE_ARGS_SIZE_VER1");
+_Static_assert (offsetofend(struct clone_args, cgroup) == CLONE_ARGS_SIZE_VER2,
+ "offsetofend(struct clone_args, cgroup) != CLONE_ARGS_SIZE_VER2");
+_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)
+{
+ int ret;
+#ifdef HAVE_CLONE3_WAPPER
+ /* Try clone3 first. */
+ int saved_errno = errno;
+ ret = __clone3 (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);
+#else
+ /* Check invalid arguments. */
+ if (cl_args == NULL || func == NULL)
+ {
+ __set_errno (EINVAL);
+ return -1;
+ }
+#endif
+
+ /* Map clone3 arguments to clone arguments. NB: No need to check
+ invalid clone3 specific bits since this is an internal function. */
+ int flags = cl_args->flags | cl_args->exit_signal;
+ void *stack = cast_to_pointer (cl_args->stack);
+
+#ifdef __ia64__
+ ret = __clone2 (func, stack, cl_args->stack_size,
+ flags, arg,
+ cast_to_pointer (cl_args->parent_tid),
+ cast_to_pointer (cl_args->tls),
+ cast_to_pointer (cl_args->child_tid));
+#else
+# if !_STACK_GROWS_DOWN && !_STACK_GROWS_UP
+# error "Define either _STACK_GROWS_DOWN or _STACK_GROWS_UP"
+# endif
+
+# if _STACK_GROWS_DOWN
+ stack += cl_args->stack_size;
+# endif
+ ret = __clone (func, stack, flags, arg,
+ cast_to_pointer (cl_args->parent_tid),
+ cast_to_pointer (cl_args->tls),
+ cast_to_pointer (cl_args->child_tid));
+#endif
+ return ret;
+}
+
+libc_hidden_def (__clone_internal)
new file mode 100644
@@ -0,0 +1,5 @@
+#include <clone3.h>
+
+--
+
+CLONE_ARGS_SIZE sizeof (struct clone_args)
new file mode 100644
@@ -0,0 +1 @@
+/* An empty placeholder. */
new file mode 100644
@@ -0,0 +1,55 @@
+/* The wrapper of clone3.
+ Copyright (C) 2021 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/>. */
+
+#ifndef _CLONE3_H
+#define _CLONE3_H 1
+
+#include <features.h>
+#include <stdint.h>
+
+__BEGIN_DECLS
+
+struct clone_args
+{
+ uint64_t flags; /* Flags bit mask. */
+ uint64_t pidfd; /* Where to store PID file descriptor
+ (pid_t *). */
+ uint64_t child_tid; /* Where to store child TID, in child's memory
+ (pid_t *). */
+ uint64_t parent_tid; /* Where to store child TID, in parent's memory
+ (int *). */
+ uint64_t exit_signal; /* Signal to deliver to parent on child
+ termination */
+ uint64_t stack; /* The lowest address of stack. */
+ uint64_t stack_size; /* Size of stack. */
+ uint64_t tls; /* Location of new TLS. */
+ uint64_t set_tid; /* Pointer to a pid_t array
+ (since Linux 5.5). */
+ uint64_t set_tid_size; /* Number of elements in set_tid
+ (since Linux 5.5). */
+ uint64_t cgroup; /* File descriptor for target cgroup
+ of child (since Linux 5.7). */
+} __attribute__ ((aligned (8)));
+
+/* The wrapper of clone3. */
+extern int clone3 (struct clone_args *__cl_args,
+ int (*__func) (void *__arg), void *__arg);
+
+__END_DECLS
+
+#endif /* clone3.h */
@@ -25,15 +25,10 @@
#include <ldsodefs.h>
#include <tls.h>
#include <stdint.h>
+#include <clone_internal.h>
#include <arch-fork.h>
-#ifdef __NR_clone2
-# define ARCH_CLONE __clone2
-#else
-# define ARCH_CLONE __clone
-#endif
-
/* See the comments in pthread_create.c for the requirements for these
two macros and the create_thread function. */
@@ -47,7 +42,8 @@ static int start_thread (void *arg) __attribute__ ((noreturn));
static int
create_thread (struct pthread *pd, const struct pthread_attr *attr,
- bool *stopped_start, STACK_VARIABLES_PARMS, bool *thread_ran)
+ bool *stopped_start, void *stackaddr, size_t stacksize,
+ bool *thread_ran)
{
/* Determine whether the newly created threads has to be started
stopped since we have to set the scheduling parameters or set the
@@ -100,9 +96,18 @@ create_thread (struct pthread *pd, const struct pthread_attr *attr,
TLS_DEFINE_INIT_TP (tp, pd);
- if (__glibc_unlikely (ARCH_CLONE (&start_thread, STACK_VARIABLES_ARGS,
- clone_flags, pd, &pd->tid, tp, &pd->tid)
- == -1))
+ struct clone_args args =
+ {
+ .flags = clone_flags,
+ .pidfd = (uintptr_t) &pd->tid,
+ .parent_tid = (uintptr_t) &pd->tid,
+ .child_tid = (uintptr_t) &pd->tid,
+ .stack = (uintptr_t) stackaddr,
+ .stack_size = stacksize,
+ .tls = (uintptr_t) tp,
+ };
+ int ret = __clone_internal (&args, &start_thread, pd);
+ if (__glibc_unlikely (ret == -1))
return errno;
/* It's started now, so if we fail below, we'll have to cancel it
@@ -31,6 +31,7 @@
#include <dl-sysdep.h>
#include <libc-pointer-arith.h>
#include <ldsodefs.h>
+#include <clone_internal.h>
#include "spawn_int.h"
/* The Linux implementation of posix_spawn{p} uses the clone syscall directly
@@ -59,21 +60,6 @@
normal program exit with the exit code 127. */
#define SPAWN_ERROR 127
-#ifdef __ia64__
-# define CLONE(__fn, __stackbase, __stacksize, __flags, __args) \
- __clone2 (__fn, __stackbase, __stacksize, __flags, __args, 0, 0, 0)
-#else
-# define CLONE(__fn, __stack, __stacksize, __flags, __args) \
- __clone (__fn, __stack, __flags, __args)
-#endif
-
-/* Since ia64 wants the stackbase w/clone2, re-use the grows-up macro. */
-#if _STACK_GROWS_UP || defined (__ia64__)
-# define STACK(__stack, __stack_size) (__stack)
-#elif _STACK_GROWS_DOWN
-# define STACK(__stack, __stack_size) (__stack + __stack_size)
-#endif
-
struct posix_spawn_args
{
@@ -378,8 +364,14 @@ __spawnix (pid_t * pid, const char *file,
need for CLONE_SETTLS. Although parent and child share the same TLS
namespace, there will be no concurrent access for TLS variables (errno
for instance). */
- new_pid = CLONE (__spawni_child, STACK (stack, stack_size), stack_size,
- CLONE_VM | CLONE_VFORK | SIGCHLD, &args);
+ struct clone_args clone_args =
+ {
+ .flags = CLONE_VM | CLONE_VFORK,
+ .exit_signal = SIGCHLD,
+ .stack = (uintptr_t) stack,
+ .stack_size = stack_size,
+ };
+ new_pid = __clone_internal (&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