Implement clone3() system call

Subject: [RFC][v8][PATCH 0/10] Implement clone3() system call

To support application checkpoint/restart, a task must have the same pid it had when it was checkpointed. When containers are nested, the tasks within the containers exist in multiple pid namespaces and hence have multiple pids to specify during restart.

This patchset implements a new system call, clone3() that lets a process specify the pids of the child process.

Patches 1 through 7 are helper patches, needed for choosing a pid for the child process.

PATCH 9 defines a prototype of the new system call. PATCH 10 adds some documentation on the new system call, some/all of which will eventually go into a man page.

Changelog[v8]:
- [Oren Laadan, Louis Rilling, KOSAKI Motohiro]
The name 'clone2()' is in use - renamed new syscall to clone3().
- [Oren Laadan] ->parent_tidptr and ->child_tidptr need to be 64bit.
- [Oren Laadan] Ensure that unused fields/flags in clone_struct are 0. (Added [PATCH 7/10] to the patchset).

Changelog[v7]:
- [Peter Zijlstra, Arnd Bergmann]
Group the arguments to clone2() into a 'struct clone_arg' to workaround the issue of exceeding 6 arguments to the system call. Also define clone-flags as u64 to allow additional clone-flags.

Changelog[v6]:
- [Nathan Lynch, Arnd Bergmann, H. Peter Anvin, Linus Torvalds] Change 'pid_set.pids' to 'pid_t pids[]' so sizeof(struct pid_set) is constant across architectures (Patches 7, 8).
- (Nathan Lynch) Change pid_set.num_pids to unsigned and remove 'unum_pids < 0' check (Patches 7,8)
- (Pavel Machek) New patch (Patch 9) to add some documentation.

Changelog[v5]:
- Make 'pid_max' a property of pid_ns (Integrated Serge Hallyn's patch into this set)
- (Eric Biederman): Avoid the new function, set_pidmap() - added couple of checks on 'target_pid' in alloc_pidmap() itself.

=== IMPORTANT TODO:

clone() system call has another limitation - all but one, available bits in clone-flags are in use and if more new clone-flags are needed, we will need a variant of the clone() system call.

It appears to make sense to try and extend this new system call to address this limitation as well. The requirements of a new clone system call could then be summarized as:

- do everything clone() does today, and
- give application an ability to choose pids for the child process in all ancestor pid namespaces, and
- allow more clone_flags

Contstraints:

- system-calls are restricted to 6 parameters and clone() already takes 5 parameters, any extension to clone() interface would require one or more copy_from_user(). (Not sure if copy_from_user() of ~40 bytes would have a significant impact on performance of clone()).

Based on these requirements and constraints, we explored a couple of system call interfaces (in earlier versions of this patchset). Based on input from Arnd Bergmann and others, the new interface of the system call is:

	struct clone_struct {
		u64 flags;
		u64 child_stack;
		u32 nr_pids;
		u32 reserved1;
		u64 parent_tid;
		u64 child_tid;
		u64 reserved2;
	};

	sys_clone3(struct clone_struct __user *cs, pid_t __user *pids)

Subject: [RFC][v8][PATCH 1/10]: Factor out code to allocate pidmap page

To simplify alloc_pidmap(), move code to allocate a pid map page to a separate function.

Changelog[v3]:
- Earlier version of patchset called alloc_pidmap_page() from two places. But now its called from only one place. Even so, moving this code out into a separate function simplifies alloc_pidmap().
Changelog[v2]:
- (Matt Helsley, Dave Hansen) Have alloc_pidmap_page() return -ENOMEM on error instead of -1.

---
 kernel/pid.c |   45 ++++++++++++++++++++++++++++++---------------
 1 file changed, 30 insertions(+), 15 deletions(-)

Index: linux-2.6/kernel/pid.c
===================================================================
@@ -122,9 +122,35 @@ static void free_pidmap(struct upid *upi
 	atomic_inc(&map->nr_free);
 }
 
+static int alloc_pidmap_page(struct pidmap *map)
+{
+	void *page;
+
+	if (likely(map->page))
+		return 0;
+
+	page = kzalloc(PAGE_SIZE, GFP_KERNEL);
+
+	/*
+	 * Free the page if someone raced with us installing it:
+	 */
+	spin_lock_irq(&pidmap_lock);
+	if (map->page)
+		kfree(page);
+	else
+		map->page = page;
+	spin_unlock_irq(&pidmap_lock);
+
+	if (unlikely(!map->page))
+		return -ENOMEM;
+
+	return 0;
+}
+
 static int alloc_pidmap(struct pid_namespace *pid_ns)
 {
 	int i, offset, max_scan, pid, last = pid_ns->last_pid;
+	int rc;
 	struct pidmap *map;
 
 	pid = last + 1;

@@ -134,21 +160,10 @@ static int alloc_pidmap(struct pid_names
 	map = &pid_ns->pidmap[pid/BITS_PER_PAGE];
 	max_scan = (pid_max + BITS_PER_PAGE - 1)/BITS_PER_PAGE - !offset;
 	for (i = 0; i <= max_scan; ++i) {
-		if (unlikely(!map->page)) {
-			void *page = kzalloc(PAGE_SIZE, GFP_KERNEL);
-			/*
-			 * Free the page if someone raced with us
-			 * installing it:
-			 */
-			spin_lock_irq(&pidmap_lock);
-			if (map->page)
-				kfree(page);
-			else
-				map->page = page;
-			spin_unlock_irq(&pidmap_lock);
-			if (unlikely(!map->page))
-				break;
-		}
+		rc = alloc_pidmap_page(map);
+		if (rc)
+			break;
+
 		if (likely(atomic_read(&map->nr_free))) {
 			do {
 				if (!test_and_set_bit(offset, map->page)) {

Subject: [RFC][v8][PATCH 2/10]: Have alloc_pidmap() return actual error code

alloc_pidmap() can fail either because all pid numbers are in use or because memory allocation failed. With support for setting a specific pid number, alloc_pidmap() would also fail if either the given pid number is invalid or in use.

Rather than have callers assume -ENOMEM, have alloc_pidmap() return the actual error.

---
 kernel/fork.c |    5 +++--
 kernel/pid.c  |   14 +++++++++-----
 2 files changed, 12 insertions(+), 7 deletions(-)

Index: linux-2.6/kernel/fork.c
===================================================================
@@ -1110,10 +1110,11 @@ static struct task_struct *copy_process(
 		goto bad_fork_cleanup_io;
 
 	if (pid != &init_struct_pid) {
-		retval = -ENOMEM;
 		pid = alloc_pid(p->nsproxy->pid_ns);
-		if (!pid)
+		if (IS_ERR(pid)) {
+			retval = PTR_ERR(pid);
 			goto bad_fork_cleanup_io;
+		}
 
 		if (clone_flags & CLONE_NEWPID) {
 			retval = pid_ns_prepare_proc(p->nsproxy->pid_ns);

Index: linux-2.6/kernel/pid.c
===================================================================
@@ -150,7 +150,7 @@ static int alloc_pidmap_page(struct pidm
 static int alloc_pidmap(struct pid_namespace *pid_ns)
 {
 	int i, offset, max_scan, pid, last = pid_ns->last_pid;
-	int rc;
+	int rc = -EAGAIN;
 	struct pidmap *map;
 
 	pid = last + 1;

@@ -189,12 +189,14 @@ static int alloc_pidmap(struct pid_names
 		} else {
 			map = &pid_ns->pidmap[0];
 			offset = RESERVED_PIDS;
-			if (unlikely(last == offset))
+			if (unlikely(last == offset)) {
+				rc = -EAGAIN;
 				break;
+			}
 		}
 		pid = mk_pid(pid_ns, map, offset);
 	}
-	return -1;
+	return rc;
 }
 
 int next_pidmap(struct pid_namespace *pid_ns, int last)

@@ -263,8 +265,10 @@ struct pid *alloc_pid(struct pid_namespa
 	struct upid *upid;
 
 	pid = kmem_cache_alloc(ns->pid_cachep, GFP_KERNEL);
-	if (!pid)
+	if (!pid) {
+		pid = ERR_PTR(-ENOMEM);
 		goto out;
+	}
 
 	tmp = ns;
 	for (i = ns->level; i >= 0; i--) {

@@ -299,7 +303,7 @@ out_free:
 		free_pidmap(pid->numbers + i);
 
 	kmem_cache_free(ns->pid_cachep, pid);
-	pid = NULL;
+	pid = ERR_PTR(nr);
 	goto out;
 }
 

From: Serge Hallyn <[email protected]>
Subject: [RFC][v8][PATCH 3/10]: Make pid_max a pid_ns property

Remove the pid_max global, and make it a property of the pid_namespace. When a pid_ns is created, it inherits the parent's pid_ns.

Fixing up sysctl (trivial akin to ipc version, but potentially tedious to get right for all CONFIG* combinations) is left for later.

Changelog[v2]:
- Port to newer kernel
- Make pid_max a local variable in alloc_pidmap() to simplify code/patch

---
 include/linux/pid_namespace.h |    1 +
 kernel/pid.c                  |    4 ++--
 kernel/pid_namespace.c        |    1 +
 kernel/sysctl.c               |    4 ++--
 4 files changed, 6 insertions(+), 4 deletions(-)

Index: linux-2.6/include/linux/pid_namespace.h
===================================================================
@@ -30,6 +30,7 @@ struct pid_namespace {
 #ifdef CONFIG_BSD_PROCESS_ACCT
 	struct bsd_acct_struct *bacct;
 #endif
+	int pid_max;
 };
 
 extern struct pid_namespace init_pid_ns;

Index: linux-2.6/kernel/pid.c
===================================================================
@@ -43,8 +43,6 @@ static struct hlist_head *pid_hash;
 static int pidhash_shift;
 struct pid init_struct_pid = INIT_STRUCT_PID;
 
-int pid_max = PID_MAX_DEFAULT;
-
 #define RESERVED_PIDS		300
 
 int pid_max_min = RESERVED_PIDS + 1;

@@ -78,6 +76,7 @@ struct pid_namespace init_pid_ns = {
 	.last_pid = 0,
 	.level = 0,
 	.child_reaper = &init_task,
+	.pid_max = PID_MAX_DEFAULT,
 };
 EXPORT_SYMBOL_GPL(init_pid_ns);
 

@@ -151,6 +150,7 @@ static int alloc_pidmap(struct pid_names
 {
 	int i, offset, max_scan, pid, last = pid_ns->last_pid;
 	int rc = -EAGAIN;
+	int pid_max = pid_ns->pid_max;
 	struct pidmap *map;
 
 	pid = last + 1;

Index: linux-2.6/kernel/pid_namespace.c
===================================================================
@@ -87,6 +87,7 @@ static struct pid_namespace *create_pid_
 
 	kref_init(&ns->kref);
 	ns->level = level;
+	ns->pid_max = parent_pid_ns->pid_max;
 	ns->parent = get_pid_ns(parent_pid_ns);
 
 	set_bit(0, ns->pidmap[0].page);

Index: linux-2.6/kernel/sysctl.c
===================================================================
@@ -55,6 +55,7 @@
 
 #include <asm/uaccess.h>
 #include <asm/processor.h>
+#include <linux/pid_namespace.h>
 
 #ifdef CONFIG_X86
 #include <asm/nmi.h>

@@ -78,7 +79,6 @@ extern int max_threads;
 extern int core_uses_pid;
 extern int suid_dumpable;
 extern char core_pattern[];
-extern int pid_max;
 extern int min_free_kbytes;
 extern int pid_max_min, pid_max_max;
 extern int sysctl_drop_caches;

@@ -670,7 +670,7 @@ static struct ctl_table kern_table[] = {
 	{
 		.ctl_name	= KERN_PIDMAX,
 		.procname	= "pid_max",
-		.data		= &pid_max,
+		.data		= &init_pid_ns.pid_max,
 		.maxlen		= sizeof (int),
 		.mode		= 0644,
 		.proc_handler	= &proc_dointvec_minmax,

pid_max is not pid_ns property, it's just sysctl to limit cpu time during bitmap search. Just like vm.max_map_count limits kernel memory used by VMAs.

Not that I'm about to slow down or block the process, but... frankly I don't see the reason for doing so. Why should we? Especially taking into account, that we essentially cannot change thin in the namespace level 3 and deeper?

This patch isn't a core part of the clone_with_pid functionality, just something Eric has asked for. So I don't object to dropping it. But I disagree with Alexey's claim that this isn't a namespace property. It should be.

What do you mean by that? With this patchset we're not, it's true, but we trivially can - even now, userspace can simply not give the container CAP_SYS_ADMIN or write access to the sysctl so they can't do any more CLONE_NEWPIDS or change the sysctl.

-serge

OK

It's a misprint - I meant "level 2 and deeper". Sysctl is only pointing at the init_pid_ns variable.

Right, and I'm saying that's to be fixed up as with all other containerized sysctl's. You're right that this patch doesn't solve that problem, but you seem to be arguing that it bc it's not done in this patch, we should act as though it can't be done.

But again, maybe we're best off dropping this patch (sorry, Suka, I had suggested you add it...) and focusing on the rest of the set for now.

thanks,
-serge

Right, and I'm saying that's to be fixed up as with all other containerized sysctl's. You're right that this patch doesn't solve that problem, but you seem to be arguing that it bc it's not done in this patch, we should act as though it can't be done.

But again, maybe we're best off dropping this patch (sorry, Suka, I had suggested you add it...) and focusing on the rest of the set for now.

thanks,
-serge

OK

It's a misprint - I meant "level 2 and deeper". Sysctl is only pointing at the init_pid_ns variable.

Subject: [RFC][v8][PATCH 4/10]: Add target_pid parameter to alloc_pidmap()

With support for setting a specific pid number for a process, alloc_pidmap() will need a 'target_pid' parameter.

Changelog[v6]:
- Separate target_pid > 0 case to minimize the number of checks needed.
Changelog[v3]:
- (Eric Biederman): Avoid set_pidmap() function. Added couple of checks for target_pid in alloc_pidmap() itself.
Changelog[v2]:
- (Serge Hallyn) Check for 'pid < 0' in set_pidmap().(Code actually checks for 'pid <= 0' for completeness).

---
 kernel/pid.c |   26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Index: linux-2.6/kernel/pid.c
===================================================================
@@ -146,16 +146,22 @@ static int alloc_pidmap_page(struct pidm
 	return 0;
 }
 
-static int alloc_pidmap(struct pid_namespace *pid_ns)
+static int alloc_pidmap(struct pid_namespace *pid_ns, int target_pid)
 {
 	int i, offset, max_scan, pid, last = pid_ns->last_pid;
 	int rc = -EAGAIN;
 	int pid_max = pid_ns->pid_max;
 	struct pidmap *map;
 
-	pid = last + 1;
-	if (pid >= pid_max)
-		pid = RESERVED_PIDS;
+	if (target_pid) {
+		pid = target_pid;
+		if (pid < 0 || pid >= pid_max)
+			return -EINVAL;
+	} else {
+		pid = last + 1;
+		if (pid >= pid_max)
+			pid = RESERVED_PIDS;
+	}
 	offset = pid & BITS_PER_PAGE_MASK;
 	map = &pid_ns->pidmap[pid/BITS_PER_PAGE];
 	max_scan = (pid_max + BITS_PER_PAGE - 1)/BITS_PER_PAGE - !offset;

@@ -164,6 +170,15 @@ static int alloc_pidmap(struct pid_names
 		if (rc)
 			break;
 
+		if (target_pid) {
+			rc = -EBUSY;
+			if (!test_and_set_bit(offset, map->page)) {
+				atomic_dec(&map->nr_free);
+				rc = pid;
+			}
+			return rc;
+		}
+
 		if (likely(atomic_read(&map->nr_free))) {
 			do {
 				if (!test_and_set_bit(offset, map->page)) {

@@ -196,6 +211,7 @@ static int alloc_pidmap(struct pid_names
 		}
 		pid = mk_pid(pid_ns, map, offset);
 	}
+
 	return rc;
 }
 

@@ -272,7 +288,7 @@ struct pid *alloc_pid(struct pid_namespa
 
 	tmp = ns;
 	for (i = ns->level; i >= 0; i--) {
-		nr = alloc_pidmap(tmp);
+		nr = alloc_pidmap(tmp, 0);
 		if (nr < 0)
 			goto out_free;
 

[ snip ]

Pavel Emelyanov [[email protected]] wrote:

| Sukadev Bhattiprolu wrote:
| > 
| > Subject: [RFC][v8][PATCH 4/10]: Add target_pid parameter to alloc_pidmap()
| > 
| 
| [ snip ]
| 
| > @@ -146,16 +146,22 @@ static int alloc_pidmap_page(struct pidm
| >  	return 0;
| >  }
| >  
| > -static int alloc_pidmap(struct pid_namespace *pid_ns)
| > +static int alloc_pidmap(struct pid_namespace *pid_ns, int target_pid)
| 
| Please no! Better create another function, that will just
| atomic_test_and_set() the proper bit in the map. This one
| is heavy enough already.

I had a set_pidmap() function before:

	http://lkml.org/lkml/2009/8/7/24

but dropped it based on some comments and the current version evolved from it. I could go back to that if it still makes sense.

Thanks,

Sukadev

Subject: [RFC][v8][PATCH 5/10]: Add target_pids parameter to alloc_pid()

This parameter is currently NULL, but will be used in a follow-on patch.

---
 include/linux/pid.h |    2 +-
 kernel/fork.c       |    3 ++-
 kernel/pid.c        |    9 +++++++--
 3 files changed, 10 insertions(+), 4 deletions(-)

Index: linux-2.6/include/linux/pid.h
===================================================================
@@ -119,7 +119,7 @@ extern struct pid *find_get_pid(int nr);
 extern struct pid *find_ge_pid(int nr, struct pid_namespace *);
 int next_pidmap(struct pid_namespace *pid_ns, int last);
 
-extern struct pid *alloc_pid(struct pid_namespace *ns);
+extern struct pid *alloc_pid(struct pid_namespace *ns, pid_t *target_pids);
 extern void free_pid(struct pid *pid);
 
 /*

Index: linux-2.6/kernel/fork.c
===================================================================
@@ -940,6 +940,7 @@ static struct task_struct *copy_process(
 	int retval;
 	struct task_struct *p;
 	int cgroup_callbacks_done = 0;
+	pid_t *target_pids = NULL;
 
 	if ((clone_flags & (CLONE_NEWNS|CLONE_FS)) == (CLONE_NEWNS|CLONE_FS))
 		return ERR_PTR(-EINVAL);

@@ -1110,7 +1111,7 @@ static struct task_struct *copy_process(
 		goto bad_fork_cleanup_io;
 
 	if (pid != &init_struct_pid) {
-		pid = alloc_pid(p->nsproxy->pid_ns);
+		pid = alloc_pid(p->nsproxy->pid_ns, target_pids);
 		if (IS_ERR(pid)) {
 			retval = PTR_ERR(pid);
 			goto bad_fork_cleanup_io;

Index: linux-2.6/kernel/pid.c
===================================================================
@@ -272,13 +272,14 @@ void free_pid(struct pid *pid)
 	call_rcu(&pid->rcu, delayed_put_pid);
 }
 
-struct pid *alloc_pid(struct pid_namespace *ns)
+struct pid *alloc_pid(struct pid_namespace *ns, pid_t *target_pids)
 {
 	struct pid *pid;
 	enum pid_type type;
 	int i, nr;
 	struct pid_namespace *tmp;
 	struct upid *upid;
+	int tpid;
 
 	pid = kmem_cache_alloc(ns->pid_cachep, GFP_KERNEL);
 	if (!pid) {

@@ -288,7 +289,11 @@ struct pid *alloc_pid(struct pid_namespa
 
 	tmp = ns;
 	for (i = ns->level; i >= 0; i--) {
-		nr = alloc_pidmap(tmp, 0);
+		tpid = 0;
+		if (target_pids)
+			tpid = target_pids[i];
+
+		nr = alloc_pidmap(tmp, tpid);
 		if (nr < 0)
 			goto out_free;
 

Subject: [RFC][v8][PATCH 6/10]: Add target_pids parameter to copy_process()

Add a 'target_pids' parameter to copy_process(). The new parameter will be used in a follow-on patch when clone_with_pids() is implemented.

---
 kernel/fork.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Index: linux-2.6/kernel/fork.c
===================================================================
@@ -935,12 +935,12 @@ static struct task_struct *copy_process(
 					unsigned long stack_size,
 					int __user *child_tidptr,
 					struct pid *pid,
+					pid_t *target_pids,
 					int trace)
 {
 	int retval;
 	struct task_struct *p;
 	int cgroup_callbacks_done = 0;
-	pid_t *target_pids = NULL;
 
 	if ((clone_flags & (CLONE_NEWNS|CLONE_FS)) == (CLONE_NEWNS|CLONE_FS))
 		return ERR_PTR(-EINVAL);

@@ -1319,7 +1319,7 @@ struct task_struct * __cpuinit fork_idle
 	struct pt_regs regs;
 
 	task = copy_process(CLONE_VM, 0, idle_regs(&regs), 0, NULL,
-			    &init_struct_pid, 0);
+			    &init_struct_pid, NULL, 0);
 	if (!IS_ERR(task))
 		init_idle(task, cpu);
 

@@ -1342,6 +1342,7 @@ long do_fork(unsigned long clone_flags,
 	struct task_struct *p;
 	int trace = 0;
 	long nr;
+	pid_t *target_pids = NULL;
 
 	/*
 	 * Do some preliminary argument and permissions checking before we

@@ -1382,7 +1383,7 @@ long do_fork(unsigned long clone_flags,
 		trace = tracehook_prepare_clone(clone_flags);
 
 	p = copy_process(clone_flags, stack_start, regs, stack_size,
-			 child_tidptr, NULL, trace);
+			 child_tidptr, NULL, target_pids, trace);
 	/*
 	 * Do this prior waking up the new thread - the thread pointer
 	 * might get invalid after that point, if the thread exits quickly.

Subject: [RFC][v8][PATCH 7/10]: Check invalid clone flags

As pointed out by Oren Laadan, we want to ensure that unused bits in the clone-flags remain unused and available for future. To ensure this, define a mask of clone-flags and check the flags in the clone() system calls.

Changelog[v8]:
- New patch in set

---
 include/linux/sched.h |   10 ++++++++++
 kernel/fork.c         |    3 +++
 2 files changed, 13 insertions(+)

Index: linux-2.6/include/linux/sched.h
===================================================================
@@ -29,6 +29,16 @@
 #define CLONE_NEWNET		0x40000000	/* New network namespace */
 #define CLONE_IO		0x80000000	/* Clone io context */
 
+#define VALID_CLONE_FLAGS	(CSIGNAL | CLONE_VM | CLONE_FS | CLONE_FILES |\
+				 CLONE_SIGHAND | CLONE_PTRACE | CLONE_VFORK  |\
+				 CLONE_PARENT | CLONE_THREAD | CLONE_NEWNS   |\
+				 CLONE_SYSVSEM | CLONE_SETTLS                |\
+				 CLONE_PARENT_SETTID | CLONE_CHILD_CLEARTID  |\
+				 CLONE_DETACHED | CLONE_UNTRACED             |\
+				 CLONE_CHILD_SETTID | CLONE_STOPPED          |\
+				 CLONE_NEWUTS | CLONE_NEWIPC | CLONE_NEWUSER |\
+				 CLONE_NEWPID | CLONE_NEWNET| CLONE_IO)
+
 /*
  * Scheduling policies
  */

Index: linux-2.6/kernel/fork.c
===================================================================
@@ -942,6 +942,9 @@ static struct task_struct *copy_process(
 	struct task_struct *p;
 	int cgroup_callbacks_done = 0;
 
+	if (clone_flags & ~VALID_CLONE_FLAGS)
+		return ERR_PTR(-EINVAL);
+
 	if ((clone_flags & (CLONE_NEWNS|CLONE_FS)) == (CLONE_NEWNS|CLONE_FS))
 		return ERR_PTR(-EINVAL);
 

Oren Laadan [[email protected]] wrote:

| 
| 
| Sukadev Bhattiprolu wrote:
| > 
| > Subject: [RFC][v8][PATCH 7/10]: Check invalid clone flags
| > 
| > As pointed out by Oren Laadan, we want to ensure that unused bits in the
| > clone-flags remain unused and available for future. To ensure this, define
| > a mask of clone-flags and check the flags in the clone() system calls.
| > 
| > Changelog[v8]:
| > 	- New patch in set
| > 
| > Signed-off-by: Sukadev Bhattiprolu <[email protected]>
| > 
| > ---
| >  include/linux/sched.h |   10 ++++++++++
| >  kernel/fork.c         |    3 +++
| >  2 files changed, 13 insertions(+)
| > 
| > Index: linux-2.6/include/linux/sched.h
| > ===================================================================
| > --- linux-2.6.orig/include/linux/sched.h	2009-10-02 18:53:55.000000000 -0700
| > +++ linux-2.6/include/linux/sched.h	2009-10-02 19:58:21.000000000 -0700
| > @@ -29,6 +29,16 @@
| >  #define CLONE_NEWNET		0x40000000	/* New network namespace */
| >  #define CLONE_IO		0x80000000	/* Clone io context */
| >  
| > +#define VALID_CLONE_FLAGS	(CSIGNAL | CLONE_VM | CLONE_FS | CLONE_FILES |\
| > +				 CLONE_SIGHAND | CLONE_PTRACE | CLONE_VFORK  |\
| > +				 CLONE_PARENT | CLONE_THREAD | CLONE_NEWNS   |\
| > +				 CLONE_SYSVSEM | CLONE_SETTLS                |\
| > +				 CLONE_PARENT_SETTID | CLONE_CHILD_CLEARTID  |\
| > +				 CLONE_DETACHED | CLONE_UNTRACED             |\
| > +				 CLONE_CHILD_SETTID | CLONE_STOPPED          |\
| > +				 CLONE_NEWUTS | CLONE_NEWIPC | CLONE_NEWUSER |\
| > +				 CLONE_NEWPID | CLONE_NEWNET| CLONE_IO)
| > +
| >  /*
| >   * Scheduling policies
| >   */
| > Index: linux-2.6/kernel/fork.c
| > ===================================================================
| > --- linux-2.6.orig/kernel/fork.c	2009-10-02 19:00:08.000000000 -0700
| > +++ linux-2.6/kernel/fork.c	2009-10-02 19:57:36.000000000 -0700
| > @@ -942,6 +942,9 @@ static struct task_struct *copy_process(
| >  	struct task_struct *p;
| >  	int cgroup_callbacks_done = 0;
| >  
| 
| We can safely apply these tests to clone3(), because it is a new syscall.
| 
| However, I don't know if applying it to clone() can break existing
| application that may already be (incorrectly) using invalid flags ?

Doing the check in copy_process() seems would apply to all architectures.

As for breaking an existing app, there is only one unused flag, 0x00001000 (bw CLONE_SIGHAND and CLONE_PTRACE). If an application could be using that and we don't want to break it, maybe we can add following macro to the VALID_CLONE_FLAGS list ?

	#define CLONE_UNUSED_BIT	0x00001000

Subject: [RFC][v8][PATCH 8/10]: Define do_fork_with_pids()

do_fork_with_pids() is same as do_fork(), except that it takes an additional, 'pid_set', parameter. This parameter, currently unused, specifies the set of target pids of the process in each of its pid namespaces.

Changelog[v7]:
- Drop 'struct pid_set' object and pass in 'pid_t *target_pids' instead of 'struct pid_set *'.

Changelog[v6]:
- (Nathan Lynch, Arnd Bergmann, H. Peter Anvin, Linus Torvalds) Change 'pid_set.pids' to a 'pid_t pids[]' so size of 'struct pid_set' is constant across architectures.
- (Nathan Lynch) Change 'pid_set.num_pids' to 'unsigned int'.

Changelog[v4]:
- Rename 'struct target_pid_set' to 'struct pid_set' since it may be useful in other contexts.

Changelog[v3]:
- Fix "long-line" warning from checkpatch.pl

Changelog[v2]:
- To facilitate moving architecture-inpdendent code to kernel/fork.c pass in 'struct target_pid_set __user *' to do_fork_with_pids() rather than 'pid_t *' (next patch moves the arch-independent code to kernel/fork.c)

---
 include/linux/sched.h |    3 +++
 kernel/fork.c         |   17 +++++++++++++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

Index: linux-2.6/include/linux/sched.h
===================================================================
@@ -2064,6 +2064,9 @@ extern int disallow_signal(int);
 
 extern int do_execve(char *, char __user * __user *, char __user * __user *, struct pt_regs *);
 extern long do_fork(unsigned long, unsigned long, struct pt_regs *, unsigned long, int __user *, int __user *);
+extern long do_fork_with_pids(unsigned long, unsigned long, struct pt_regs *,
+				unsigned long, int __user *, int __user *,
+				unsigned int, pid_t __user *);
 struct task_struct *fork_idle(int);
 
 extern void set_task_comm(struct task_struct *tsk, char *from);

Index: linux-2.6/kernel/fork.c
===================================================================
@@ -1335,12 +1335,14 @@ struct task_struct * __cpuinit fork_idle
  * It copies the process, and if successful kick-starts
  * it and waits for it to finish using the VM if required.
  */
-long do_fork(unsigned long clone_flags,
+long do_fork_with_pids(unsigned long clone_flags,
 	      unsigned long stack_start,
 	      struct pt_regs *regs,
 	      unsigned long stack_size,
 	      int __user *parent_tidptr,
-	      int __user *child_tidptr)
+	      int __user *child_tidptr,
+	      unsigned int num_pids,
+	      pid_t __user *upids)
 {
 	struct task_struct *p;
 	int trace = 0;

@@ -1443,6 +1445,17 @@ long do_fork(unsigned long clone_flags,
 	return nr;
 }
 
+long do_fork(unsigned long clone_flags,
+	      unsigned long stack_start,
+	      struct pt_regs *regs,
+	      unsigned long stack_size,
+	      int __user *parent_tidptr,
+	      int __user *child_tidptr)
+{
+	return do_fork_with_pids(clone_flags, stack_start, regs, stack_size,
+			parent_tidptr, child_tidptr, 0, NULL);
+}
+
 #ifndef ARCH_MIN_MMSTRUCT_ALIGN
 #define ARCH_MIN_MMSTRUCT_ALIGN 0
 #endif

Subject: [RFC][v8][PATCH 9/10]: Define clone3() syscall

Container restart requires that a task have the same pid it had when it was checkpointed. When containers are nested the tasks within the containers exist in multiple pid namespaces and hence have multiple pids to specify during restart.

clone3(), intended for use during restart, is the same as clone(), except that it takes a 'pids' paramter. This parameter lets caller choose specific pid numbers for the child process, in the process's active and ancestor pid namespaces. (Descendant pid namespaces in general don't matter since processes don't have pids in them anyway, but see comments in copy_target_pids() regarding CLONE_NEWPID).

Clone2() system call also attempts to address a second limitation of the clone() system call. clone() is restricted to 32 clone flags and most (all ?) of these are in use. If a new clone flag is needed, we will be forced to define a new variant of the clone() system call.

To prevent unprivileged processes from misusing this interface, clone3() currently needs CAP_SYS_ADMIN, when the 'pids' parameter is non-NULL.

See Documentation/clone3 in next patch for more details of clone3() and an example of its usage.

NOTE:
System calls are restricted to 6 parameters and the number and sizes of parameters needed for sys_clone3() exceed 6 integers. The new prototype works around this restriction while providing some flexibility if clone3() needs to be further extended in the future.
TODO:
- May need additional sanity checks in do_fork_with_pids().

Changelog[v8]
- [Oren Laadan] parent_tid and child_tid fields in 'struct clone_arg' must be 64-bit.
- clone2() is in use in IA64. Rename system call to clone3().

Changelog[v7]:
- [Peter Zijlstra, Arnd Bergmann] Rename system call to clone2() and group parameters into a new 'struct clone_struct' object.

Changelog[v6]:
- (Nathan Lynch, Arnd Bergmann, H. Peter Anvin, Linus Torvalds) Change 'pid_set.pids' to a 'pid_t pids[]' so size of 'struct pid_set' is constant across architectures.
- (Nathan Lynch) Change pid_set.num_pids to unsigned and remove 'unum_pids < 0' check.

Changelog[v4]:
- (Oren Laadan) rename 'struct target_pid_set' to 'struct pid_set'

Changelog[v3]:
- (Oren Laadan) Allow CLONE_NEWPID flag (by allocating an extra pid in the target_pids[] list and setting it 0. See copy_target_pids()).
- (Oren Laadan) Specified target pids should apply only to youngest pid-namespaces (see copy_target_pids())
- (Matt Helsley) Update patch description.

Changelog[v2]:
- Remove unnecessary printk and add a note to callers of copy_target_pids() to free target_pids.
- (Serge Hallyn) Mention CAP_SYS_ADMIN restriction in patch description.
- (Oren Laadan) Add checks for 'num_pids < 0' (return -EINVAL) and 'num_pids == 0' (fall back to normal clone()).
- Move arch-independent code (sanity checks and copy-in of target-pids) into kernel/fork.c and simplify sys_clone_with_pids()

Changelog[v1]:
- Fixed some compile errors (had fixed these errors earlier in my git tree but had not refreshed patches before emailing them)

---
 arch/x86/include/asm/syscalls.h    |    1 
 arch/x86/include/asm/unistd_32.h   |    1 
 arch/x86/kernel/process_32.c       |   34 +++++++++++++
 arch/x86/kernel/syscall_table_32.S |    1 
 include/linux/types.h              |   10 +++
 kernel/fork.c                      |   96 ++++++++++++++++++++++++++++++++++++-
 6 files changed, 142 insertions(+), 1 deletion(-)

Index: linux-2.6/include/linux/types.h
===================================================================
@@ -204,6 +204,16 @@ struct ustat {
 	char			f_fpack[6];
 };
 
+struct clone_struct {
+	u64 flags;
+	u64 child_stack;
+	u32 nr_pids;
+	u32 reserved1;
+	u64 parent_tid;
+	u64 child_tid;
+	u64 reserved2;
+};
+
 #endif	/* __KERNEL__ */
 #endif /*  __ASSEMBLY__ */
 #endif /* _LINUX_TYPES_H */

Index: linux-2.6/arch/x86/include/asm/syscalls.h
===================================================================
@@ -55,6 +55,7 @@ struct sel_arg_struct;
 struct oldold_utsname;
 struct old_utsname;
 
+asmlinkage long sys_clone3(struct clone_struct __user *cs, pid_t *pids);
 asmlinkage long sys_mmap2(unsigned long, unsigned long, unsigned long,
 			  unsigned long, unsigned long, unsigned long);
 asmlinkage int old_mmap(struct mmap_arg_struct __user *);

Index: linux-2.6/arch/x86/include/asm/unistd_32.h
===================================================================
@@ -342,6 +342,7 @@
 #define __NR_pwritev		334
 #define __NR_rt_tgsigqueueinfo	335
 #define __NR_perf_counter_open	336
+#define __NR_clone3		337
 
 #ifdef __KERNEL__
 

Index: linux-2.6/arch/x86/kernel/process_32.c
===================================================================
@@ -443,6 +443,40 @@ int sys_clone(struct pt_regs *regs)
 	return do_fork(clone_flags, newsp, regs, 0, parent_tidptr, child_tidptr);
 }
 
+asmlinkage long
+sys_clone3(struct clone_struct __user *ucs, pid_t __user *pids)
+{
+	int rc;
+	struct clone_struct kcs;
+	unsigned long clone_flags;
+	int __user *parent_tid_ptr;
+	int __user *child_tid_ptr;
+	unsigned long __user child_stack_base;
+	struct pt_regs *regs;
+
+	rc = copy_from_user(&kcs, ucs, sizeof(kcs));
+	if (rc)
+		return -EFAULT;
+
+	if (kcs.reserved1 || kcs.reserved2)
+		return -EINVAL;
+
+	/*
+	 * TODO: Convert clone_flags to 64-bit
+	 */
+	clone_flags = (unsigned long)kcs.flags;
+	child_stack_base = (unsigned long)kcs.child_stack;
+	parent_tid_ptr = (int *)ucs->parent_tid;
+	child_tid_ptr =  (int *)ucs->child_tid;
+	regs = task_pt_regs(current);
+
+	if (!child_stack_base)
+		child_stack_base = user_stack_pointer(regs);
+
+	return do_fork_with_pids(clone_flags, child_stack_base, regs, 0,
+			parent_tid_ptr, child_tid_ptr, kcs.nr_pids, pids);
+}
+
 /*
  * sys_execve() executes a new program.
  */

Index: linux-2.6/arch/x86/kernel/syscall_table_32.S
===================================================================
@@ -336,3 +336,4 @@ ENTRY(sys_call_table)
 	.long sys_pwritev
 	.long sys_rt_tgsigqueueinfo	/* 335 */
 	.long sys_perf_counter_open
+	.long sys_clone3

Here is an updated patch with the following interface:

	long sys_clone3(unsigned int flags_low, struct clone_args __user *cs,
			pid_t *pids);

There are just two other (minor) changes pending to this patchset:

- PATCH 7: add a CLONE_UNUSED bit to VALID_CLONE_FLAGS().
- PATCH 10: update documentation to reflect new interface.

If this looks ok, we repost entire patchset next week.
---

Subject: [RFC][v8][PATCH 9/10]: Define clone3() syscall

Container restart requires that a task have the same pid it had when it was checkpointed. When containers are nested the tasks within the containers exist in multiple pid namespaces and hence have multiple pids to specify during restart.

clone3(), intended for use during restart, is the same as clone(), except that it takes a 'pids' paramter. This parameter lets caller choose specific pid numbers for the child process, in the process's active and ancestor pid namespaces. (Descendant pid namespaces in general don't matter since processes don't have pids in them anyway, but see comments in copy_target_pids() regarding CLONE_NEWPID).

Clone2() system call also attempts to address a second limitation of the clone() system call. clone() is restricted to 32 clone flags and most (all ?) of these are in use. If a new clone flag is needed, we will be forced to define a new variant of the clone() system call.

To prevent unprivileged processes from misusing this interface, clone3() currently needs CAP_SYS_ADMIN, when the 'pids' parameter is non-NULL.

See Documentation/clone3 in next patch for more details of clone3() and an example of its usage.

NOTE:
- System calls are restricted to 6 parameters and the number and sizes of parameters needed for sys_clone3() exceed 6 integers. The new prototype works around this restriction while providing some flexibility if clone3() needs to be further extended in the future.
TODO:
- We should convert clone-flags to 64-bit value in all architectures. Its probably best to do that as a separate patchset since clone_flags touches several functions and that patchset seems independent of this new system call.

Changelog[v9-rc1]:
- [Roland McGrath, H. Peter Anvin] To avoid confusion on 64-bit architectures split the new clone-flags into 'low' and 'high' words and pass in the 'lower' flags as the first argument. This would maintain similarity of the clone3() with clone()/ clone2(). Also has the side-effect of the name matching the number of parameters :-)
- [Roland McGrath] Rename structure to 'clone_args' and add a 'child_stack_size' field

Changelog[v8]
- [Oren Laadan] parent_tid and child_tid fields in 'struct clone_arg' must be 64-bit.
- clone2() is in use in IA64. Rename system call to clone3().

Changelog[v7]:
- [Peter Zijlstra, Arnd Bergmann] Rename system call to clone2() and group parameters into a new 'struct clone_struct' object.

Changelog[v6]:
- (Nathan Lynch, Arnd Bergmann, H. Peter Anvin, Linus Torvalds) Change 'pid_set.pids' to a 'pid_t pids[]' so size of 'struct pid_set' is constant across architectures.
- (Nathan Lynch) Change pid_set.num_pids to unsigned and remove 'unum_pids < 0' check.

Changelog[v4]:
- (Oren Laadan) rename 'struct target_pid_set' to 'struct pid_set'

Changelog[v3]:
- (Oren Laadan) Allow CLONE_NEWPID flag (by allocating an extra pid in the target_pids[] list and setting it 0. See copy_target_pids()).
- (Oren Laadan) Specified target pids should apply only to youngest pid-namespaces (see copy_target_pids())
- (Matt Helsley) Update patch description.

Changelog[v2]:
- Remove unnecessary printk and add a note to callers of copy_target_pids() to free target_pids.
- (Serge Hallyn) Mention CAP_SYS_ADMIN restriction in patch description.
- (Oren Laadan) Add checks for 'num_pids < 0' (return -EINVAL) and 'num_pids == 0' (fall back to normal clone()).
- Move arch-independent code (sanity checks and copy-in of target-pids) into kernel/fork.c and simplify sys_clone_with_pids()

Changelog[v1]:
- Fixed some compile errors (had fixed these errors earlier in my git tree but had not refreshed patches before emailing them)

---
 arch/x86/include/asm/syscalls.h    |    2 
 arch/x86/include/asm/unistd_32.h   |    1 
 arch/x86/kernel/process_32.c       |   61 +++++++++++++++++++++++
 arch/x86/kernel/syscall_table_32.S |    1 
 include/linux/types.h              |   11 ++++
 kernel/fork.c                      |   96 ++++++++++++++++++++++++++++++++++++-
 6 files changed, 171 insertions(+), 1 deletion(-)

Index: linux-2.6/include/linux/types.h
===================================================================
@@ -204,6 +204,17 @@ struct ustat {
 	char			f_fpack[6];
 };
 
+struct clone_args {
+	u64 clone_flags_high;
+	u64 child_stack_base;
+	u64 child_stack_size;
+	u64 parent_tid_ptr;
+	u64 child_tid_ptr;
+	u32 nr_pids;
+	u32 clone_args_size;
+	u64 reserved1;
+};
+
 #endif	/* __KERNEL__ */
 #endif /*  __ASSEMBLY__ */
 #endif /* _LINUX_TYPES_H */

Index: linux-2.6/arch/x86/include/asm/syscalls.h
===================================================================
@@ -55,6 +55,8 @@ struct sel_arg_struct;
 struct oldold_utsname;
 struct old_utsname;
 
+asmlinkage long sys_clone3(unsigned int flags_low,
+			struct clone_args __user *cs, pid_t *pids);
 asmlinkage long sys_mmap2(unsigned long, unsigned long, unsigned long,
 			  unsigned long, unsigned long, unsigned long);
 asmlinkage int old_mmap(struct mmap_arg_struct __user *);

Index: linux-2.6/arch/x86/include/asm/unistd_32.h
===================================================================
@@ -342,6 +342,7 @@
 #define __NR_pwritev		334
 #define __NR_rt_tgsigqueueinfo	335
 #define __NR_perf_counter_open	336
+#define __NR_clone3		337
 
 #ifdef __KERNEL__
 

Index: linux-2.6/arch/x86/kernel/syscall_table_32.S
===================================================================
@@ -336,3 +336,4 @@ ENTRY(sys_call_table)
 	.long sys_pwritev
 	.long sys_rt_tgsigqueueinfo	/* 335 */
 	.long sys_perf_counter_open
+	.long sys_clone3

Hi Sukadev

I know I'm late to this discussion, but why the name clone3()? It's not consistent with any other convention used fo syscall naming, AFAICS. I think a name like clone_ext() or clonex() (for extended) might make more sense.

Cheers,

Michael

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface" http://blog.man7.org/

Michael Kerrisk [[email protected]] wrote:

| Hi Sukadev
| 
| On Fri, Oct 16, 2009 at 6:20 AM, Sukadev Bhattiprolu
| <[email protected]> wrote:
| > Here is an updated patch with the following interface:
| >
| >        long sys_clone3(unsigned int flags_low, struct clone_args __user *cs,
| >                        pid_t *pids);
| >
| > There are just two other (minor) changes pending to this patchset:
| >
| >        - PATCH 7: add a CLONE_UNUSED bit to VALID_CLONE_FLAGS().
| >        - PATCH 10: update documentation to reflect new interface.
| >
| > If this looks ok, we repost entire patchset next week.
| 
| I know I'm late to this discussion, but why the name clone3()? It's
| not consistent with any other convention used fo syscall naming,
| AFAICS. I think a name like clone_ext() or clonex() (for extended)
| might make more sense.

Sure, we talked about calling it clone_extended() and I can go back to that.

Only minor concern with that name was if this new call ever needs to be extended, what would we call it :-). With clone3() we could add a real/fake parameter and call it clone4() :-p

Sukadev

Perhaps clone64 (somewhat like stat64 for example)?

Cheers,
-Matt Helsley

This assumption, of course, is just plain wrong. Look at the wait system calls, for example. However, when a small integer is used like that, it pretty much always reflects numbers of arguments.

I think that doesn't exactly reflect the nature of the changes.

clone3() is actually pretty good.

-hpa

Yes. Without adopting an impractical encoding scheme it's quite unlikely a small number like 3 or 64 could exactly reflect all the changes :). I don't think that's a realistic objection though so...

I agree.

Cheers,
-Matt Helsley

My question here is: what does "3" actually mean? In general, system calls have not followed any convention of numbering to indicate successive versions -- clone2() being the one possible exception that I know of.

The only other conventions used for numbering new versions of system calls relates either to arguments size (e.g., 32 versus 64) or to their number number of arguments (dup2(), dup3(), wait3(), wait4(), accept4(), eventfd2(), inotify_init1(), epoll_create1(), evetfd2(), signalfd4()). The former convention makes some sense, but the latter is rather questionable, for a couple of reasons. One is that the number of arguments for the system call may change in the future (several of the newer system calls have a flags argument which could be used to indeicate the presence of an additional, optional argument). Another reason that the latter convention is questionable is that the number of arguments exposed to userspace by glibc may be different from the number of arguments in the raw syscall. For example, signafd4() has 4 arguments, but the glibc interface (signalfd() http://www.kernel.org/doc/man-pages/online/pages/man2/signalfd.2.html)
has 3.

Using the name clone3() follows no historical convention, which is why it seems unwise to me. Thus my suggestion of clonex() (like e.g., adjtimex()), though quite possibly there could be some better name.

Sukadev, you wrote "With clone3() we could add areal/fake parameter and call it clone4()". This rasies for me the question: should clone3() have a flags argument, so as to allow these types of extensions (i.e., not for clone flags, but rather to indicate changes in the system call interface). Yes, I understand there is a 64-bit flags in 'struct clone_struct', but I wonder whether there is any virtue in having an additional flags argument in the base signature of the function, as per the following:
http://www.kernel.org/doc/man-pages/online/pages/man2/dup3.2.html
http://www.kernel.org/doc/man-pages/online/pages/man2/signalfd4.2.html
http://www.kernel.org/doc/man-pages/online/pages/man2/eventfd2.2.html
see also
http://linux-man-pages.blogspot.com/2008/10/recent-changes-in-file-descriptor.html

By the way, one further thought: why "struct clone_struct"? We know it's a struct. Therefore, it seems pointless to include that in the name. Something like "struct clone_args" would seem less redundant and slightly more meaningful as a name.

Cheers,

Michael

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface" http://blog.man7.org/

"3" is number of arguments. It's better than "extended" or something like that simply because "extended" just means "more than", and a number at least tells you *how much more than*.

-hpa

To me, it is a version number.

mmap() and mmap2() both have 6 parameters.

Besides if wait4() were born before wait3(), would it still be wait4() :-) But I see that it is hard to get one-convention-that-fits-all.

And extended assumes we wont extend again.

An informal poll of reviewers has clone3() with a slight advantage :-)

clone_extended() camp: Serge Hallyn, Kerrisk, Louis Rilling, clone3(): Sukadev, H. Peter Anvin, Oren, Matt Helsley.

I like clone3() but am not insisting on it. I just want a name...

Sukadev

You keep bringing this up. mmap2() is (a) a non-user-visible call; (b) an exception (a mistake, if you want.)

Yes. wait3() came before wait4(), but there never was a wait2().

Exactly.

-hpa

You keep bringing this up. mmap2() is (a) a non-user-visible call; (b) an exception (a mistake, if you want.)

Yes. wait3() came before wait4(), but there never was a wait2().

Exactly.

-hpa

Sukadev,

See my precending mail. Isn't the number of arguments "2".

Yes -- that's exactly right.

Well, if we do things right in this design, we may not need to ever extend (by creating a new syscall) again. That's why I mentioned the "flags" argument idea. Did you give this some thought?

And I'm not really insisting on a change. As you rightly point out, there is much inconsistency in the naming conventions that have been used over the years.

But, because there has been no consistency in the use of numbers, and because the number of arguments that are presented in a glibc interface may differ from the number of arguments in an underlying syscall (several precedents: signalfd4(), pselect(), ppoll()), I'm inclined to think that clonex() or clone_ext() is slighly better than clone3(). But, certainly, my arguments are not compelling.

Thanks,

Michael

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface" http://blog.man7.org/

Sukadev,

See my precending mail. Isn't the number of arguments "2".

Yes -- that's exactly right.

Well, if we do things right in this design, we may not need to ever extend (by creating a new syscall) again. That's why I mentioned the "flags" argument idea. Did you give this some thought?

And I'm not really insisting on a change. As you rightly point out, there is much inconsistency in the naming conventions that have been used over the years.

But, because there has been no consistency in the use of numbers, and because the number of arguments that are presented in a glibc interface may differ from the number of arguments in an underlying syscall (several precedents: signalfd4(), pselect(), ppoll()), I'm inclined to think that clonex() or clone_ext() is slighly better than clone3(). But, certainly, my arguments are not compelling.

Thanks,

Michael

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface" http://blog.man7.org/

Michael Kerrisk [[email protected]] wrote:

| Sukadev,
| 
| On Wed, Oct 21, 2009 at 9:44 PM, Sukadev Bhattiprolu
| <[email protected]> wrote:
| > H. Peter Anvin [[email protected]] wrote:
| >> On 10/21/2009 01:26 PM, Michael Kerrisk wrote:
| >>>
| >>> My question here is: what does "3" actually mean? In general, system
| >>> calls have not followed any convention of numbering to indicate
| >>> successive versions -- clone2() being the one possible exception that
| >>> I know of.
| >>>
| >>
| >> "3" is number of arguments.
| >
| > To me, it is a version number.
| 
| See my precending mail. Isn't the number of arguments "2".

Well it was 2 at one point, but I have posted a new version of just that one patch - please see http://lkml.org/lkml/2009/10/16/3
for comments.

I am working on some updates and will post a new patchset - it will have 3 parameters to clone3() as shown in the above mail.

| 
| > mmap() and mmap2() both have 6 parameters.
| >
| > Besides if wait4() were born before wait3(), would it still be wait4() :-)
| > But I see that it is hard to get one-convention-that-fits-all.
| 
| Yes -- that's exactly right.
| 
| >> It's better than "extended" or something
| >> like that simply because "extended" just means "more than", and a number
| >> at least tells you *how much more than*.
| >
| > And extended assumes we wont extend again.
| 
| Well, if we do things right in this design, we may not need to ever
| extend (by creating a new syscall) again. That's why I mentioned the
| "flags" argument idea. Did you give this some thought?

Yes, we have done the best we can to avoid extending clone() again anytime soon (some reserved bytes and clone_args_size field). Would we still need the flags parameter ? Again its in the new patch that I pointed to above.

| 
| > An informal poll of reviewers has clone3() with a slight advantage :-)
| >
| >        clone_extended() camp: Serge Hallyn, Kerrisk, Louis Rilling,
| >        clone3(): Sukadev, H. Peter Anvin, Oren, Matt Helsley.
| >
| > I like clone3() but am not insisting on it. I just want a name...
| 
| And I'm not really insisting on a change. As you rightly point out,
| there is much inconsistency in the naming conventions that have been
| used over the years.
| 
| But, because there has been no consistency in the use of numbers, and
| because the number of arguments that are presented in a glibc
| interface may differ from the number of arguments in an underlying
| syscall (several precedents: signalfd4(), pselect(), ppoll()), I'm
| inclined to think that clonex() or clone_ext() is slighly better than
| clone3(). But, certainly, my arguments are not compelling.

Michael Kerrisk [mtk.manpages-gM/[email protected]] wrote:

| Sukadev,
| 
| On Wed, Oct 21, 2009 at 9:44 PM, Sukadev Bhattiprolu
| <[email protected]> wrote:
| > H. Peter Anvin [[email protected]] wrote:
| >> On 10/21/2009 01:26 PM, Michael Kerrisk wrote:
| >>>
| >>> My question here is: what does "3" actually mean? In general, system
| >>> calls have not followed any convention of numbering to indicate
| >>> successive versions -- clone2() being the one possible exception that
| >>> I know of.
| >>>
| >>
| >> "3" is number of arguments.
| >
| > To me, it is a version number.
| 
| See my precending mail. Isn't the number of arguments "2".

Well it was 2 at one point, but I have posted a new version of just that one patch - please see http://lkml.org/lkml/2009/10/16/3
for comments.

I am working on some updates and will post a new patchset - it will have 3 parameters to clone3() as shown in the above mail.

| 
| > mmap() and mmap2() both have 6 parameters.
| >
| > Besides if wait4() were born before wait3(), would it still be wait4() :-)
| > But I see that it is hard to get one-convention-that-fits-all.
| 
| Yes -- that's exactly right.
| 
| >> It's better than "extended" or something
| >> like that simply because "extended" just means "more than", and a number
| >> at least tells you *how much more than*.
| >
| > And extended assumes we wont extend again.
| 
| Well, if we do things right in this design, we may not need to ever
| extend (by creating a new syscall) again. That's why I mentioned the
| "flags" argument idea. Did you give this some thought?

Yes, we have done the best we can to avoid extending clone() again anytime soon (some reserved bytes and clone_args_size field). Would we still need the flags parameter ? Again its in the new patch that I pointed to above.

| 
| > An informal poll of reviewers has clone3() with a slight advantage :-)
| >
| >        clone_extended() camp: Serge Hallyn, Kerrisk, Louis Rilling,
| >        clone3(): Sukadev, H. Peter Anvin, Oren, Matt Helsley.
| >
| > I like clone3() but am not insisting on it. I just want a name...
| 
| And I'm not really insisting on a change. As you rightly point out,
| there is much inconsistency in the naming conventions that have been
| used over the years.
| 
| But, because there has been no consistency in the use of numbers, and
| because the number of arguments that are presented in a glibc
| interface may differ from the number of arguments in an underlying
| syscall (several precedents: signalfd4(), pselect(), ppoll()), I'm
| inclined to think that clonex() or clone_ext() is slighly better than
| clone3(). But, certainly, my arguments are not compelling.

To me, it is a version number.

mmap() and mmap2() both have 6 parameters.

Besides if wait4() were born before wait3(), would it still be wait4() :-) But I see that it is hard to get one-convention-that-fits-all.

And extended assumes we wont extend again.

An informal poll of reviewers has clone3() with a slight advantage :-)

clone_extended() camp: Serge Hallyn, Kerrisk, Louis Rilling, clone3(): Sukadev, H. Peter Anvin, Oren, Matt Helsley.

I like clone3() but am not insisting on it. I just want a name...

Sukadev

Peter,

sys_clone3(struct clone_struct __user *ucs, pid_t __user *pids)

It appears to me that the number of arguments is 2.

I'm not sure why you think including a number in the name tells us "how much more than". Unless you are considering the numbering to be version numbers, which apparently is not what you mean.

Thanks,

Michael

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface" http://blog.man7.org/

Peter,

sys_clone3(struct clone_struct __user *ucs, pid_t __user *pids)

It appears to me that the number of arguments is 2.

I'm not sure why you think including a number in the name tells us "how much more than". Unless you are considering the numbering to be version numbers, which apparently is not what you mean.

Thanks,

Michael

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface" http://blog.man7.org/

It was 3 at one point... I'm not sure when that changed last :-/

It is a version number of sorts.

-hpa

Peter,

So, sometimes, a number in a system call should be the bit width of some arguments(s), sometimes it should be the number of arguments, and sometimes (well, just occasionally, as in mmap2() and clone()) -- it should be a version number? Does the weather play any part in the decision? ;-)

Cheers,

Michael

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface" http://blog.man7.org/

The notion is that they are *some* kind of description on how the system call has been augmented. The bitwidths and argument numbers are non-overlapping and visually very different, so are not subject to confusion. Your argument makes about as much sense as saying the letter "a" should have the same meaning in every context.

-hpa

The notion is that they are *some* kind of description on how the system call has been augmented. The bitwidths and argument numbers are non-overlapping and visually very different, so are not subject to confusion. Your argument makes about as much sense as saying the letter "a" should have the same meaning in every context.

-hpa

In this case 3 could be both the number of arguments and the version number (clone, clone2, clone3). Match 2 conventions with one choice of name. :)

Cheers,
-Matt Helsley

In this case 3 could be both the number of arguments and the version number (clone, clone2, clone3). Match 2 conventions with one choice of name. :)

Cheers,
-Matt Helsley

Peter,

So, sometimes, a number in a system call should be the bit width of some arguments(s), sometimes it should be the number of arguments, and sometimes (well, just occasionally, as in mmap2() and clone()) -- it should be a version number? Does the weather play any part in the decision? ;-)

Cheers,

Michael

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface" http://blog.man7.org/

It was 3 at one point... I'm not sure when that changed last :-/

It is a version number of sorts.

-hpa

"3" is number of arguments. It's better than "extended" or something like that simply because "extended" just means "more than", and a number at least tells you *how much more than*.

-hpa

My question here is: what does "3" actually mean? In general, system calls have not followed any convention of numbering to indicate successive versions -- clone2() being the one possible exception that I know of.

The only other conventions used for numbering new versions of system calls relates either to arguments size (e.g., 32 versus 64) or to their number number of arguments (dup2(), dup3(), wait3(), wait4(), accept4(), eventfd2(), inotify_init1(), epoll_create1(), evetfd2(), signalfd4()). The former convention makes some sense, but the latter is rather questionable, for a couple of reasons. One is that the number of arguments for the system call may change in the future (several of the newer system calls have a flags argument which could be used to indeicate the presence of an additional, optional argument). Another reason that the latter convention is questionable is that the number of arguments exposed to userspace by glibc may be different from the number of arguments in the raw syscall. For example, signafd4() has 4 arguments, but the glibc interface (signalfd() http://www.kernel.org/doc/man-pages/online/pages/man2/signalfd.2.html)
has 3.

Using the name clone3() follows no historical convention, which is why it seems unwise to me. Thus my suggestion of clonex() (like e.g., adjtimex()), though quite possibly there could be some better name.

Sukadev, you wrote "With clone3() we could add areal/fake parameter and call it clone4()". This rasies for me the question: should clone3() have a flags argument, so as to allow these types of extensions (i.e., not for clone flags, but rather to indicate changes in the system call interface). Yes, I understand there is a 64-bit flags in 'struct clone_struct', but I wonder whether there is any virtue in having an additional flags argument in the base signature of the function, as per the following:
http://www.kernel.org/doc/man-pages/online/pages/man2/dup3.2.html
http://www.kernel.org/doc/man-pages/online/pages/man2/signalfd4.2.html
http://www.kernel.org/doc/man-pages/online/pages/man2/eventfd2.2.html
see also
http://linux-man-pages.blogspot.com/2008/10/recent-changes-in-file-descriptor.html

By the way, one further thought: why "struct clone_struct"? We know it's a struct. Therefore, it seems pointless to include that in the name. Something like "struct clone_args" would seem less redundant and slightly more meaningful as a name.

Cheers,

Michael

Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface" http://blog.man7.org/

Yes. Without adopting an impractical encoding scheme it's quite unlikely a small number like 3 or 64 could exactly reflect all the changes :). I don't think that's a realistic objection though so...

I agree.

Cheers,
-Matt Helsley

This assumption, of course, is just plain wrong. Look at the wait system calls, for example. However, when a small integer is used like that, it pretty much always reflects numbers of arguments.

I think that doesn't exactly reflect the nature of the changes.

clone3() is actually pretty good.

-hpa

Perhaps clone64 (somewhat like stat64 for example)?

Cheers,
-Matt Helsley

Michael Kerrisk [mtk.manpages-gM/[email protected]] wrote:

| Hi Sukadev
| 
| On Fri, Oct 16, 2009 at 6:20 AM, Sukadev Bhattiprolu
| <[email protected]> wrote:
| > Here is an updated patch with the following interface:
| >
| >        long sys_clone3(unsigned int flags_low, struct clone_args __user *cs,
| >                        pid_t *pids);
| >
| > There are just two other (minor) changes pending to this patchset:
| >
| >        - PATCH 7: add a CLONE_UNUSED bit to VALID_CLONE_FLAGS().
| >        - PATCH 10: update documentation to reflect new interface.
| >
| > If this looks ok, we repost entire patchset next week.
| 
| I know I'm late to this discussion, but why the name clone3()? It's
| not consistent with any other convention used fo syscall naming,
| AFAICS. I think a name like clone_ext() or clonex() (for extended)
| might make more sense.

Sure, we talked about calling it clone_extended() and I can go back to that.

Only minor concern with that name was if this new call ever needs to be extended, what would we call it :-). With clone3() we could add a real/fake parameter and call it clone4() :-p

Sukadev

Subject: [RFC][v8][PATCH 10/10]: Document clone3() syscall

This gives a brief overview of the clone3() system call. We should eventually describe more details in existing clone(2) man page or in a new man page.

Changelog[v8]:
- clone2() is already in use in IA64. Rename syscall to clone3()
- Add notes to say that we return -EINVAL if invalid clone flags are specified or if the reserved fields are not 0.
Changelog[v7]:
- Rename clone_with_pids() to clone2()
- Changes to reflect new prototype of clone2() (using clone_struct).

---
 Documentation/clone2 |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

Index: linux-2.6/Documentation/clone2
===================================================================
@@ -0,0 +1,89 @@
+
+struct clone_struct {
+	u64 flags;
+	u64 child_stack;
+	u32 nr_pids;
+	u32 reserved1;
+	u64 parent_tid;
+	u64 child_tid;
+	u64 reserved2;
+};
+
+clone3(struct clone_struct * __user clone_args, pid_t * __user pids)
+
+	In addition to doing everything that clone() system call does,
+	the clone3() system call:
+
+		- allows additional clone flags (all 32 bits in the flags
+		  parameter to clone() are in use)
+
+		- allows user to specify a pid for the child process in its
+		  active and ancestor pid name spaces.
+
+	This system call is meant to be used when restarting an application
+	from a checkpoint.  Such restart requires that the processes in the
+	application have the same pids they had when the application was
+	checkpointed. When containers are nested, the processes within the
+	containers exist in multiple pid namespaces and hence have multiple
+	pids to specify during restart.
+
+	The @pids defines the set of pids that should be assigned to the child
+	process in its active and ancestor pid name spaces. The descendant pid
+	namespaces do not matter since a process does not have a pid in
+	descendant namespaces, unless the process is in a new pid namespace
+	in which case the process is a container-init (and must have the pid 1
+	in that namespace).
+
+	See CLONE_NEWPID section of clone(2) man page for details about pid
+	namespaces.
+
+	The order pids in @pids corresponds to the nesting order of pid-
+	namespaces, with @pids[0] corresponding to the init_pid_ns.
+
+	If a pid in the @pids list is 0, the kernel will assign the next
+	available pid in the pid namespace, for the process.
+
+	If a pid in the @pids list is non-zero, the kernel tries to assign
+	the specified pid in that namespace.  If that pid is already in use
+	by another process, the system call fails with -EBUSY.
+
+	On success, the system call returns the pid of the child process in
+	the parent's active pid namespace.
+
+	On failure, clone3() returns -1 and sets 'errno' to one of following
+	values (the child process is not created).
+
+	EPERM	Caller does not have the SYS_ADMIN privilege needed to excute
+		this call.
+
+	EINVAL	The number of pids specified in 'clone_struct.nr_pids' exceeds
+		the current nesting level of parent process
+
+	EINVAL	Not all specified clone-flags are valid.
+
+	EINVAL	The reserved fields in the clone_struct argument are not 0.
+
+	EBUSY	A requested pid is in use by another process in that name space.
+
+Example:
+
+	pid_t pids[] = { 77, 99 };
+	struct clone_struct cs;
+
+	cs.flags = (u64) SIGCHLD;
+	cs.child_stack = (u64) setup_child_stack();
+	cs.nr_pids = 2;
+	cs.parent_tid = 0LL;
+	cs.child_tid = 0LL;
+
+	rc = syscall(__NR_clone3, &cs, pids);
+
+	if (rc < 0) {
+		perror("clone3()");
+		exit(1);
+	} else if (rc) {
+		/* Parent */
+	} else {
+		/* Child */
+	}
+

Arnd Bergmann [[email protected]] wrote:

| On Tuesday 13 October 2009, Sukadev Bhattiprolu wrote:
| > +clone3(struct clone_struct * __user clone_args, pid_t * __user pids)
| > +
| > +       In addition to doing everything that clone() system call does,
| > +       the clone3() system call:
| > +
| > +               - allows additional clone flags (all 32 bits in the flags
| > +                 parameter to clone() are in use)
| > +
| > +               - allows user to specify a pid for the child process in its
| > +                 active and ancestor pid name spaces.
| 
| Someone (sorry, can't find the old mail) pointed out last time that
| the 'pid_t *__user tidptr' argument needs to be an independent pointer,
| in order to allow the same use patterns with CLONE_CHILD_SETTID and
| CLONE_CHILD_CLEARTID that you can do with the current clone
| implementation.

I think it was Oren Laadan.

| 
| Moving that argument from clone_struct into the argument list would
| also make this a three-argument syscall, which solves the naming problem.

How about we pull 'nr_pids' out of clone_args struct and make that a separate parameter. It seems the odd man out and controls the pid_t list parameter.
|
| Arnd <><

Hi!

Thanks!

M. Kerrisk (see MAINTAINERS) maintains man pages...

...

...

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

That would make it incompatible between 64 bit kernels and 32 bit user space, requiring a wrapper. Better leave it at u64.

Quite the opposite is true.

The man page describes what the user space sees, which is errno. Returning -ERRCODE to libc from the kernel is part of the architecture specific kernel ABI and should not be documented in this place but in the architecture documentation.

I suggested doing that, it's a lot easier to handle fixed length data structures than an array at the end.

	Arnd <><

Ok.

Well, it is still inconsistent. Half the docs talks -ERRCODE, half talks -1/errno=ERRCODE.

You could still put the something * pids at the end of (fixed length) structure.

(Not that I agree with the argument, pid array is variable-length, anyway, and inlining it at the end of structure should not make code more complex).

Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Please read up on the v1 to v7 patches, we've been through all this before. There are obviously a lot of ways to do it, many of which are equally good. The important thing to note is that the user-visible interface should *not* pass the structure into the library that provides this function but build the structure itself, so it doesn't matter that much.

If you put the array at the end of the data structure, the wrapper will have to copy the array provided by the user to a temporary buffer. Passing the array as a separate syscall argument avoids this.

	Arnd <><

Ok, sounds sane, sorry for noise.
Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Ok, sounds sane, sorry for noise.
Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Please read up on the v1 to v7 patches, we've been through all this before. There are obviously a lot of ways to do it, many of which are equally good. The important thing to note is that the user-visible interface should *not* pass the structure into the library that provides this function but build the structure itself, so it doesn't matter that much.

If you put the array at the end of the data structure, the wrapper will have to copy the array provided by the user to a temporary buffer. Passing the array as a separate syscall argument avoids this.

	Arnd <><

Ok.

Well, it is still inconsistent. Half the docs talks -ERRCODE, half talks -1/errno=ERRCODE.

You could still put the something * pids at the end of (fixed length) structure.

(Not that I agree with the argument, pid array is variable-length, anyway, and inlining it at the end of structure should not make code more complex).

Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

That would make it incompatible between 64 bit kernels and 32 bit user space, requiring a wrapper. Better leave it at u64.

Quite the opposite is true.

The man page describes what the user space sees, which is errno. Returning -ERRCODE to libc from the kernel is part of the architecture specific kernel ABI and should not be documented in this place but in the architecture documentation.

I suggested doing that, it's a lot easier to handle fixed length data structures than an array at the end.

	Arnd <><

Pavel Machek [[email protected]] wrote:

| Hi!
| 
| > This gives a brief overview of the clone3() system call.  We should
| 
| Thanks!
| 
| > eventually describe more details in existing clone(2) man page or in
| > a new man page.
| 
| M. Kerrisk (see MAINTAINERS) maintains man pages...

Ok. I copied linux-api and M. Kerrisk is looking at this patchset.

| 
| > Changelog[v8]:
| > 	- clone2() is already in use in IA64. Rename syscall to clone3()
| ...
| > Index: linux-2.6/Documentation/clone2
| > ===================================================================
| > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
| > +++ linux-2.6/Documentation/clone2	2009-10-12 19:54:38.000000000 -0700
| 
| clone3?

Ah, yes, will fix.

| 
| > +struct clone_struct {
| > +	u64 flags;
| > +	u64 child_stack;
| 
| u64 seems wrong  on 32 bit platforms. ulong?
| 
| > +	u32 nr_pids;
| 
| So nr_pids is either 1 or 2?

No. With pid namespaces, which can be nested to arbitrary levels, each process has several pid numbers - one in its own namespace and one in each ancestor pid namespaces.

nr_pids specifies the number of pids the user cares about. IOW, if you checkpoint a process that is 3 levels below the init-pid-ns, and you care about the pids for those three levels, you would pass in an array of 4 pid_ts and nr_pids would be 4.

| 
| > +	u32 reserved1;
| > +	u64 parent_tid;
| > +	u64 child_tid;
| > +	u64 reserved2;
| > +};
| > +
| 
| 
| > +	See CLONE_NEWPID section of clone(2) man page for details about pid
| > +	namespaces.
| > +
| > +	The order pids in @pids corresponds to the nesting order of pid-
| > +	namespaces, with @pids[0] corresponding to the init_pid_ns.
| 
| Ok, so I'm confused.

Hope the above note on pid-namespaces helps, if not let me know.

| 
| > +	If a pid in the @pids list is 0, the kernel will assign the next
| > +	available pid in the pid namespace, for the process.
| > +
| > +	If a pid in the @pids list is non-zero, the kernel tries to assign
| > +	the specified pid in that namespace.  If that pid is already in use
| > +	by another process, the system call fails with -EBUSY.
| ...
| > +	On failure, clone3() returns -1 and sets 'errno' to one of following
| > +	values (the child process is not created).
| 
| Inconsistent with above. Syscalls really return -ERRCODE, errno is
| glibc magic.

Ok. Will make it consistent to say that it returns one of the error codes.

Pavel Machek [[email protected]] wrote:

| Hi!
| 
| > This gives a brief overview of the clone3() system call.  We should
| 
| Thanks!
| 
| > eventually describe more details in existing clone(2) man page or in
| > a new man page.
| 
| M. Kerrisk (see MAINTAINERS) maintains man pages...

Ok. I copied linux-api and M. Kerrisk is looking at this patchset.

| 
| > Changelog[v8]:
| > 	- clone2() is already in use in IA64. Rename syscall to clone3()
| ...
| > Index: linux-2.6/Documentation/clone2
| > ===================================================================
| > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
| > +++ linux-2.6/Documentation/clone2	2009-10-12 19:54:38.000000000 -0700
| 
| clone3?

Ah, yes, will fix.

| 
| > +struct clone_struct {
| > +	u64 flags;
| > +	u64 child_stack;
| 
| u64 seems wrong  on 32 bit platforms. ulong?
| 
| > +	u32 nr_pids;
| 
| So nr_pids is either 1 or 2?

No. With pid namespaces, which can be nested to arbitrary levels, each process has several pid numbers - one in its own namespace and one in each ancestor pid namespaces.

nr_pids specifies the number of pids the user cares about. IOW, if you checkpoint a process that is 3 levels below the init-pid-ns, and you care about the pids for those three levels, you would pass in an array of 4 pid_ts and nr_pids would be 4.

| 
| > +	u32 reserved1;
| > +	u64 parent_tid;
| > +	u64 child_tid;
| > +	u64 reserved2;
| > +};
| > +
| 
| 
| > +	See CLONE_NEWPID section of clone(2) man page for details about pid
| > +	namespaces.
| > +
| > +	The order pids in @pids corresponds to the nesting order of pid-
| > +	namespaces, with @pids[0] corresponding to the init_pid_ns.
| 
| Ok, so I'm confused.

Hope the above note on pid-namespaces helps, if not let me know.

| 
| > +	If a pid in the @pids list is 0, the kernel will assign the next
| > +	available pid in the pid namespace, for the process.
| > +
| > +	If a pid in the @pids list is non-zero, the kernel tries to assign
| > +	the specified pid in that namespace.  If that pid is already in use
| > +	by another process, the system call fails with -EBUSY.
| ...
| > +	On failure, clone3() returns -1 and sets 'errno' to one of following
| > +	values (the child process is not created).
| 
| Inconsistent with above. Syscalls really return -ERRCODE, errno is
| glibc magic.

Ok. Will make it consistent to say that it returns one of the error codes.

Hi!

Thanks!

M. Kerrisk (see MAINTAINERS) maintains man pages...

...

...

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Some userland debugging things and so forth like to look at the clone_flags argument, so that is kept simpler for them if it stays in a register (i.e. its own argument) rather than a user pointer fetch for that argument. Any problem with:

sys_clone3(unsigned long clone_flags,
struct clone_struct __user *cs, pid_t __user *pids)

?

That also has the side benefit that instead of non-ia64 users forever asking, "Why is it clone3 when there is no clone2?" you can instead pretend that it follows the "clone3 because it takes three arguments" convention. ;-)

Btw, IMHO "struct foo_struct" is one of the lamest naming conventions ever. How about "struct clone_args"?

Also, if you were to replace:

		u64 child_stack;

with:

		u64 child_stack_base;
		u64 child_stack_size;

and use in sys_clone3 (for most arch's):

	child_stack_ptr = kcs.child_stack_base + kcs.child_stack_size;

then the same clone3 interface would cover ia64 as well.

Thanks,
Roland

Roland McGrath [[email protected]] wrote:

| Some userland debugging things and so forth like to look at the clone_flags
| argument, so that is kept simpler for them if it stays in a register
| (i.e. its own argument) rather than a user pointer fetch for that argument.
| Any problem with:
| 
| 	sys_clone3(unsigned long clone_flags,
| 		   struct clone_struct __user *cs, pid_t __user *pids)
| 
| ?

My only concern is the support of 64-bit clone flags on 32-bit architectures. We would need an additional register/parameter, clone_flags_high ? Also, hopefully we won't need more than 64 flags, but if we do, the plan AFACIT, was that we could use one of the reserved fields.

| 
| That also has the side benefit that instead of non-ia64 users forever
| asking, "Why is it clone3 when there is no clone2?" you can instead pretend
| that it follows the "clone3 because it takes three arguments" convention. ;-)
| 
| Btw, IMHO "struct foo_struct" is one of the lamest naming conventions ever.
| How about "struct clone_args"?

Sure :-) In earlier version of patches I had mixed up clone_struct and clone_arg in comments/descriptions and cleaned up in this version.

| 
| Also, if you were to replace:
| 
| 		u64 child_stack;
| 
| with:
| 
| 		u64 child_stack_base;
| 		u64 child_stack_size;
| 
| and use in sys_clone3 (for most arch's):
| 
| 	child_stack_ptr = kcs.child_stack_base + kcs.child_stack_size;
| 
| then the same clone3 interface would cover ia64 as well.

Ok

| 
| 
| Thanks,
| Roland

Oy. I didn't realize there was serious consideration of having more than 32 flags. IMHO it would be a bad choice, since they could only be used via clone3. Having high-bit flags work in clone on 64-bit machines but not on 32-bit machines just seems like a wrongly confusing way for things to be. If any high-bits flags are constrained even on 64-bit machines to uses in clone3 calls for sanity purposes, then it seems questionable IMHO to have them be more flags in the same u64 at all.

Since all new features will be via this struct, various new kinds of things could potentially be done by other new struct fields independent of flags. But that would of course require putting enough reserved fields in now and requiring that they be zero-filled now in anticipation of such future uses, which is not very pleasant either.

In short, I guess I really am saying that "clone_flags_high" (or "more_flags" or something) does seem better to me than any of the possibilities for having more than 32 CLONE_* in the current flags word.

Thanks,
Roland

Overall it seems sane to:

a) make it an actual 3-argument call;
b) make the existing flags a u32 forever, and make it a separate argument;
c) any new expansion can be via the struct, which may want to have an "c3_flags" field first in the structure.

-hpa

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

H. Peter Anvin [[email protected]] wrote:

| On 10/13/2009 04:53 PM, Roland McGrath wrote:
| >> My only concern is the support of 64-bit clone flags on 32-bit architectures.
| > 
| > Oy.  I didn't realize there was serious consideration of having more than
| > 32 flags.  IMHO it would be a bad choice, since they could only be used via
| > clone3.  Having high-bit flags work in clone on 64-bit machines but not on
| > 32-bit machines just seems like a wrongly confusing way for things to be.
| > If any high-bits flags are constrained even on 64-bit machines to uses in
| > clone3 calls for sanity purposes, then it seems questionable IMHO to have
| > them be more flags in the same u64 at all.
| > 
| > Since all new features will be via this struct, various new kinds of things
| > could potentially be done by other new struct fields independent of flags.
| > But that would of course require putting enough reserved fields in now and
| > requiring that they be zero-filled now in anticipation of such future uses,
| > which is not very pleasant either.
| > 
| > In short, I guess I really am saying that "clone_flags_high" (or
| > "more_flags" or something) does seem better to me than any of the
| > possibilities for having more than 32 CLONE_* in the current flags word.
| > 
| 
| Overall it seems sane to:
| 
| a) make it an actual 3-argument call;
| b) make the existing flags a u32 forever, and make it a separate
|    argument;
| c) any new expansion can be via the struct, which may want to have
|    an "c3_flags" field first in the structure.

Wouldn't that make it a somewhat awkward interface - applications or libc have to split the clone flags and specify them in two different fields ? Do we need to preserve the u32 flags forever ?

The closest example I can think of is the distinction between signal() and sigaction(). sigaction() interface makes a lot of sense in part bc it does not try to look like signal(). And strace seems to be able decode flags like SA_RESTART and SA_SIGINFO with the pointer to the sigaction buffer.

Won't the debug tools be able to do the same with a pointer to struct clone_args ?

If it needs the new features of clone3(), the application has to be modified. At that point wouldn't it be easy to write a clone3() wrapper that calls sys_clone3() if it exists or call sys_clone() if not ?

If clone3() supersedes clone(), all architectures could use the clone3() interface and it would seem best to make clone3() as simple as possible without having to preserve the u32 flags.

Would it help to use a type clone_flags_64_t to make the distinction between types more explicit ?

Sukadev

The problem with using the same flags in two places, one as a 32-bit and one as a 64-bit number, is that using one in the wrong place will cause silent, but deadly, truncation.

-hpa

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

H. Peter Anvin [[email protected]] wrote:

| On 10/13/2009 04:53 PM, Roland McGrath wrote:
| >> My only concern is the support of 64-bit clone flags on 32-bit architectures.
| > 
| > Oy.  I didn't realize there was serious consideration of having more than
| > 32 flags.  IMHO it would be a bad choice, since they could only be used via
| > clone3.  Having high-bit flags work in clone on 64-bit machines but not on
| > 32-bit machines just seems like a wrongly confusing way for things to be.
| > If any high-bits flags are constrained even on 64-bit machines to uses in
| > clone3 calls for sanity purposes, then it seems questionable IMHO to have
| > them be more flags in the same u64 at all.
| > 
| > Since all new features will be via this struct, various new kinds of things
| > could potentially be done by other new struct fields independent of flags.
| > But that would of course require putting enough reserved fields in now and
| > requiring that they be zero-filled now in anticipation of such future uses,
| > which is not very pleasant either.
| > 
| > In short, I guess I really am saying that "clone_flags_high" (or
| > "more_flags" or something) does seem better to me than any of the
| > possibilities for having more than 32 CLONE_* in the current flags word.
| > 
| 
| Overall it seems sane to:
| 
| a) make it an actual 3-argument call;
| b) make the existing flags a u32 forever, and make it a separate
|    argument;
| c) any new expansion can be via the struct, which may want to have
|    an "c3_flags" field first in the structure.

Ok, So will this work ?

	struct clone_args {
		u32 flags_high;		/* new clone flags (higher bits) */ 
		u32 reserved1;
		u32 nr_pids;
		u32 reserved2;
		u64 child_stack_base;
		u64 child_stack_size;
		u64 parent_tid_ptr;
		u64 child_tid_ptr;
		u64 reserved3;
	};

	sys_clone3(u32 flags_low, struct clone_args *args, pid_t *pid_list)

Even on 64bit architectures the applications have to use sys_clone3() for the extended features.

Yes, although I'd just make flags_high a u64. The other thing that might be worthwhile is to have a length field on the structure; that way we could add new fields at the end if ever necessary in the future.

-hpa

H. Peter Anvin [[email protected]] wrote:

| On 10/14/2009 03:36 PM, Sukadev Bhattiprolu wrote:
| > H. Peter Anvin [[email protected]] wrote:
| > | 
| > | Overall it seems sane to:
| > | 
| > | a) make it an actual 3-argument call;
| > | b) make the existing flags a u32 forever, and make it a separate
| > |    argument;
| > | c) any new expansion can be via the struct, which may want to have
| > |    an "c3_flags" field first in the structure.
| > 
| > Ok, So will this work ?
| > 
| > 	struct clone_args {
| > 		u32 flags_high;		/* new clone flags (higher bits) */ 
| > 		u32 reserved1;
| > 		u32 nr_pids;
| > 		u32 reserved2;
| > 		u64 child_stack_base;
| > 		u64 child_stack_size;
| > 		u64 parent_tid_ptr;
| > 		u64 child_tid_ptr;
| > 		u64 reserved3;
| > 	};
| > 
| > 	sys_clone3(u32 flags_low, struct clone_args *args, pid_t *pid_list)
| > 
| > Even on 64bit architectures the applications have to use sys_clone3() for
| > the extended features.
| 
| Yes, although I'd just make flags_high a u64.

so we allow 96 bits for flags ?

| The other thing that might be worthwhile is to have a length field on
| the structure; that way we could add new fields at the end if ever
| necessary in the future.

So:

	struct clone_args {
		u64 flags_high;		/* new clone flags (higher bits) */ 
		u64 reserved1;

		u32 nr_pids;
		u32 clone_args_size;

		u64 child_stack_base;
		u64 child_stack_size;

		u64 parent_tid_ptr;
		u64 child_tid_ptr;
	};

	sys_clone3(u32 flags_low, struct clone_args *args, pid_t *pid_list)

BTW, on 64-bit architectures, the flags_low would be 64-bits, but the high- bits there would be ignored right ?

Not sure if we need a second reserved field now that we add ->clone_args_size.

Thanks,

Sukadev

Yes, although I'd just make flags_high a u64. The other thing that might be worthwhile is to have a length field on the structure; that way we could add new fields at the end if ever necessary in the future.

-hpa

A system call named clone3() taking two parameters is just too weird to live. No, please.

-hpa

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Except we can't use clone2() because it conflicts on ia64. Care to propose a name you would prefer?

Also I was a bit suprised to discover there are plenty of examples where this convention has not been followed: vm86, lseek64, and mmap2 to name a few. In fact, of the 46 __NR_foo[[:digit:]]+, 36 break this convention on x86-32.

Cheers,
-Matt Helsley

The -86, -64 and so on are visually obviously not a parameter count. sys_mmap2 is not user visible, and so doesn't really matter.

-hpa

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

H. Peter Anvin [[email protected]] wrote:

| > 
| > Except we can't use clone2() because it conflicts on ia64.  Care to propose
| > a name you would prefer?

Yes, I am running out of ideas :-)

How about clone64_with_pids() ? - hope we don't need a 65th clone-flag :p

never_clone_alone() ;)

-hpa

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Well, maybe accept that adding more flags will still require a new syscall, and call this clone_with_pids(), and only take a single 32-bit flags field?

Or, go back to clone_extended().

-serge

Sorry for jumping so late in the discussion and for having maybe my remarks pointless...

If this syscall is only for checkpoint / restart, why this shouldn't be used with a future generic sys_restart syscall ?
Otherwise, shouldn't be more convenient to have something usable for everyone, let's say:

cloneat(pid_t pid, pid_t desiredpid, ...);

Where 'desiredpid' is a hint of for the kernel for the pid to be allocated (zero means the kernel will choose one for us) and the newly allocated task is the son of 'pid'.
That looks more consistent with the "<syscall>at" family, 'openat', 'faccessat', 'readlinkat', etc ... and usable for something else than the checkpoint / restart.

Thanks
-- Daniel

As I tried to explain in PATCH 0/9, the ability to choose a pid is only for C/R but we are also trying to clone-flags so we won't need yet another variant of clone() fairly soon.

Hmm, so P1 would call cloneat() to create a child P3 _on behalf_ of process P2 ? I did not know we had a requirement for that. Can you explain the use-case more ? IOW, why can't P2 create the child P3 by itself ?

Note also that 'desiredpid' must be a list of pids (one for each pid namespaces that the child will belong to) and hence we need 'nr_pids' to specify the list. Given that we are limited to 6 parameters to the syscall, such parameters must be stuffed into 'struct clone_args'.

So we should do something like:

sys_clone3(u32 flags_low, pid_t pid, struct clone_args *carg,
pid_t *desired_pids)

or (to match the name and parameters, move 'pid' parameter into clone_args)

The subtle difference though is that openat() does not open a file on behalf of another process and so the 'at' suffix would not apply ?

I forgot to mention a constraint with the specified pid : P2 has to be child of P1. In other word, you can not specify a pid to clonat which is not your descendant (including yourself). With this constraint I think there is no security issues.

Concerning of forking on behalf of another process, we can consider it is up to the caller / programmer to know what it does. If a process in the process hierarchy exec'ed a program and we cloneat this process and then the program fails because of an "unexpected error", well, we should have not done that. A similar example is when the IPC are removed while they are used by some other processes.

Here it is a interesting use case:
* if you created a pid namespace, and, let's say, booted a system container where the container init is the "init" process, then with this call you can enter the container at any time by doing cloneat() followed by an exec of your command. I think that was a requirement when there were discussions around "sys_hijack".

Another point. It's another way to extend the exhausted clone flags as the cloneat can be called as a compatibility way, with cloneat(getpid(), 0, ... )

Well, hiding multiple clone in one clone call is ... weird. AFAIR, there was a debate between kernel or userspace proctree creation but it looks like it's done from the kernel with this call.

I don't really see a difference between sys_restart(pid_t pid , int fd, long flags) where pid_t is the topmost in the hierarchy, fd is a file descriptor to a structure "pid_t * + struct clone_args *" and flags is "PROCTREE".

IMHO, it is nicer to recursively restore the process tree for the nested pid namespaces, that will be really an userspace process tree creation and cloneat will be your friend here :)

Yes and no, depending of where you put the cursor. If you consider the 'at' suffix means a process context, then I agree with you, there is a difference because the cloneat will be out of the current process context. But if you consider the 'at' suffix as a context in general, and openat means "relatively to a file descriptor" and cloneat means "relatively to a pid namespace" the 'at' suffix may apply. But I agree that we are so used to call the posix "fork", that cloneat sounds scary :)

Thanks
-- Daniel

[...]

Sounds dangerous. What if your descendant executed a setuid program ?

Before the user can program with this syscall, _you_ need to define the semantics of this syscall. For example, do you freeze that other process to ensure that the clone is consistent ?

Can you define more precisely what you mean by "enter" the container ?

If you simply want create a new process in the container, you can achieve the same thing with a daemon, or a smart init process (in there), or even ptrace tricks.

Also, there is a reason why sys_hijack() was hijacked away ... And I honestly think that a syscall to force another process to clone would be shot down by the kernel guys.

Which is what the proposed new clone_....() does.

It isn't multiple clones in one clone. The syscall creates *one* single process. We just ask the kernel to assign a specific pid to that process.

And because processes that live in nested pid-namespace own multiple pids (one for each level), we need to specify multiple pids (one for each level) for this single process that we create.

Then, to create the entire restart process tree, we have to call this system call as many times as the number of processes to restart.

And yes, this is all done in userspace. The _only_ kernel-space help is an interface to request specific pid(s) for each restarted process.

What you describe here is the way openvz's kernel-side process tree restart works. This is not the current approach.

The restart.c utility from the userspace tools (user-cr) is your friend here... I'm sure you'll think it's nice :p

At this point, I really couldn't care less about how we name the new syscall.

Check out the containers IRC channel for the latest pearls:
clone_plus_with_aloe() and clone_plus_with_aloe_3_args() are the prominent contenders, together with xerox() and ditto()...

There you go; these should be enough to keep the discussion around on life support for at least another week.

I really really really hope we can settle down on *a* name, *any* name, and move forward. Amen.

Oren.

<snip>

Just to be clear -- Suka's proposing to extend the clone flags. However I don't believe reusing the "pid" parameters as Daniel seemed to suggest was ever part of Suka's proposed changes.

<snip>

I think the difference has to do with keeping the code maintainable.

Clone creates the process so it's already involved in allocating and assigning pids to the new task. Switching pids at sys_restart() would add another point in the code where pids are allocated and assigned. This suggests we may have to worry about introducing new obscure races for anyone who's working on the pid allocator to be careful of. At least when all the code is "localized" to the clone paths we can be reasonably certain of proper maintenance.

<snip>

clone3() seemed to be the leading contender from what I've read so far. Does anyone still object to clone3() after reading the whole thread?

Cheers,
-Matt Helsley

I object to what clone3() is. The name is not particularly interesting.

The sanity checks for assigning pids are missing and there is a todo about it. I am not comfortable with assigning pids to a new process in a pid namespace with other processes user space processes executing in it.

How we handle a clone extension depends critically on if we want to create a processes for restart in user space or kernel space.

Could some one give me or point me at a strong case for creating the processes for restart in user space?

The pid assignment code is currently ugly. I asked that we just pass in the min max pid pids that already exist into the core pid assignment function and a constrained min/max that only admits a single pid when we are allocating a struct pid for restart. That was not done and now we have a weird abortion with unnecessary special cases.

Eric

Eric W. Biederman [[email protected]] wrote:

| > clone3() seemed to be the leading contender from what I've read so far.
| > Does anyone still object to clone3() after reading the whole thread?
| 
| I object to what clone3() is.  The name is not particularly interesting.
| 
| The sanity checks for assigning pids are missing and there is a todo
| about it.  I am not comfortable with assigning pids to a new process
| in a pid namespace with other processes user space processes executing
| in it.

Could you clarify ? How is the call to alloc_pidmap() from clone3() different from the call from clone() itself ?

| 
| How we handle a clone extension depends critically on if we want to
| create a processes for restart in user space or kernel space.
| 
| Could some one give me or point me at a strong case for creating the
| processes for restart in user space?

There has been a lot of discussion on this with reference to the Checkpoint/Restart patchset. See http://lkml.org/lkml/2009/4/13/401
for instance.

| 
| The pid assignment code is currently ugly.  I asked that we just pass
| in the min max pid pids that already exist into the core pid
| assignment function and a constrained min/max that only admits a
| single pid when we are allocating a struct pid for restart.  That was
| not done and now we have a weird abortion with unnecessary special cases.

I did post a version of the patch attemptint to implement that. As pointed out in:

	http://lkml.org/lkml/2009/8/17/445

we would need more checks in alloc_pidmap() to cover cases like min or max being invalid or min being greater than max or max being greater than pid_max etc. Those checks also made the code ugly (imo).

Sukadev

I think it is totally inappropriate to assign pids in a pid namespace where there are user space processes already running.

Just read it. Thank you. Now I am certain clone_with_pids() is not useful functionality to be exporting to userspace.

The only real argument in favor of doing this in user space is greater flexibility. I can see checkpointing/restoring a single thread process without a pid namespace. Anything more and you are just asking for trouble.

A design that weakens security. Increases maintenance costs. All for an unreliable result seems like a bad one to me.

If you need more checks you are doing it wrong. The code already has min and max values, and even a start value. I was just strongly suggesting we generalize where we get the values from, and then we have not special cases.

Eric

Yyyyup.

How does it weaken security? It requires CAP_SYS_ADMIN, and, when targeted capabilities are added, it will require CAP_SYS_ADMIN to only those pid namespaces in which we choose a pid.

Does it really? It re-uses most of the existing code. More so than the only existing in-kernel restart code I've seen does.

I've personally always been on the fence as to whether we rebuild the process tree in user-space or kernel. And I'm still on the fence, as nothing you've said has convinced me otherwise.

(I'm not sure whether this argument is a separate one - regarding the implementation of choosing the pid - from the kernel-vs-userspace one or not, so will wait for a response to my other email about your API)

thanks,
-serge

Yyyyup.

How does it weaken security? It requires CAP_SYS_ADMIN, and, when targeted capabilities are added, it will require CAP_SYS_ADMIN to only those pid namespaces in which we choose a pid.

Does it really? It re-uses most of the existing code. More so than the only existing in-kernel restart code I've seen does.

I've personally always been on the fence as to whether we rebuild the process tree in user-space or kernel. And I'm still on the fence, as nothing you've said has convinced me otherwise.

(I'm not sure whether this argument is a separate one - regarding the implementation of choosing the pid - from the kernel-vs-userspace one or not, so will wait for a response to my other email about your API)

thanks,
-serge

Eric W. Biederman [ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]] wrote:

| > Could you clarify ? How is the call to alloc_pidmap() from clone3() different
| > from the call from clone() itself ?
| 
| I think it is totally inappropriate to assign pids in a pid namespace
| where there are user space processes already running.

Honestly, I don't understand why it is inappropriate or how this differs from normal clone() - which also assigns pids in own and ancestor pid namespaces.

| 
| > | How we handle a clone extension depends critically on if we want to
| > | create a processes for restart in user space or kernel space.
| > | 
| > | Could some one give me or point me at a strong case for creating the
| > | processes for restart in user space?
| >
| > There has been a lot of discussion on this with reference to the
| > Checkpoint/Restart patchset. See http://lkml.org/lkml/2009/4/13/401
| > for instance.
| 
| Just read it.  Thank you.

Sorry. I should have mentioned the reason here. (Like you mention below), flexibility is the main reason.

| Now I am certain clone_with_pids() is not useful functionality to be
| exporting to userspace.
| 
| The only real argument in favor of doing this in user space is greater
| flexibility.  I can see checkpointing/restoring a single thread process
| without a pid namespace.  Anything more and you are just asking for
| trouble.
| 
| A design that weakens security.  Increases maintenance costs.  All for
| an unreliable result seems like a bad one to me.
| 
| > | The pid assignment code is currently ugly.  I asked that we just pass
| > | in the min max pid pids that already exist into the core pid
| > | assignment function and a constrained min/max that only admits a
| > | single pid when we are allocating a struct pid for restart.  That was
| > | not done and now we have a weird abortion with unnecessary special cases.
| >
| > I did post a version of the patch attemptint to implement that. As
| > pointed out in:
| >
| > 	http://lkml.org/lkml/2009/8/17/445
| >
| > we would need more checks in alloc_pidmap() to cover cases like min or max
| > being invalid or min being greater than max or max being greater than pid_max
| > etc. Those checks also made the code ugly (imo).
| 
| If you need more checks you are doing it wrong.  The code already has min
| and max values, and even a start value.  I was just strongly suggesting
| we generalize where we get the values from, and then we have not special
| cases. 

Well, if alloc_pidmap(pid_ns, min, max) does not have to check the parameters passed in (ie assumes that callers pass it in correctly) it might be simple. But when user specifies the pid, the

	min == max == user's target pid

so we will need to check the values either here or in callers.

Yes the code already has values and a start value. But these are controlled by alloc_pidmap() and not passed in from the user space.

alloc_pidmap() needs to assign the next available pid or a specific target pid. Generalizing it to alloc a pid in a range seemed be a bit of an over kill for currently known usages.

I will post a version of the patch outside this patchset with min and max parameters and we can see if it can be optimized/beautified.

Sukadev

Eric W. Biederman [[email protected]] wrote:

| > Could you clarify ? How is the call to alloc_pidmap() from clone3() different
| > from the call from clone() itself ?
| 
| I think it is totally inappropriate to assign pids in a pid namespace
| where there are user space processes already running.

Honestly, I don't understand why it is inappropriate or how this differs from normal clone() - which also assigns pids in own and ancestor pid namespaces.

| 
| > | How we handle a clone extension depends critically on if we want to
| > | create a processes for restart in user space or kernel space.
| > | 
| > | Could some one give me or point me at a strong case for creating the
| > | processes for restart in user space?
| >
| > There has been a lot of discussion on this with reference to the
| > Checkpoint/Restart patchset. See http://lkml.org/lkml/2009/4/13/401
| > for instance.
| 
| Just read it.  Thank you.

Sorry. I should have mentioned the reason here. (Like you mention below), flexibility is the main reason.

| Now I am certain clone_with_pids() is not useful functionality to be
| exporting to userspace.
| 
| The only real argument in favor of doing this in user space is greater
| flexibility.  I can see checkpointing/restoring a single thread process
| without a pid namespace.  Anything more and you are just asking for
| trouble.
| 
| A design that weakens security.  Increases maintenance costs.  All for
| an unreliable result seems like a bad one to me.
| 
| > | The pid assignment code is currently ugly.  I asked that we just pass
| > | in the min max pid pids that already exist into the core pid
| > | assignment function and a constrained min/max that only admits a
| > | single pid when we are allocating a struct pid for restart.  That was
| > | not done and now we have a weird abortion with unnecessary special cases.
| >
| > I did post a version of the patch attemptint to implement that. As
| > pointed out in:
| >
| > 	http://lkml.org/lkml/2009/8/17/445
| >
| > we would need more checks in alloc_pidmap() to cover cases like min or max
| > being invalid or min being greater than max or max being greater than pid_max
| > etc. Those checks also made the code ugly (imo).
| 
| If you need more checks you are doing it wrong.  The code already has min
| and max values, and even a start value.  I was just strongly suggesting
| we generalize where we get the values from, and then we have not special
| cases. 

Well, if alloc_pidmap(pid_ns, min, max) does not have to check the parameters passed in (ie assumes that callers pass it in correctly) it might be simple. But when user specifies the pid, the

	min == max == user's target pid

so we will need to check the values either here or in callers.

Yes the code already has values and a start value. But these are controlled by alloc_pidmap() and not passed in from the user space.

alloc_pidmap() needs to assign the next available pid or a specific target pid. Generalizing it to alloc a pid in a range seemed be a bit of an over kill for currently known usages.

I will post a version of the patch outside this patchset with min and max parameters and we can see if it can be optimized/beautified.

Sukadev

The fact we can specify which pids we want. I won't claim it is as exploitable as NULL pointer deferences have been but it has that kind of feel to it.

Agreed. When you are talking about the target pid. That code path needs the extra check.

I was only thinking passed in from someplace else in kernel/pid.c

alloc_pidmap in assigning the next available pid is allocating a pid in a range.

Thanks,
Eric

This security concern was first brought up by Linus, and to address it we made clone3() require that the user be privileged to select pids.

But, honestly, a clone3() that allows the caller to request a specific pid is like a restart() syscall that allows the caller to restore a process with its original pid: you would simply checkpoint, and then alter the pid in the checkpoint image and restart, repeat ad infinitum.

a user to have an arbitrary process with a selected pid. So it doesn't really matter - for this security concern - if you select the pid in the kernel through restart() or from userspace through clone3().

Sure, you can also "choose" a pid today, by repeatedly forking until you get what you want... but that is harder to exploit. In contrast, both clone3() and restart() allow pid selection, instantaneously. Which is why both require privileges if the caller wants to select/ restore pids.

Oren.

This security concern was first brought up by Linus, and to address it we made clone3() require that the user be privileged to select pids.

But, honestly, a clone3() that allows the caller to request a specific pid is like a restart() syscall that allows the caller to restore a process with its original pid: you would simply checkpoint, and then alter the pid in the checkpoint image and restart, repeat ad infinitum.

From your security prism, they are equivalent: they practically allow a user to have an arbitrary process with a selected pid. So it doesn't really matter - for this security concern - if you select the pid in the kernel through restart() or from userspace through clone3().

Sure, you can also "choose" a pid today, by repeatedly forking until you get what you want... but that is harder to exploit. In contrast, both clone3() and restart() allow pid selection, instantaneously. Which is why both require privileges if the caller wants to select/ restore pids.

Oren.

Eric W. Biederman [ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]] wrote:

| > I will post a version of the patch outside this patchset with min
| > and max parameters and we can see if it can be optimized/beautified.
| 
| Thanks,
| Eric

Here is the patch. alloc_pid() calls alloc_pidmap() as follows:

- nr = alloc_pidmap(tmp);
+ min = max = 0;
+ if (target_pids) {
+ min = target_pids[i];
+ max = min + 1;
+ }
+ nr = alloc_pidmap(tmp, min, max);

It does look like this patch executes a bit more code even in the common case but let me know if you think this is better.

---

From: Sukadev Bhattiprolu <suka@suka.(none)>
Date: Tue, 20 Oct 2009 17:01:18 -0700
Subject: [PATCH] Add min and max parameters to alloc_pidmap()

With support for setting a specific pid number for a process, alloc_pidmap() will need a 'min' and a 'max' parameter.

---
 kernel/pid.c |   47 ++++++++++++++++++++++++++++++++++-------------
 1 files changed, 34 insertions(+), 13 deletions(-)

index c4d9914..56e13c1 100644
@@ -147,18 +147,30 @@ static int alloc_pidmap_page(struct pidmap *map)
 	return 0;
 }
 
-static int alloc_pidmap(struct pid_namespace *pid_ns)
+static int alloc_pidmap(struct pid_namespace *pid_ns, int min, int max)
 {
 	int i, offset, max_scan, pid, last = pid_ns->last_pid;
 	int rc = -EAGAIN;
 	struct pidmap *map;
 
-	pid = last + 1;
-	if (pid >= pid_max)
-		pid = RESERVED_PIDS;
+	if (min < 0 || max < 0 || max > pid_max)
+		return -EINVAL;
+
+	if (!max)
+		max = pid_max;
+
+	pid = min;
+	if (pid && pid >= max)
+		return -EINVAL;
+	else if (!pid) {
+		pid = last + 1;
+		if (pid >= pid_max)
+			pid = RESERVED_PIDS;
+	}
+
 	offset = pid & BITS_PER_PAGE_MASK;
 	map = &pid_ns->pidmap[pid/BITS_PER_PAGE];
-	max_scan = (pid_max + BITS_PER_PAGE - 1)/BITS_PER_PAGE - !offset;
+	max_scan = (max + BITS_PER_PAGE - 1)/BITS_PER_PAGE - !offset;
 	for (i = 0; i <= max_scan; ++i) {
 		rc = alloc_pidmap_page(map);
 		if (rc)

@@ -168,7 +180,12 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
 			do {
 				if (!test_and_set_bit(offset, map->page)) {
 					atomic_dec(&map->nr_free);
-					pid_ns->last_pid = pid;
+					/*
+					 * 'last_pid' only makes sense when
+					 * choosing from entire range
+					 */
+					if (!min)
+						pid_ns->last_pid = pid;
 					return pid;
 				}
 				offset = find_next_offset(map, offset);

@@ -179,22 +196,26 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
 			 * bitmap block and the final block was the same
 			 * as the starting point, pid is before last_pid.
 			 */
-			} while (offset < BITS_PER_PAGE && pid < pid_max &&
+			} while (offset < BITS_PER_PAGE && pid < max &&
 					(i != max_scan || pid < last ||
 					    !((last+1) & BITS_PER_PAGE_MASK)));
 		}
-		if (map < &pid_ns->pidmap[(pid_max-1)/BITS_PER_PAGE]) {
+		if (map < &pid_ns->pidmap[(max-1)/BITS_PER_PAGE]) {
 			++map;
 			offset = 0;
 		} else {
 			map = &pid_ns->pidmap[0];
 			offset = RESERVED_PIDS;
-			if (unlikely(last == offset)) {
-				rc = -EAGAIN;
-				break;
-			}
 		}
 		pid = mk_pid(pid_ns, map, offset);
+		/*
+		 * If we are back to where we started, well, no pids are
+		 * currently available in selected range.
+		 */
+		if (pid < min || unlikely(last == offset)) {
+			rc = -EAGAIN;
+			break;
+		}
 	}
 	return rc;
 }