Factor out code to allocate pidmap page

From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:40 -0700
Subject: [RFC][v3][PATCH 2/7] 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.

Signed-off-by Sukadev Bhattiprolu <[email protected]>

Acked-by Serge Hallyn <serue-r/[email protected]>

Reviewed-by Oren Laadan <[email protected]>

---

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

kernel/fork.c +3 -2

index b9e2edd..f8411a8 100644
@@ -1119,10 +1119,11 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		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);

kernel/pid.c +6 -3

index 9ff33cc..b2d6a19 100644
@@ -158,6 +158,7 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
 	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;
+	rc = -EAGAIN;
 	for (i = 0; i <= max_scan; ++i) {
 		rc = alloc_pidmap_page(map);
 		if (rc)

@@ -188,12 +189,14 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
 		} 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)

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

Signature · 6 lines

1.5.2.5

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:41 -0700
Subject: [RFC][v3][PATCH 3/7] Add target_pid parameter to alloc_pidmap()

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

Changelog[v2]:
- (Serge Hallyn) Check for 'pid < 0' in set_pidmap().(Code actually checks for 'pid <= 0' for completeness).

Signed-off-by Sukadev Bhattiprolu <[email protected]>

Acked-by Serge Hallyn <serue-r/[email protected]>

Reviewed-by Oren Laadan <[email protected]>

From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:42 -0700
Subject: [RFC][v3][PATCH 4/7] Add target_pids parameter to alloc_pid()

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

kernel/pid.c +2 -1

index b44dd21..090b221 100644
@@ -280,13 +280,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)

From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:43 -0700
Subject: [RFC][v3][PATCH 5/7] Add target_pids parameter to copy_process()

The new parameter will be used in a follow-on patch when clone_with_pids() is implemented.

Signed-off-by Sukadev Bhattiprolu <[email protected]>

Acked-by Serge Hallyn <serue-r/[email protected]>

Reviewed-by Oren Laadan <[email protected]>

From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:44 -0700
Subject: [RFC][v3][PATCH 6/7] 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[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)

Signed-off-by Sukadev Bhattiprolu <[email protected]>

Acked-by Serge Hallyn <serue-r/[email protected]>

Reviewed-by Oren Laadan <[email protected]>

---

include/linux/sched.h |    3 +++
include/linux/types.h |    5 +++++
kernel/fork.c         |   16 ++++++++++++++--
3 files changed, 22 insertions(+), 2 deletions(-)

include/linux/sched.h +3 -0

index b4c38bc..e3bdb86 100644
@@ -1995,6 +1995,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 *,
+				struct target_pid_set __user *pid_set);
 struct task_struct *fork_idle(int);
 
 extern void set_task_comm(struct task_struct *tsk, char *from);

include/linux/types.h +0 -0

index 5abe354..17ec186 100644

| >diff --git a/include/linux/types.h b/include/linux/types.h
| >index 5abe354..17ec186 100644
| >--- a/include/linux/types.h
| >+++ b/include/linux/types.h
| >@@ -204,6 +204,11 @@ struct ustat {
| > 	char			f_fpack[6];
| > };
| > 
| >+struct target_pid_set {
| >+	int num_pids;
| >+	pid_t *target_pids;
| >+};
| >+
| 
| A quick question: why not putting this definition into
| include/linux/pid.h?

Well, the data structure needs to be visible to applications so I chose types.h since its already listed in fork(2) man page and the related type, 'pid_t' is defined there.

Isn't pid.h is a kernel-only file ?

Sukadev

Quoted 17 lines · Sukadev Bhattiprolu

| >diff --git a/include/linux/types.h b/include/linux/types.h
| >index 5abe354..17ec186 100644
| >--- a/include/linux/types.h
| >+++ b/include/linux/types.h
| >@@ -204,6 +204,11 @@ struct ustat {
| > 	char			f_fpack[6];
| > };
| > 
| >+struct target_pid_set {
| >+	int num_pids;
| >+	pid_t *target_pids;
| >+};
| >+
| 
| A quick question: why not putting this definition into
| include/linux/pid.h?

Quoted 3 lines

Well, the data structure needs to be visible to applications so I chose types.h since its already listed in fork(2) man page and the related type, 'pid_t' is defined there.

I don't recall linux/types.h mentioned in the manpage.

And, heh, it ended up within #ifdef __KERNEL__ there... In fact, the types defined in that header are for kernel use.

Serge,

I have removed restriction on CLONE_NEWPID and may need a new ack.

Sukadev
---
From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:45 -0700
Subject: [RFC][v3][PATCH 7/7] Define clone_with_pids 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.

clone_with_pids(), intended for use during restart, is the same as clone(), except that it takes a 'target_pid_set' 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).

Unlike clone(), clone_with_pids() needs CAP_SYS_ADMIN, at least for now, to prevent unprivileged processes from misusing this interface.

Call clone_with_pids as follows:

	pid_t pids[] = { 0, 77, 99 };
	struct target_pid_set pid_set;

	pid_set.num_pids = sizeof(pids) / sizeof(int);
	pid_set.target_pids = &pids;

	syscall(__NR_clone_with_pids, flags, stack, NULL, NULL, NULL, &pid_set);

If a target-pid is 0, the kernel continues to assign a pid for the process in that namespace. In the above example, pids[0] is 0, meaning the kernel will assign next available pid to the process in init_pid_ns. But kernel will assign pid 77 in the child pid namespace 1 and pid 99 in pid namespace 2. If either 77 or 99 are taken, the system call fails with -EBUSY.

If 'pid_set.num_pids' exceeds the current nesting level of pid namespaces, the system call fails with -EINVAL.

Its mostly an exploratory patch seeking feedback on the interface.

NOTE:
Compared to clone(), clone_with_pids() needs to pass in two more pieces of information:

- number of pids in the set
- user buffer containing the list of pids.

But since clone() already takes 5 parameters, use a 'struct target_pid_set'.

TODO:
- Gently tested.
- May need additional sanity checks in do_fork_with_pids().

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)

Signed-off-by Sukadev Bhattiprolu <[email protected]>

---
 arch/x86/include/asm/syscalls.h    |    1 +
 arch/x86/include/asm/unistd_32.h   |    1 +
 arch/x86/kernel/entry_32.S         |    1 +
 arch/x86/kernel/process_32.c       |   21 +++++++
 arch/x86/kernel/syscall_table_32.S |    1 +
 kernel/fork.c                      |  108 +++++++++++++++++++++++++++++++++++-
 6 files changed, 132 insertions(+), 1 deletions(-)

arch/x86/include/asm/syscalls.h +1 -0

index 7043408..1fdc149 100644
@@ -31,6 +31,7 @@ asmlinkage int sys_get_thread_area(struct user_desc __user *);
 /* kernel/process_32.c */
 int sys_fork(struct pt_regs *);
 int sys_clone(struct pt_regs *);
+int sys_clone_with_pids(struct pt_regs *);
 int sys_vfork(struct pt_regs *);
 int sys_execve(struct pt_regs *);
 

arch/x86/include/asm/unistd_32.h +1 -0

index 6e72d74..90f906f 100644
@@ -340,6 +340,7 @@
 #define __NR_inotify_init1	332
 #define __NR_preadv		333
 #define __NR_pwritev		334
+#define __NR_clone_with_pids	335
 
 #ifdef __KERNEL__
 

arch/x86/kernel/entry_32.S +1 -0

index c929add..ee92b0d 100644
@@ -707,6 +707,7 @@ ptregs_##name: \
 PTREGSCALL(iopl)
 PTREGSCALL(fork)
 PTREGSCALL(clone)
+PTREGSCALL(clone_with_pids)
 PTREGSCALL(vfork)
 PTREGSCALL(execve)
 PTREGSCALL(sigaltstack)

arch/x86/kernel/process_32.c +21 -0

index 76f8f84..1efc3de 100644
@@ -445,6 +445,27 @@ int sys_clone(struct pt_regs *regs)
 	return do_fork(clone_flags, newsp, regs, 0, parent_tidptr, child_tidptr);
 }
 
+int sys_clone_with_pids(struct pt_regs *regs)
+{
+	unsigned long clone_flags;
+	unsigned long newsp;
+	int __user *parent_tidptr;
+	int __user *child_tidptr;
+	void __user *upid_setp;
+
+	clone_flags = regs->bx;
+	newsp = regs->cx;
+	parent_tidptr = (int __user *)regs->dx;
+	child_tidptr = (int __user *)regs->di;
+	upid_setp = (void __user *)regs->bp;
+
+	if (!newsp)
+		newsp = regs->sp;
+
+	return do_fork_with_pids(clone_flags, newsp, regs, 0, parent_tidptr,
+			child_tidptr, upid_setp);
+}
+
 /*
  * sys_execve() executes a new program.
  */

arch/x86/kernel/syscall_table_32.S +1 -0

index ff5c873..94c1a58 100644
@@ -334,3 +334,4 @@ ENTRY(sys_call_table)
 	.long sys_inotify_init1
 	.long sys_preadv
 	.long sys_pwritev
+	.long ptregs_clone_with_pids	/* 335 */

kernel/fork.c +41 -0

index a16ef7b..7738aed 100644
@@ -1335,6 +1335,97 @@ struct task_struct * __cpuinit fork_idle(int cpu)
 }
 
 /*
+ * If user specified any 'target-pids' in @upid_setp, copy them from
+ * user and return a pointer to a local copy of the list of pids. The
+ * caller must free the list, when they are done using it.
+ *
+ * If user did not specify any target pids, return NULL (caller should
+ * treat this like normal clone).
+ *
+ * On any errors, return the error code
+ */
+static pid_t *copy_target_pids(void __user *upid_setp)
+{
+	int j;
+	int rc;
+	int size;
+	int unum_pids;		/* # of pids specified by user */
+	int knum_pids;		/* # of pids needed in kernel */
+	pid_t *target_pids;
+	struct target_pid_set pid_set;
+
+	if (!upid_setp)
+		return NULL;
+
+	rc = copy_from_user(&pid_set, upid_setp, sizeof(pid_set));
+	if (rc)
+		return ERR_PTR(-EFAULT);
+
+	unum_pids = pid_set.num_pids;
+	knum_pids = task_pid(current)->level + 1;
+
+	if (!unum_pids)
+		return NULL;
+
+	if (unum_pids < 0 || unum_pids > knum_pids)
+		return ERR_PTR(-EINVAL);
+
+	/*
+	 * To keep alloc_pid() simple, allocate an extra pid_t in target_pids[]
+	 * and set it to 0. This last entry in target_pids[] corresponds to the
+	 * (yet-to-be-created) descendant pid-namespace if CLONE_NEWPID was
+	 * specified. If CLONE_NEWPID was not specified, this last entry will
+	 * simply be ignored.

kernel/fork.c +56 -1

+	 *
+	 * If this checkpointed process is however restarted in a level 3 pid
+	 * namespace, the restarted process has an extra ancestor pid namespace
+	 * (i.e 'unknown namespace') and 'knum_pids' exceeds 'unum_pids'.
+	 *
+	 * During restart, the process requests specific pids for its 'known
+	 * namespaces' and lets kernel assign pids to its 'unknown namespaces'.
+	 *
+	 * Since the requested-pids correspond to 'known namespaces' and since
+	 * 'known-namespaces' are younger than (i.e descendants of) 'unknown-
+	 * namespaces', copy requested pids to the back-end of target_pids[]
+	 * (i.e before the last entry for CLONE_NEWPID mentioned above).
+	 * Any entries in target_pids[] not corresponding to a requested pid
+	 * will be set to zero and kernel assigns a pid in those namespaces.
+	 *
+	 * NOTE: The order of pids in target_pids[] is oldest pid namespace to
+	 * 	 youngest (target_pids[0] corresponds to init_pid_ns). i.e.
+	 * 	 the order is:
+	 *
+	 * 		- pids for 'unknown-namespaces' (if any)
+	 * 		- pids for 'known-namespaces' (requested pids)
+	 * 		- 0 in the last entry (for CLONE_NEWPID).
+	 */
+	j = knum_pids - unum_pids;
+	size = unum_pids * sizeof(pid_t);
+
+	rc = copy_from_user(&target_pids[j], pid_set.target_pids, size);
+	if (rc) {
+		rc = -EFAULT;
+		goto out_free;
+	}
+
+	return target_pids;
+
+out_free:
+	kfree(target_pids);
+	return ERR_PTR(rc);
+}
+
+/*
  *  Ok, this is the main fork-routine.
  *
  * It copies the process, and if successful kick-starts

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

@@ -1385,6 +1476,17 @@ long do_fork_with_pids(unsigned long clone_flags,
 		}
 	}
 
+	target_pids = copy_target_pids(pid_setp);
+
+	if (target_pids) {
+		if (IS_ERR(target_pids))
+			return PTR_ERR(target_pids);
+
+		nr = -EPERM;
+		if (!capable(CAP_SYS_ADMIN))
+			goto out_free;
+	}
+
 	/*
 	 * When called from kernel_thread, don't do user tracing stuff.
 	 */

@@ -1446,6 +1548,10 @@ long do_fork_with_pids(unsigned long clone_flags,
 	} else {
 		nr = PTR_ERR(p);
 	}
+
+out_free:
+	kfree(target_pids);
+
 	return nr;
 }
 

Quoting Sukadev Bhattiprolu ([email protected]):

Bear with me as I try to get this straight...

Let's say we have the following task being checkpointed (btw, the actual checkpoint code hopefully will, as Oren mentioned, not be always checkpointing all of a task's pids, but only the pids from the checkpointer's pidns level an up - but that doesn't matter to you):

Either:

level no | pid
0 1001
1 50
2 3

or

level no | pid
0 1001
1 50
2 1

Now we want to restart that inside a container. So for the first task we have a parent task:

level no       |     pid
0                    5009
1                    1000

2 49
3 2

calling clone_with_pid with upids {1001,50,3} to produce task:

level no | pid
0 5010
1 1001
2 50
3 3

So the numbers in your code for this case become:

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1364

Quoted 1 line

kernel/fork.c +1 -0

+	unum_pids = pid_set.num_pids;

unum_pids = 3

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1365

Quoted 1 line

kernel/fork.c +1 -0

+	knum_pids = task_pid(current)->level + 1;

        knum_pids = 3 + 1 = 4

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1380

Quoted 1 line

kernel/fork.c +1 -0

+	target_pids = kzalloc((knum_pids + 1) * sizeof(pid_t), GFP_KERNEL);

XXX First problem:
target_pids gets room for 5 pids, one more than we need.

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1412

Quoted 2 lines

kernel/fork.c +1 -0

+	j = knum_pids - unum_pids;

j = 1, so we copy the 3 pids in the right place.

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1421

Quoted 7 lines

kernel/fork.c +7 -0

+	rc = copy_from_user(&target_pids[j], pid_set.target_pids, size);
+	if (rc) {
+		rc = -EFAULT;
+		goto out_free;
+	}
+
+	return target_pids;

For the second one, we have a parent task

level no | pid
0 5009
1 1000
2 49

calling clone_with_pid with CLONE_NEWPID and {1001,50,1} to produce:

level no | pid
0 5010
1 1001
2 50
3 1

So the numbers in your code become:

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1364

Quoted 1 line

kernel/fork.c +1 -0

+	unum_pids = pid_set.num_pids;

unum_pids = 3

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1365

Quoted 1 line

kernel/fork.c +1 -0

+	knum_pids = task_pid(current)->level + 1;

        knum_pids = 2 + 1 = 3

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1380

Quoted 1 line

kernel/fork.c +1 -0

+	target_pids = kzalloc((knum_pids + 1) * sizeof(pid_t), GFP_KERNEL);

target_pids gets room for 4 pids

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1412

Quoted 1 line

kernel/fork.c +1 -0

+	j = knum_pids - unum_pids;

XXX Second problem:
j = 0, which is not right, should be 1?

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1421

Quoted 7 lines

kernel/fork.c +7 -0

+	rc = copy_from_user(&target_pids[j], pid_set.target_pids, size);
+	if (rc) {
+		rc = -EFAULT;
+		goto out_free;
+	}
+
+	return target_pids;

So it looks like your usage of knum_pids isn't quite right.

(right?)

Or are you expecting userspace to not specify the 1, only 2 upids? In that case, you're requiring different behavior for pid 1 than any others, which is kind of weird? Or is that just me?

The two issues are related, and this is intentional. The idea was that when you use CLONE_NEWPID, you imply a new nesting level and a pid==1 there. So you are not supposed to specify the pid of that new level.

IOW, the parent specify the pid's of the child from the _parent's_ level and up (of the desired depth). CLONE_NEWPID creates a new pidns level below the parent's, and that is not covered in the array of pids.

By allocation an extra slot and forcing it to be 0, we ensure that the case of CLONE_NEWPID is covered correctly. Clearly, if this flag isn't set, then the extra slot is redundant (but doesn't hurt).

Quoted 27 lines

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1412

Quoted 1 line

kernel/fork.c +1 -0

+	j = knum_pids - unum_pids;

j = 1, so we copy the 3 pids in the right place.

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1421

Quoted 7 lines

kernel/fork.c +7 -0

+	rc = copy_from_user(&target_pids[j], pid_set.target_pids, size);
+	if (rc) {
+		rc = -EFAULT;
+		goto out_free;
+	}
+
+	return target_pids;

For the second one, we have a parent task

level no | pid
0 5009
1 1000
2 49

calling clone_with_pid with CLONE_NEWPID and {1001,50,1} to produce:

level no | pid
0 5010
1 1001
2 50
3 1

Quoted 4 lines

So the numbers in your code become:

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1364

Quoted 1 line

kernel/fork.c +1 -0

+	unum_pids = pid_set.num_pids;

unum_pids = 3

This is a "bug" of the parent. The parent should specify the pids from the parent's level only and up, and not include the new level below that will be created. (After all, it will have to be 1).

So unum_pids = 3 will not do what you want; instead it will try to create the process:

0 1001
1 50
2 1
3 1

And will fail, of course, because pid==1 at level 2 is already taken.

Instead, parent should say use {1001, 50}.

Quoted 9 lines

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1365

Quoted 2 lines

kernel/fork.c +1 -0

+	knum_pids = task_pid(current)->level + 1;

        knum_pids = 2 + 1 = 3

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1380

Quoted 1 line

kernel/fork.c +1 -0

+	target_pids = kzalloc((knum_pids + 1) * sizeof(pid_t), GFP_KERNEL);

target_pids gets room for 4 pids

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1412

Quoted 1 line

kernel/fork.c +1 -0

+	j = knum_pids - unum_pids;

Quoted 5 lines · Oren Laadan

The two issues are related, and this is intentional. The idea was that when you use CLONE_NEWPID, you imply a new nesting level and a pid==1 there. So you are not supposed to specify the pid of that new level.

Quoted 8 lines

IOW, the parent specify the pid's of the child from the _parent's_ level and up (of the desired depth). CLONE_NEWPID creates a new pidns level below the parent's, and that is not covered in the array of pids.

By allocation an extra slot and forcing it to be 0, we ensure that the case of CLONE_NEWPID is covered correctly. Clearly, if this flag isn't set, then the extra slot is redundant (but doesn't hurt).

Ok, I see - I guess I don't mind those semantics. So:

Quoted 42 lines

Quoted 26 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	j = knum_pids - unum_pids;

j = 1, so we copy the 3 pids in the right place.

Review

Quoted 7 lines

(quoted diff) +7 -0

+	rc = copy_from_user(&target_pids[j], pid_set.target_pids, size);
+	if (rc) {
+		rc = -EFAULT;
+		goto out_free;
+	}
+
+	return target_pids;

For the second one, we have a parent task

level no | pid
0 5009
1 1000
2 49

calling clone_with_pid with CLONE_NEWPID and {1001,50,1} to produce:

level no | pid
0 5010
1 1001
2 50
3 1

Quoted 4 lines

So the numbers in your code become:

Review

Quoted 1 line

(quoted diff) +1 -0

+	unum_pids = pid_set.num_pids;

unum_pids = 3

This is a "bug" of the parent. The parent should specify the pids from the parent's level only and up, and not include the new level below that will be created. (After all, it will have to be 1).

So unum_pids = 3 will not do what you want; instead it will try to create the process:

0 1001
1 50
2 1
3 1

Quoted 4 lines

And will fail, of course, because pid==1 at level 2 is already taken.

Instead, parent should say use {1001, 50}.

Ok, but then we have the task:

level no | pid
0 5009
1 1000
2 49

calling clone(CLONE_NEWPID) with unum_pids = 2, so

Quoted 7 lines

Quoted 7 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	knum_pids = task_pid(current)->level + 1;

        knum_pids = 2 + 1 = 3

Review

Quoted 1 line

(quoted diff) +1 -0

+	target_pids = kzalloc((knum_pids + 1) * sizeof(pid_t), GFP_KERNEL);

target_pids gets room for 4 pids

Review

Quoted 1 line

(quoted diff) +1 -0

+	j = knum_pids - unum_pids;

j = 3 - 2 = 1, so we copy 1001 into pid[1] and
50 into pid[2], with 0 in pid[0] and pid[3]

Looks good. Thanks for indulging me :)

Acked-by Serge Hallyn <serue-r/[email protected]>

One last thought - should there be an explicit check to make sure that if CLONE_NEWPID, then at the end pid[knum_pids+1] = 0? Or is that there and I just missed it?

Quoted 3 lines

One last thought - should there be an explicit check to make sure that if CLONE_NEWPID, then at the end pid[knum_pids+1] = 0? Or is that there and I just missed it?

the wonders of kzalloc() ...

Quoted 4 lines · Oren Laadan

Quoted 3 lines

One last thought - should there be an explicit check to make sure that if CLONE_NEWPID, then at the end pid[knum_pids+1] = 0? Or is that there and I just missed it?

the wonders of kzalloc() ...

No. I'm saying that I don't see anything stopping the user from doing CLONE_NEWPID while specifying an extra pid, so that they end up trying to create a pidns init with vpid 5

Did I entirely misread your question ?

Quoted 43 lines · Serge E. Hallyn

Quoting Sukadev Bhattiprolu ([email protected]):

Bear with me as I try to get this straight...

Let's say we have the following task being checkpointed (btw, the actual checkpoint code hopefully will, as Oren mentioned, not be always checkpointing all of a task's pids, but only the pids from the checkpointer's pidns level an up - but that doesn't matter to you):

Either:

level no | pid
0 1001
1 50
2 3

or

level no | pid
0 1001
1 50
2 1

Now we want to restart that inside a container. So for the first task we have a parent task:

level no       |     pid
0                    5009
1                    1000

2 49
3 2

calling clone_with_pid with upids {1001,50,3} to produce task:

level no | pid
0 5010
1 1001
2 50
3 3

So the numbers in your code for this case become:

Quoted 8 lines

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1364

Quoted 1 line

kernel/fork.c +1 -0

+	unum_pids = pid_set.num_pids;

unum_pids = 3

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1365

Quoted 1 line

kernel/fork.c +1 -0

+	knum_pids = task_pid(current)->level + 1;

        knum_pids = 3 + 1 = 4

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1380

Quoted 1 line

kernel/fork.c +1 -0

+	target_pids = kzalloc((knum_pids + 1) * sizeof(pid_t), GFP_KERNEL);

XXX First problem:
target_pids gets room for 5 pids, one more than we need.

The two issues are related, and this is intentional. The idea was that when you use CLONE_NEWPID, you imply a new nesting level and a pid==1 there. So you are not supposed to specify the pid of that new level.

IOW, the parent specify the pid's of the child from the _parent's_ level and up (of the desired depth). CLONE_NEWPID creates a new pidns level below the parent's, and that is not covered in the array of pids.

By allocation an extra slot and forcing it to be 0, we ensure that the case of CLONE_NEWPID is covered correctly. Clearly, if this flag isn't set, then the extra slot is redundant (but doesn't hurt).

Quoted 26 lines

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1412

Quoted 1 line

kernel/fork.c +1 -0

+	j = knum_pids - unum_pids;

j = 1, so we copy the 3 pids in the right place.

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1421

Quoted 7 lines

kernel/fork.c +7 -0

+	rc = copy_from_user(&target_pids[j], pid_set.target_pids, size);
+	if (rc) {
+		rc = -EFAULT;
+		goto out_free;
+	}
+
+	return target_pids;

For the second one, we have a parent task

level no | pid
0 5009
1 1000
2 49

calling clone_with_pid with CLONE_NEWPID and {1001,50,1} to produce:

level no | pid
0 5010
1 1001
2 50
3 1

Quoted 4 lines

So the numbers in your code become:

kernel/fork.c struct task_struct * __cpuinit fork_idle(int cpu) · line 1364

Quoted 1 line

kernel/fork.c +1 -0

+	unum_pids = pid_set.num_pids;

unum_pids = 3

This is a "bug" of the parent. The parent should specify the pids from the parent's level only and up, and not include the new level below that will be created. (After all, it will have to be 1).

| Call clone_with_pids as follows:
| 
| 	pid_t pids[] = { 0, 77, 99 };
| 	struct target_pid_set pid_set;
| 
| 	pid_set.num_pids = sizeof(pids) / sizeof(int);
| 	pid_set.target_pids = &pids;
| 
| 	syscall(__NR_clone_with_pids, flags, stack, NULL, NULL, NULL, &pid_set);

Now that we need to do a copy_from_user() anyway, is it worth collapsing one or more parameters into a structure ?

	struct clone_with_pid_args {
		u64 flags;
		void *stack;
		void *tls;
		struct target_pid_set *pid_set;
	};

	struct clone_tid_info {
		int *parent_tidptr;
		int *child_tidptr;
	}

	struct clone_with_pid_args cwp_args;
	struct clone_tid_info ctid_inf;

	...

	syscall(__NR_clone_with_pids, &cwp_args, &ctid_info);

One advantage would be that it would make room for 64 bit clone flags, so we won't need a new flavor of clone_with_pids() when clone_flags is extended.

From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:39 -0700
Subject: [RFC][v3][PATCH 1/7] Factor out code to allocate pidmap page

To implement support for clone_with_pids() system call we would need to allocate pidmap page in more than one place. Move this code to a new function alloc_pidmap_page().

Changelog[v2]:
- (Matt Helsley, Dave Hansen) Have alloc_pidmap_page() return -ENOMEM on error instead of -1.

Signed-off-by Sukadev Bhattiprolu <[email protected]>

Acked-by Serge Hallyn <serue-r/[email protected]>

Reviewed-by Oren Laadan <[email protected]>