From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x32a.google.com (mail-ot1-x32a.google.com [IPv6:2607:f8b0:4864:20::32a]) by sourceware.org (Postfix) with ESMTPS id 12AE63857C7B for ; Sat, 22 May 2021 01:15:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 12AE63857C7B Received: by mail-ot1-x32a.google.com with SMTP id d25-20020a0568300459b02902f886f7dd43so19689914otc.6 for ; Fri, 21 May 2021 18:15:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=JBWBF30/R/V07DV0kzG0HUNxTjASUMnVSQSIm+csgHQ=; b=hxRUSjKfSNGhG7QkEGHApq2HngTA/Tfk/DzCdcq3cwO0FlRItk7DyFblWAnHt7heXx r4I8+LYaIr2w4M7JDOxAfSHrw07GQQPK7ZLgQz9rzT/VEvKG1JYbyct1Hl8DH3zZgRuX 2X1hVhOOWJS2VocKXU+38JRJnNhHfDIhijlv6z2MzJS5gMSjAePZ/7tZ+g57DoW+U69w O2Fm3UcyQtaDkYxbqlZ6zQwKVJ9tBIx4fFHXVCZG3nj7AKeowX9U0fRHdMyIKntFy4zZ qB7TUau5IvxZzzsKeOsscjbI1KJE+H4/rayMLZehLIfV/jndu6Fejup9AYsyvdTEt1S9 gpPA== X-Gm-Message-State: AOAM531GQEuM/HIP+h0ILp35VZ9CaVC3BatViLBq5hjeRelSryqtXljo fsdtveMUvaP6pFzg15ny69Xx4FAoBGZ38qQOmIQ= X-Google-Smtp-Source: ABdhPJyi1ajj6mxpjUX0z6cFnaffL7edrPPfXnA30AU3LScHbW923lGqym7PwK+N3vO6R2LMGODEkLWjmebyi7jPJsM= X-Received: by 2002:a9d:67cf:: with SMTP id c15mr10472080otn.285.1621646128353; Fri, 21 May 2021 18:15:28 -0700 (PDT) MIME-Version: 1.0 References: <20210515123442.1432385-1-hjl.tools@gmail.com> <20210515123442.1432385-2-hjl.tools@gmail.com> <8735uho43v.fsf@oldenburg.str.redhat.com> In-Reply-To: <8735uho43v.fsf@oldenburg.str.redhat.com> From: "H.J. Lu" Date: Fri, 21 May 2021 18:14:52 -0700 Message-ID: Subject: Re: [PATCH v5 1/5] Add an internal wrapper for clone, clone2 and clone3 To: Florian Weimer Cc: GNU C Library , Adhemerval Zanella Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3034.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_NUMSUBJECT, RCVD_IN_DNSWL_NONE, 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: Sat, 22 May 2021 01:15:30 -0000 On Thu, May 20, 2021 at 7:46 AM Florian Weimer 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 > > + > > +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. -- H.J.