Re: [PATCH v5 1/5] Add an internal wrapper for clone, clone2 and clone3

[Florian Weimer <fweimer@redhat.com>]

* 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