From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 04F2738515EB for ; Thu, 20 May 2021 14:46:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 04F2738515EB Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-227-_CVxf3WvPdeTAQQ6vLzrtw-1; Thu, 20 May 2021 10:46:17 -0400 X-MC-Unique: _CVxf3WvPdeTAQQ6vLzrtw-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id F0E00801106; Thu, 20 May 2021 14:46:15 +0000 (UTC) Received: from oldenburg.str.redhat.com (ovpn-112-137.ams2.redhat.com [10.36.112.137]) by smtp.corp.redhat.com (Postfix) with ESMTPS id B75D96062F; Thu, 20 May 2021 14:46:14 +0000 (UTC) From: Florian Weimer To: "H.J. Lu" Cc: libc-alpha@sourceware.org, Adhemerval Zanella Subject: Re: [PATCH v5 1/5] Add an internal wrapper for clone, clone2 and clone3 References: <20210515123442.1432385-1-hjl.tools@gmail.com> <20210515123442.1432385-2-hjl.tools@gmail.com> Date: Thu, 20 May 2021 16:46:12 +0200 In-Reply-To: <20210515123442.1432385-2-hjl.tools@gmail.com> (H. J. Lu's message of "Sat, 15 May 2021 05:34:38 -0700") Message-ID: <8735uho43v.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_NUMSUBJECT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 May 2021 14:46:22 -0000 * 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 > + > +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