[PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[Christian Brauner]

Reset all signal handlers of the child not set to SIG_IGN to SIG_DFL.
Mutually exclusive with CLONE_SIGHAND to not disturb other thread's
signal handler.

In the spirit of closer cooperation between glibc developers and kernel
developers (cf. [2]) this patchset came out of a discussion on the glibc
mailing list for improving posix_spawn() (cf. [1], [3], [4]). Kernel
support for this feature has been explicitly requested by glibc and I
see no reason not to help them with this.

The child helper process on Linux posix_spawn must ensure that no signal
handlers are enabled, so the signal disposition must be either SIG_DFL
or SIG_IGN. However, it requires a sigprocmask to obtain the current
signal mask and at least _NSIG sigaction calls to reset the signal
handlers for each posix_spawn call or complex state tracking that might
lead to data corruption in glibc. Adding this flags lets glibc avoid
these problems.

[1]: https://www.sourceware.org/ml/libc-alpha/2019-10/msg00149.html
[3]: https://www.sourceware.org/ml/libc-alpha/2019-10/msg00158.html
[4]: https://www.sourceware.org/ml/libc-alpha/2019-10/msg00160.html
[2]: https://lwn.net/Articles/799331/
     '[...] by asking for better cooperation with the C-library projects
     in general. They should be copied on patches containing ABI
     changes, for example. I noted that there are often times where
     C-library developers wish the kernel community had done things
     differently; how could those be avoided in the future? Members of
     the audience suggested that more glibc developers should perhaps
     join the linux-api list. The other suggestion was to "copy Florian
     on everything".'
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Florian Weimer <fweimer@redhat.com>
Cc: libc-alpha@sourceware.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 include/uapi/linux/sched.h |  3 +++
 kernel/fork.c              | 11 ++++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
index 99335e1f4a27..c583720f689f 100644
--- a/include/uapi/linux/sched.h
+++ b/include/uapi/linux/sched.h
@@ -33,6 +33,9 @@
 #define CLONE_NEWNET		0x40000000	/* New network namespace */
 #define CLONE_IO		0x80000000	/* Clone io context */
 
+/* Flags for the clone3() syscall */
+#define CLONE3_CLEAR_SIGHAND 0x100000000ULL /* Clear any signal handler and reset to SIG_DFL. */
+
 #ifndef __ASSEMBLY__
 /**
  * struct clone_args - arguments for the clone3 syscall
diff --git a/kernel/fork.c b/kernel/fork.c
index 1f6c45f6a734..661f8d1f3881 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1517,6 +1517,11 @@ static int copy_sighand(unsigned long clone_flags, struct task_struct *tsk)
 	spin_lock_irq(&current->sighand->siglock);
 	memcpy(sig->action, current->sighand->action, sizeof(sig->action));
 	spin_unlock_irq(&current->sighand->siglock);
+
+	/* Reset all signal handler not set to SIG_IGN to SIG_DFL. */
+	if (clone_flags & CLONE3_CLEAR_SIGHAND)
+		flush_signal_handlers(tsk, 0);
+
 	return 0;
 }
 
@@ -2567,7 +2572,7 @@ static bool clone3_args_valid(const struct kernel_clone_args *kargs)
 	 * All lower bits of the flag word are taken.
 	 * Verify that no other unknown flags are passed along.
 	 */
-	if (kargs->flags & ~CLONE_LEGACY_FLAGS)
+	if (kargs->flags & ~(CLONE_LEGACY_FLAGS | CLONE3_CLEAR_SIGHAND))
 		return false;
 
 	/*
@@ -2577,6 +2582,10 @@ static bool clone3_args_valid(const struct kernel_clone_args *kargs)
 	if (kargs->flags & (CLONE_DETACHED | CSIGNAL))
 		return false;
 
+	if ((kargs->flags & (CLONE_SIGHAND | CLONE3_CLEAR_SIGHAND)) ==
+	    (CLONE_SIGHAND | CLONE3_CLEAR_SIGHAND))
+		return false;
+
 	if ((kargs->flags & (CLONE_THREAD | CLONE_PARENT)) &&
 	    kargs->exit_signal)
 		return false;
-- 
2.23.0

[PATCH 2/2] tests: test CLONE3_CLEAR_SIGHAND

[Christian Brauner]

Test that CLONE3_CLEAR_SIGHAND resets signal handlers to SIG_DFL for the
child process and that CLONE3_CLEAR_SIGHAND and CLONE_SIGHAND are
mutually exclusive.

Cc: Florian Weimer <fweimer@redhat.com>
Cc: libc-alpha@sourceware.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 MAINTAINERS                                   |   1 +
 tools/testing/selftests/Makefile              |   1 +
 tools/testing/selftests/clone3/.gitignore     |   1 +
 tools/testing/selftests/clone3/Makefile       |   7 +
 .../selftests/clone3/clone3_clear_sighand.c   | 171 ++++++++++++++++++
 5 files changed, 181 insertions(+)
 create mode 100644 tools/testing/selftests/clone3/.gitignore
 create mode 100644 tools/testing/selftests/clone3/Makefile
 create mode 100644 tools/testing/selftests/clone3/clone3_clear_sighand.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 55199ef7fa74..582275d85607 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12828,6 +12828,7 @@ S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git
 F:	samples/pidfd/
 F:	tools/testing/selftests/pidfd/
+F:	tools/testing/selftests/clone3/
 K:	(?i)pidfd
 K:	(?i)clone3
 K:	\b(clone_args|kernel_clone_args)\b
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index c3feccb99ff5..6bf7aeb47650 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -4,6 +4,7 @@ TARGETS += bpf
 TARGETS += breakpoints
 TARGETS += capabilities
 TARGETS += cgroup
+TARGETS += clone3
 TARGETS += cpufreq
 TARGETS += cpu-hotplug
 TARGETS += drivers/dma-buf
diff --git a/tools/testing/selftests/clone3/.gitignore b/tools/testing/selftests/clone3/.gitignore
new file mode 100644
index 000000000000..6c9f98097774
--- /dev/null
+++ b/tools/testing/selftests/clone3/.gitignore
@@ -0,0 +1 @@
+clone3_clear_sighand
diff --git a/tools/testing/selftests/clone3/Makefile b/tools/testing/selftests/clone3/Makefile
new file mode 100644
index 000000000000..3ecd56ebc99d
--- /dev/null
+++ b/tools/testing/selftests/clone3/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-only
+CFLAGS += -g -I../../../../usr/include/
+
+TEST_GEN_PROGS := clone3_clear_sighand
+
+include ../lib.mk
+
diff --git a/tools/testing/selftests/clone3/clone3_clear_sighand.c b/tools/testing/selftests/clone3/clone3_clear_sighand.c
new file mode 100644
index 000000000000..0e38c06dd6eb
--- /dev/null
+++ b/tools/testing/selftests/clone3/clone3_clear_sighand.c
@@ -0,0 +1,171 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <sched.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <linux/sched.h>
+#include <linux/types.h>
+#include <sys/syscall.h>
+#include <sys/wait.h>
+
+#include "../kselftest.h"
+
+#ifndef CLONE3_CLEAR_SIGHAND
+#define CLONE3_CLEAR_SIGHAND 0x100000000ULL
+#endif
+
+#ifndef __NR_clone3
+#define __NR_clone3 -1
+struct clone_args {
+	__aligned_u64 flags;
+	__aligned_u64 pidfd;
+	__aligned_u64 child_tid;
+	__aligned_u64 parent_tid;
+	__aligned_u64 exit_signal;
+	__aligned_u64 stack;
+	__aligned_u64 stack_size;
+	__aligned_u64 tls;
+};
+#endif
+
+static pid_t sys_clone3(struct clone_args *args, size_t size)
+{
+	return syscall(__NR_clone3, args, size);
+}
+
+static void test_clone3_supported(void)
+{
+	pid_t pid;
+	struct clone_args args = {};
+
+	if (__NR_clone3 < 0)
+		ksft_exit_skip("clone3() syscall is not supported\n");
+
+	/* Set to something that will always cause EINVAL. */
+	args.exit_signal = -1;
+	pid = sys_clone3(&args, sizeof(args));
+	if (!pid)
+		exit(EXIT_SUCCESS);
+
+	if (pid > 0) {
+		wait(NULL);
+		ksft_exit_fail_msg(
+			"Managed to create child process with invalid exit_signal\n");
+	}
+
+	if (errno == ENOSYS)
+		ksft_exit_skip("clone3() syscall is not supported\n");
+
+	ksft_print_msg("clone3() syscall supported\n");
+}
+
+static void nop_handler(int signo)
+{
+}
+
+static int wait_for_pid(pid_t pid)
+{
+	int status, ret;
+
+again:
+	ret = waitpid(pid, &status, 0);
+	if (ret == -1) {
+		if (errno == EINTR)
+			goto again;
+
+		return -1;
+	}
+
+	if (!WIFEXITED(status))
+		return -1;
+
+	return WEXITSTATUS(status);
+}
+
+static void test_clone3_clear_sighand(void)
+{
+	int ret;
+	pid_t pid;
+	struct clone_args args = {};
+	struct sigaction new_action, old_action;
+
+	new_action.sa_handler = nop_handler;
+	ret = sigemptyset(&new_action.sa_mask);
+	if (ret < 0)
+		ksft_exit_fail_msg("%s - sigemptyset() failed\n",
+				   strerror(errno));
+
+	new_action.sa_flags = 0;
+
+	ret = sigaction(SIGUSR1, &new_action, NULL);
+	if (ret < 0)
+		ksft_exit_fail_msg(
+			"%s - sigaction(SIGUSR1, &new_action, NULL) failed\n",
+			strerror(errno));
+
+	ret = sigaction(SIGUSR2, &new_action, NULL);
+	if (ret < 0)
+		ksft_exit_fail_msg(
+			"%s - sigaction(SIGUSR2, &new_action, NULL) failed\n",
+			strerror(errno));
+
+	/*
+	 * Check that CLONE3_CLEAR_SIGHAND and CLONE_SIGHAND are mutually
+	 * exclusive.
+	 */
+	args.flags |= CLONE3_CLEAR_SIGHAND | CLONE_SIGHAND;
+	args.exit_signal = SIGCHLD;
+	pid = sys_clone3(&args, sizeof(args));
+	if (pid > 0)
+		ksft_exit_fail_msg(
+			"clone3(CLONE3_CLEAR_SIGHAND | CLONE_SIGHAND) succeeded\n");
+
+	/* Check that CLONE3_CLEAR_SIGHAND works. */
+	args.flags = CLONE3_CLEAR_SIGHAND;
+	pid = sys_clone3(&args, sizeof(args));
+	if (pid < 0)
+		ksft_exit_fail_msg("%s - clone3(CLONE3_CLEAR_SIGHAND) failed\n", strerror(errno));
+
+	if (pid == 0) {
+		struct sigaction query_action;
+
+		ret = sigaction(SIGUSR1, NULL, &query_action);
+		if (ret < 0)
+			exit(EXIT_FAILURE);
+
+		if (query_action.sa_handler != SIG_DFL)
+			exit(EXIT_FAILURE);
+
+		ret = sigaction(SIGUSR2, NULL, &query_action);
+		if (ret < 0)
+			exit(EXIT_FAILURE);
+
+		if (query_action.sa_handler != SIG_DFL)
+			exit(EXIT_FAILURE);
+
+		exit(EXIT_SUCCESS);
+	}
+
+	ret = wait_for_pid(pid);
+	if (ret)
+		ksft_exit_fail_msg(
+			"Failed to clear signal handler for child process\n");
+
+	ksft_test_result_pass("Cleared signal handlers for child process\n");
+}
+
+int main(int argc, char **argv)
+{
+	ksft_print_header();
+	ksft_set_plan(1);
+
+	test_clone3_supported();
+	test_clone3_clear_sighand();
+
+	return ksft_exit_pass();
+}
-- 
2.23.0

Re: [PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[Florian Weimer]

* Christian Brauner:

> @@ -2567,7 +2572,7 @@ static bool clone3_args_valid(const struct kernel_clone_args *kargs)
>  	 * All lower bits of the flag word are taken.
>  	 * Verify that no other unknown flags are passed along.
>  	 */
> -	if (kargs->flags & ~CLONE_LEGACY_FLAGS)
> +	if (kargs->flags & ~(CLONE_LEGACY_FLAGS | CLONE3_CLEAR_SIGHAND))
>  		return false;

Does the comment need updating?  I feel it's a bit misleading now.

Thanks,
Florian

Re: [PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[Christian Brauner]

On Thu, Oct 10, 2019 at 04:19:44PM +0200, Florian Weimer wrote:
> * Christian Brauner:
> 
> > @@ -2567,7 +2572,7 @@ static bool clone3_args_valid(const struct kernel_clone_args *kargs)
> >  	 * All lower bits of the flag word are taken.
> >  	 * Verify that no other unknown flags are passed along.
> >  	 */
> > -	if (kargs->flags & ~CLONE_LEGACY_FLAGS)
> > +	if (kargs->flags & ~(CLONE_LEGACY_FLAGS | CLONE3_CLEAR_SIGHAND))
> >  		return false;
> 
> Does the comment need updating?  I feel it's a bit misleading now.

Yeah, maybe just:

/* Verify that no unknown flags are passed along. */

?

Thanks!
Christian

Re: [PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[Florian Weimer]

* Christian Brauner:

> On Thu, Oct 10, 2019 at 04:19:44PM +0200, Florian Weimer wrote:
>> * Christian Brauner:
>> 
>> > @@ -2567,7 +2572,7 @@ static bool clone3_args_valid(const struct kernel_clone_args *kargs)
>> >  	 * All lower bits of the flag word are taken.
>> >  	 * Verify that no other unknown flags are passed along.
>> >  	 */
>> > -	if (kargs->flags & ~CLONE_LEGACY_FLAGS)
>> > +	if (kargs->flags & ~(CLONE_LEGACY_FLAGS | CLONE3_CLEAR_SIGHAND))
>> >  		return false;
>> 
>> Does the comment need updating?  I feel it's a bit misleading now.
>
> Yeah, maybe just:
>
> /* Verify that no unknown flags are passed along. */
>
> ?

Sure, looks fine to me.

Thanks,
Florian

Re: [PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[Michal Hocko]

[Cc linux-api]

On Thu 10-10-19 15:35:17, Christian Brauner wrote:
> Reset all signal handlers of the child not set to SIG_IGN to SIG_DFL.
> Mutually exclusive with CLONE_SIGHAND to not disturb other thread's
> signal handler.
> 
> In the spirit of closer cooperation between glibc developers and kernel
> developers (cf. [2]) this patchset came out of a discussion on the glibc
> mailing list for improving posix_spawn() (cf. [1], [3], [4]). Kernel
> support for this feature has been explicitly requested by glibc and I
> see no reason not to help them with this.
> 
> The child helper process on Linux posix_spawn must ensure that no signal
> handlers are enabled, so the signal disposition must be either SIG_DFL
> or SIG_IGN. However, it requires a sigprocmask to obtain the current
> signal mask and at least _NSIG sigaction calls to reset the signal
> handlers for each posix_spawn call or complex state tracking that might
> lead to data corruption in glibc. Adding this flags lets glibc avoid
> these problems.
> 
> [1]: https://www.sourceware.org/ml/libc-alpha/2019-10/msg00149.html
> [3]: https://www.sourceware.org/ml/libc-alpha/2019-10/msg00158.html
> [4]: https://www.sourceware.org/ml/libc-alpha/2019-10/msg00160.html
> [2]: https://lwn.net/Articles/799331/
>      '[...] by asking for better cooperation with the C-library projects
>      in general. They should be copied on patches containing ABI
>      changes, for example. I noted that there are often times where
>      C-library developers wish the kernel community had done things
>      differently; how could those be avoided in the future? Members of
>      the audience suggested that more glibc developers should perhaps
>      join the linux-api list. The other suggestion was to "copy Florian
>      on everything".'
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Florian Weimer <fweimer@redhat.com>
> Cc: libc-alpha@sourceware.org
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  include/uapi/linux/sched.h |  3 +++
>  kernel/fork.c              | 11 ++++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
> index 99335e1f4a27..c583720f689f 100644
> --- a/include/uapi/linux/sched.h
> +++ b/include/uapi/linux/sched.h
> @@ -33,6 +33,9 @@
>  #define CLONE_NEWNET		0x40000000	/* New network namespace */
>  #define CLONE_IO		0x80000000	/* Clone io context */
>  
> +/* Flags for the clone3() syscall */
> +#define CLONE3_CLEAR_SIGHAND 0x100000000ULL /* Clear any signal handler and reset to SIG_DFL. */
> +
>  #ifndef __ASSEMBLY__
>  /**
>   * struct clone_args - arguments for the clone3 syscall
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 1f6c45f6a734..661f8d1f3881 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1517,6 +1517,11 @@ static int copy_sighand(unsigned long clone_flags, struct task_struct *tsk)
>  	spin_lock_irq(&current->sighand->siglock);
>  	memcpy(sig->action, current->sighand->action, sizeof(sig->action));
>  	spin_unlock_irq(&current->sighand->siglock);
> +
> +	/* Reset all signal handler not set to SIG_IGN to SIG_DFL. */
> +	if (clone_flags & CLONE3_CLEAR_SIGHAND)
> +		flush_signal_handlers(tsk, 0);
> +
>  	return 0;
>  }
>  
> @@ -2567,7 +2572,7 @@ static bool clone3_args_valid(const struct kernel_clone_args *kargs)
>  	 * All lower bits of the flag word are taken.
>  	 * Verify that no other unknown flags are passed along.
>  	 */
> -	if (kargs->flags & ~CLONE_LEGACY_FLAGS)
> +	if (kargs->flags & ~(CLONE_LEGACY_FLAGS | CLONE3_CLEAR_SIGHAND))
>  		return false;
>  
>  	/*
> @@ -2577,6 +2582,10 @@ static bool clone3_args_valid(const struct kernel_clone_args *kargs)
>  	if (kargs->flags & (CLONE_DETACHED | CSIGNAL))
>  		return false;
>  
> +	if ((kargs->flags & (CLONE_SIGHAND | CLONE3_CLEAR_SIGHAND)) ==
> +	    (CLONE_SIGHAND | CLONE3_CLEAR_SIGHAND))
> +		return false;
> +
>  	if ((kargs->flags & (CLONE_THREAD | CLONE_PARENT)) &&
>  	    kargs->exit_signal)
>  		return false;
> -- 
> 2.23.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[Christian Brauner]

On Fri, Oct 11, 2019 at 10:21:18AM +0200, Michal Hocko wrote:
> [Cc linux-api]

Right, thanks Michal.
Christian

Re: [PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[Michael Kerrisk]

Hello Christian,

Why CLONE3_CLEAR_SIGHAND rather than just CLONE_CLEAR_SIGHAND?

Thanks,

Michael

Re: [PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 382 bytes --]

On 2019-10-11, Michael Kerrisk <mtk.manpages@gmail.com> wrote:
> Why CLONE3_CLEAR_SIGHAND rather than just CLONE_CLEAR_SIGHAND?

There are no more flag bits left for the classic clone()/clone2() (the
last one was used up by CLONE_PIDFD) -- thus this flag is clone3()-only.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[Michael Kerrisk (man-pages)]

Hello Aleksa,

On Sat, 12 Oct 2019 at 00:12, Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> On 2019-10-11, Michael Kerrisk <mtk.manpages@gmail.com> wrote:
> > Why CLONE3_CLEAR_SIGHAND rather than just CLONE_CLEAR_SIGHAND?
>
> There are no more flag bits left for the classic clone()/clone2() (the
> last one was used up by CLONE_PIDFD) -- thus this flag is clone3()-only.

Yes, I understand that. But, I'm not sure that the "3" in the prefix
is necessary. "CLONE_" still seems better to me.

Consider this: sometime in the near future we will probably have time
namespaces. The new flag for those namespaces will only be usable with
clone3(). It should NOT be called CLONE3_NEWTIME, but rather
CLONE_NEWTIME (or similar), because that same flag will presumably
also be used in other APIs such as unshare() and setns(). (Hmm -- I
wonder if we are going to need a new unshare2() or some such...)

Thanks,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[Christian Brauner]

On Sat, Oct 12, 2019 at 08:53:34AM +0200, Michael Kerrisk (man-pages) wrote:
> Hello Aleksa,
> 
> On Sat, 12 Oct 2019 at 00:12, Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > On 2019-10-11, Michael Kerrisk <mtk.manpages@gmail.com> wrote:
> > > Why CLONE3_CLEAR_SIGHAND rather than just CLONE_CLEAR_SIGHAND?

I don't care much how we name this apart from the "_CLEAR_SIGHAND"
suffix. But see for a little rationale below.

> >
> > There are no more flag bits left for the classic clone()/clone2() (the
> > last one was used up by CLONE_PIDFD) -- thus this flag is clone3()-only.
> 
> Yes, I understand that. But, I'm not sure that the "3" in the prefix
> is necessary. "CLONE_" still seems better to me.
> 
> Consider this: sometime in the near future we will probably have time
> namespaces. The new flag for those namespaces will only be usable with
> clone3(). It should NOT be called CLONE3_NEWTIME, but rather
> CLONE_NEWTIME (or similar), because that same flag will presumably
> also be used in other APIs such as unshare() and setns(). (Hmm -- I

There are some noteable differences though. CLONE_NEWTIME takes the
CSIGNAL bit which is in the range of a 32bit integer and thus useable by
unshare() too. The same does not hold for CLONE{3}_CLEAR_SIGHAND. You
can't pass it to unshare(). unshare() also just deals with
namespace-relevant stuff so CLONE{3}_CLEAR_SIGHAND doesn't make much
sense there.

> wonder if we are going to need a new unshare2() or some such...)

We still have one 32bit bit left (CLONE_DETACHED) which we can't reuse
with clone()/clone2() but we can reuse with clone3(). We can simply
earmark it for namespace-related stuff and thus still have one bit left
for unshare() before we have to go for unshare2() (If we have to go
there at all since I'm not sure how much more namespaces we can come up
with.).

Christian

Re: [PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[Michael Kerrisk (man-pages)]

On 10/12/19 9:48 AM, Christian Brauner wrote:
> On Sat, Oct 12, 2019 at 08:53:34AM +0200, Michael Kerrisk (man-pages) wrote:
>> Hello Aleksa,
>>
>> On Sat, 12 Oct 2019 at 00:12, Aleksa Sarai <cyphar@cyphar.com> wrote:
>>>
>>> On 2019-10-11, Michael Kerrisk <mtk.manpages@gmail.com> wrote:
>>>> Why CLONE3_CLEAR_SIGHAND rather than just CLONE_CLEAR_SIGHAND?
> 
> I don't care much how we name this apart from the "_CLEAR_SIGHAND"
> suffix. But see for a little rationale below.
> 
>>>
>>> There are no more flag bits left for the classic clone()/clone2() (the
>>> last one was used up by CLONE_PIDFD) -- thus this flag is clone3()-only.
>>
>> Yes, I understand that. But, I'm not sure that the "3" in the prefix
>> is necessary. "CLONE_" still seems better to me.
>>
>> Consider this: sometime in the near future we will probably have time
>> namespaces. The new flag for those namespaces will only be usable with
>> clone3(). It should NOT be called CLONE3_NEWTIME, but rather
>> CLONE_NEWTIME (or similar), because that same flag will presumably
>> also be used in other APIs such as unshare() and setns(). (Hmm -- I
> 
> There are some noteable differences though. CLONE_NEWTIME takes the
> CSIGNAL bit which is in the range of a 32bit integer and thus useable by
> unshare() too. The same does not hold for CLONE{3}_CLEAR_SIGHAND. You
> can't pass it to unshare(). unshare() also just deals with
> namespace-relevant stuff so CLONE{3}_CLEAR_SIGHAND doesn't make much
> sense there.

Sure, but going forward there's very likely to be more CLONE flags
for whatever reason, and some will be usable just in clone3()
while others will be more widely used (in other APIs such as
unshare() and setns()). Using two different prefixes for these
flags (CLONE_/CLONE3_) would be just confusing. AFAICS, the CLONE3_
prefix really provides no advantage, but does have the potential to
cause confusion down the track for the aforementioned reasons.
(Furthermore... Shudder! What if there's a clone4() one day. I
know you might say: "won't happen, we got things right this time",
but API history suggests that "right" now not infrequently becomes
"oops" later.) I do recommend CLONE_ for all the flags...

>> wonder if we are going to need a new unshare2() or some such...)
> 
> We still have one 32bit bit left (CLONE_DETACHED) which we can't reuse
> with clone()/clone2() but we can reuse with clone3(). We can simply
> earmark it for namespace-related stuff and thus still have one bit left
> for unshare() before we have to go for unshare2() (If we have to go
> there at all since I'm not sure how much more namespaces we can come up
> with.).

I'm sure there'll be more namespaces...

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Re: [PATCH 1/2] clone3: add CLONE3_CLEAR_SIGHAND

[Christian Brauner]

On Sat, Oct 12, 2019 at 01:46:54PM +0200, Michael Kerrisk (man-pages) wrote:
> On 10/12/19 9:48 AM, Christian Brauner wrote:
> > On Sat, Oct 12, 2019 at 08:53:34AM +0200, Michael Kerrisk (man-pages) wrote:
> >> Hello Aleksa,
> >>
> >> On Sat, 12 Oct 2019 at 00:12, Aleksa Sarai <cyphar@cyphar.com> wrote:
> >>>
> >>> On 2019-10-11, Michael Kerrisk <mtk.manpages@gmail.com> wrote:
> >>>> Why CLONE3_CLEAR_SIGHAND rather than just CLONE_CLEAR_SIGHAND?
> > 
> > I don't care much how we name this apart from the "_CLEAR_SIGHAND"
> > suffix. But see for a little rationale below.
> > 
> >>>
> >>> There are no more flag bits left for the classic clone()/clone2() (the
> >>> last one was used up by CLONE_PIDFD) -- thus this flag is clone3()-only.
> >>
> >> Yes, I understand that. But, I'm not sure that the "3" in the prefix
> >> is necessary. "CLONE_" still seems better to me.
> >>
> >> Consider this: sometime in the near future we will probably have time
> >> namespaces. The new flag for those namespaces will only be usable with
> >> clone3(). It should NOT be called CLONE3_NEWTIME, but rather
> >> CLONE_NEWTIME (or similar), because that same flag will presumably
> >> also be used in other APIs such as unshare() and setns(). (Hmm -- I
> > 
> > There are some noteable differences though. CLONE_NEWTIME takes the
> > CSIGNAL bit which is in the range of a 32bit integer and thus useable by
> > unshare() too. The same does not hold for CLONE{3}_CLEAR_SIGHAND. You
> > can't pass it to unshare(). unshare() also just deals with
> > namespace-relevant stuff so CLONE{3}_CLEAR_SIGHAND doesn't make much
> > sense there.
> 
> Sure, but going forward there's very likely to be more CLONE flags
> for whatever reason, and some will be usable just in clone3()
> while others will be more widely used (in other APIs such as
> unshare() and setns()). Using two different prefixes for these
> flags (CLONE_/CLONE3_) would be just confusing. AFAICS, the CLONE3_

I do agree with that part. And as I said in my previous mail, I don't
care about the prefix.

> prefix really provides no advantage, but does have the potential to
> cause confusion down the track for the aforementioned reasons.
> (Furthermore... Shudder! What if there's a clone4() one day. I
> know you might say: "won't happen, we got things right this time",
> but API history suggests that "right" now not infrequently becomes
> "oops" later.) I do recommend CLONE_ for all the flags...

I do love your trust in our ability to design syscalls (//Cc Aleksa ;)). :)

> 
> >> wonder if we are going to need a new unshare2() or some such...)
> > 
> > We still have one 32bit bit left (CLONE_DETACHED) which we can't reuse
> > with clone()/clone2() but we can reuse with clone3(). We can simply
> > earmark it for namespace-related stuff and thus still have one bit left
> > for unshare() before we have to go for unshare2() (If we have to go
> > there at all since I'm not sure how much more namespaces we can come up
> > with.).
> 
> I'm sure there'll be more namespaces...

Let's hope not. :) Anyway, no real reason to do unshare2() any time
soon. :)

Christian