Factor out code to allocate pidmap page

Quoted 2 lines

From a1fdec1036a952359d02a7c667d126bd2fff6804 Mon Sep 17 00:00:00 2001

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

---
 kernel/pid.c |   28 ++++++++++++++++++++++++++--
 1 files changed, 26 insertions(+), 2 deletions(-)

kernel/pid.c +26 -2

index b2d6a19..b44dd21 100644
@@ -147,11 +147,35 @@ static int alloc_pidmap_page(struct pidmap *map)
 	return 0;
 }
 
-static int alloc_pidmap(struct pid_namespace *pid_ns)
+static int set_pidmap(struct pid_namespace *pid_ns, int pid)
+{
+	int offset;
+	struct pidmap *map;
+
+	if (pid <= 0 || pid >= pid_max)
+		return -EINVAL;
+
+	offset = pid & BITS_PER_PAGE_MASK;
+	map = &pid_ns->pidmap[pid/BITS_PER_PAGE];
+
+	if (alloc_pidmap_page(map))
+		return -ENOMEM;
+
+	if (test_and_set_bit(offset, map->page))
+		return -EBUSY;
+
+	atomic_dec(&map->nr_free);
+	return pid;
+}
+
+static int alloc_pidmap(struct pid_namespace *pid_ns, int target_pid)
 {
 	int i, rc, offset, max_scan, pid, last = pid_ns->last_pid;
 	struct pidmap *map;
 
+	if (target_pid)
+		return set_pidmap(pid_ns, target_pid);
+
 	pid = last + 1;
 	if (pid >= pid_max)
 		pid = RESERVED_PIDS;

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

Signature · 1 line

1.5.2.5

From a1fdec1036a952359d02a7c667d126bd2fff6804 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:41 -0700
Subject: [RFC][PATCH 3/7][v2] 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]>

---

 kernel/pid.c |   28 ++++++++++++++++++++++++++--
 1 files changed, 26 insertions(+), 2 deletions(-)

kernel/pid.c +26 -2

index b2d6a19..b44dd21 100644
@@ -147,11 +147,35 @@ static int alloc_pidmap_page(struct pidmap *map)
 	return 0;
 }
 
-static int alloc_pidmap(struct pid_namespace *pid_ns)
+static int set_pidmap(struct pid_namespace *pid_ns, int pid)
+{
+	int offset;
+	struct pidmap *map;
+
+	if (pid <= 0 || pid >= pid_max)
+		return -EINVAL;
+
+	offset = pid & BITS_PER_PAGE_MASK;
+	map = &pid_ns->pidmap[pid/BITS_PER_PAGE];
+
+	if (alloc_pidmap_page(map))
+		return -ENOMEM;
+
+	if (test_and_set_bit(offset, map->page))
+		return -EBUSY;
+
+	atomic_dec(&map->nr_free);
+	return pid;
+}
+
+static int alloc_pidmap(struct pid_namespace *pid_ns, int target_pid)
 {
 	int i, rc, offset, max_scan, pid, last = pid_ns->last_pid;
 	struct pidmap *map;
 
+	if (target_pid)
+		return set_pidmap(pid_ns, target_pid);
+
 	pid = last + 1;
 	if (pid >= pid_max)
 		pid = RESERVED_PIDS;

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

From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:44 -0700
Subject: [RFC][PATCH 6/7][v2] 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[v2]:
- [v1] of this patch had some architecture-indpendent code in arch/x86/kernel/process_32.c. To facilitate moving this code to kernel/fork.c, in the next patch, [v2] of the patch passes 'struct target_pid_set __user *' to do_fork_with_pids() instead of 'pid_t *'.

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

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

include/linux/sched.h +1 -0

index b4c38bc..8468e54 100644
@@ -1995,6 +1995,7 @@ 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 +5 -0

index 5abe354..17ec186 100644
@@ -204,6 +204,11 @@ struct ustat {
 	char			f_fpack[6];
 };
 
+struct target_pid_set {
+	int num_pids;
+	pid_t *target_pids;
+};
+
 #endif	/* __KERNEL__ */
 #endif /*  __ASSEMBLY__ */
 #endif /* _LINUX_TYPES_H */

kernel/fork.c +14 -2

index 373411e..a16ef7b 100644
@@ -1340,12 +1340,13 @@ struct task_struct * __cpuinit fork_idle(int cpu)
  * 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,
+	      struct target_pid_set __user *pid_setp)
 {
 	struct task_struct *p;
 	int trace = 0;

@@ -1448,6 +1449,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, NULL);
+}
+
 #ifndef ARCH_MIN_MMSTRUCT_ALIGN
 #define ARCH_MIN_MMSTRUCT_ALIGN 0
 #endif

Signature · 1 line

1.5.2.5

From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:44 -0700
Subject: [RFC][PATCH 6/7][v2] 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[v2]:
- [v1] of this patch had some architecture-indpendent code in arch/x86/kernel/process_32.c. To facilitate moving this code to kernel/fork.c, in the next patch, [v2] of the patch passes 'struct target_pid_set __user *' to do_fork_with_pids() instead of 'pid_t *'.

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

---

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

include/linux/sched.h +1 -0

index b4c38bc..8468e54 100644
@@ -1995,6 +1995,7 @@ 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 +5 -0

index 5abe354..17ec186 100644
@@ -204,6 +204,11 @@ struct ustat {
 	char			f_fpack[6];
 };
 
+struct target_pid_set {
+	int num_pids;
+	pid_t *target_pids;
+};
+
 #endif	/* __KERNEL__ */
 #endif /*  __ASSEMBLY__ */
 #endif /* _LINUX_TYPES_H */

kernel/fork.c +14 -2

index 373411e..a16ef7b 100644
@@ -1340,12 +1340,13 @@ struct task_struct * __cpuinit fork_idle(int cpu)
  * 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,
+	      struct target_pid_set __user *pid_setp)
 {
 	struct task_struct *p;
 	int trace = 0;

@@ -1448,6 +1449,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, NULL);
+}
+
 #ifndef ARCH_MIN_MMSTRUCT_ALIGN
 #define ARCH_MIN_MMSTRUCT_ALIGN 0
 #endif

From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:45 -0700
Subject: [RFC][PATCH 7/7][v2] Define clone_with_pids syscall

clone_with_pids() is same as clone(), except that it takes a 'target_pid_set' paramter which lets caller choose a specific pid number for the child process in each of the child process's pid namespace. This system call would be needed to implement Checkpoint/Restart (i.e after a checkpoint, restart a process with its original pids).

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.

From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:45 -0700
Subject: [RFC][PATCH 7/7][v2] Define clone_with_pids syscall

clone_with_pids() is same as clone(), except that it takes a 'target_pid_set' paramter which lets caller choose a specific pid number for the child process in each of the child process's pid namespace. This system call would be needed to implement Checkpoint/Restart (i.e after a checkpoint, restart a process with its original pids).

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:
1. clone_with_pids(), at least for now, needs CAP_SYS_ADMIN to prevent misuse of the interface.

2. 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().
- Allow CLONE_NEWPID() with clone_with_pids() (ensure target-pid in the namespace is either 1 or 0).

Changelog[v2]:
- (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]>

index a16ef7b..f265a18 100644

kernel/fork.c +22 -0

@@ -1335,6 +1335,58 @@ 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 the list of target pids.
+ *
+ * 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 rc;
+	int size;
+	int num_pids;
+	pid_t __user *utarget_pids;
+	pid_t *target_pids;
+	struct target_pid_set pid_set;
+
+	if(!upid_setp)
+		return NULL;
+
+	if (copy_from_user(&pid_set, upid_setp, sizeof(pid_set)))
+		return ERR_PTR(-EFAULT);

In general, can a task figure out it's depth in the pid-ns hierarchy ?

Serge E. Hallyn [serue-r/[email protected]] wrote:

| Quoting Oren Laadan ([email protected]):
| > In general, can a task figure out it's depth in the pid-ns hierarchy ?
| 
| In user-space?  No.

Not from any existing user interface. But hypothetically or in the long-term can/should sys_checkpoint() not be able to figure out if it has to C/R say a bash shell that has nested pid namespaces ?

I am guessing sys_checkpoint() can know if its crossing a pid-ns boundary by comparing or computing the max nesting level while walking a process tree. i.e if task_pid(task)->pid_ns->level is not the same for all process in the tree, then we have nested namespaces. How to checkpoint/restart such a tree is of course a bigger challenge.

But if sys_checkpoint() can find the max nesting level ? If that info is saved in checkpoint image, clone_with_pids() could use that info - no ?

Serge E. Hallyn [[email protected]] wrote:

| 
| If a container at pidns level 2, with 1 more pidns underneath it,
| is checkpointed, then the checkpoint image should only reflect
| the base and child pidns.  The two ancestor pidns levels should
| not be there.

Agree. I meant to say that we would need to specify more than 2 pids when checkpoint/restarting apps with deeply nested namespaces if any :-)

Specifying bottom-up may solve the problem I guess.

Oren Laadan [[email protected]] wrote:

| > +
| > +	if (num_pids < 0 || num_pids > task_pid(current)->level + 1)
| > +		return ERR_PTR(-EINVAL);
| 
| What happens if (num_pids < task_pid(current->level) + 1) ?
| Unless I missed something elsewhere, I suspect it will oops,

Yep. the kzalloc() below this check should use:

	task_pid(current)->pid_ns->level+1 * sizeof(pid_t)

| because in patch #4, you had:
| 
|  	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;
| 
| In general, can a task figure out it's depth in the pid-ns hierarchy ?
| 
| I'm thinking of a case in which a checkpoint was taken of a (flat)
| container which is in depth 2 of the global hierarchy, and then it
| is restarted in depth 3, or in depth 1.
| 
| Perhaps the semantics of the syscall should be that target_pids will
| indicate the desired pids _from the bottom up_. A value of "0" means

Hmm, I thought bottom up would be confusing, but I guess that would work better :-) Will fix and resend.

| "don't care" and let the kernel assign. The remaining levels (upwards)
| will be treated as zeros.
| 
| [...]
| 
| Oren.
| 

Here is an updated patch with hopefully some useful comments on why we copy to the end of target_pids[] (comments were harder to write than the code :-)

---
From: Sukadev Bhattiprolu <[email protected]>
Date: Mon, 4 May 2009 01:17:45 -0700
Subject: [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 descendant pid namespaces. Since the application does/can not know of any ancestor pid namespaces, it cannot choose a pid in those namespaces.

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().
- Allow CLONE_NEWPID() with clone_with_pids() (ensure target-pid in the namespace is either 1 or 0).

Changelog[v2]:
- (Oren Laadan) Specified target pids should apply to youngest pid-namespaces only (see comments in copy_target_pids())
- Remove unnecessary printk and add a note to callers of copy_target_pids() to free target_pids.
- (Matt Helsley) Update patch description.
- (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]>

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

---

kernel/fork.c +19 -0

index a16ef7b..06b1583 100644
@@ -1335,6 +1335,84 @@ 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 num_pids;
+	int nesting;
+	pid_t __user *utarget_pids;
+	pid_t *target_pids;
+	struct target_pid_set pid_set;

kernel/fork.c +24 -0

+
+	if (!num_pids)
+		return NULL;
+
+	if (num_pids < 0 || num_pids > nesting)
+		return ERR_PTR(-EINVAL);
+
+	target_pids = kzalloc((nesting * sizeof(pid_t)), GFP_KERNEL);
+	if (!target_pids)
+		return ERR_PTR(-ENOMEM);
+
+	/*
+	 * A process running in a level-1 pid namespace has two pid
+	 * namespaces and hence two pid numbers. If this process is
+	 * checkpointed, information about these two namespaces are
+	 * saved. We refer to these namespaces as 'known namespaces'.
+	 *
+	 * If this checkpointed process is however restarted in a 
+	 * level-2 pid namespace, the restarted process has an extra
+	 * ancestor pid namespace (i.e 'unknown namespace').
+	 *
+	 * During restart, the process requests specific pids for its
+	 * 'known namespaces' and lets kernel assign pids to its 'unknown
+	 * namespaces'. 

Oren Laadan [[email protected]] wrote:

| 
| 
| Sukadev Bhattiprolu wrote:
| > Here is an updated patch with hopefully some useful comments on why
| > we copy to the end of target_pids[] (comments were harder to write
| > than the code :-)
| > 
| > ---
| > From: Sukadev Bhattiprolu <[email protected]>
| > Date: Mon, 4 May 2009 01:17:45 -0700
| > Subject: [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 descendant pid namespaces. Since the application does/can not know of any
| > ancestor pid namespaces, it cannot choose a pid in those namespaces.

Grr, I got this backwards. The application chooses pid numbers in active and ancestor namespaces. It can/does NOT need a pid in descendant namespaces. I had it wrong in the code too and fixed it but missed the patch description.

| 
| I think the term "descendant pid namespaces" is confusing, because it
| can be interpreted as the collection of all descendant namespaces (e.g.
| by all children of a task), which is a tree. And I'm unsure what does
| "ancestor pid namespaces" mean.
| 
| I'd say that both "descendant" and "ancestor" here are defined with
| respect to the nesting level of root task of a restart. Or otherwise
| say explicitly that it's relative to the nesting level at which the
| restart operation occurs.
| 
| In my mind, clone_with_pid() is performed from "within" the deepest
| level - in which the child will "live" - and the pids array then
| works "bottom-up" in the sense that it indicates the desired pids
| as you go up the ancestry chain (going up is always well defined).
| 
| Right now the restart with a flat pid-ns works by first devising a
| schedule and then following that schedule with forks/clones to
| generate a process tree.
| 
| To also restore pids, we need only use clone_with_pid() with an array
| of size 1, and we're good.
| 
| To restart nested pid-ns from userspace, we need to devise a schedule
| that will command a sequence of fork/clones. The schedule will tell
| when to use the CLONE_NEWPID to create a sub-pid-ns. When that happens,
| we'll increment the size of the pids array - for that clone and all
| subsequent clones by that task and its descendents.
| 
| So the clone_with_pids occurs in the context of the deepest pid-ns, so
| to speak, and the arrays of pids works its way "upwards"
| 
| (That's probably what you meant, anyway).
| 
| > 
| > 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().
| > 	- Allow CLONE_NEWPID() with clone_with_pids() (ensure target-pid in
| > 	  the namespace is either 1 or 0).
| > 
| > Changelog[v2]:
| > 	- (Oren Laadan) Specified target pids should apply to youngest
| > 	  pid-namespaces only (see comments in copy_target_pids())
| > 	- Remove unnecessary printk and add a note to callers of
| > 	  copy_target_pids() to free target_pids.
| > 	- (Matt Helsley) Update patch description.
| > 	- (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]>
| > Acked-by: Serge Hallyn <serue-r/[email protected]>
| > ---
| 
| [...]
| 
| 
| > diff --git a/kernel/fork.c b/kernel/fork.c
| > index a16ef7b..06b1583 100644
| > --- a/kernel/fork.c
| > +++ b/kernel/fork.c
| > @@ -1335,6 +1335,84 @@ 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 num_pids;
| > +	int nesting;
| > +	pid_t __user *utarget_pids;
| > +	pid_t *target_pids;
| > +	struct target_pid_set pid_set;
| > +
| > +	if(!upid_setp)
| > +		return NULL;
| > +
| > +	if (copy_from_user(&pid_set, upid_setp, sizeof(pid_set)))
| > +		return ERR_PTR(-EFAULT);
| > +
| > +	num_pids = pid_set.num_pids;
| > +	utarget_pids = pid_set.target_pids;
| > +	nesting = task_pid(current)->level + 1;
| 
| I should have mentioned earlier, but there is also the case of
| CLONE_NEWPID. If CLONE_NEWPID is given, then @nesting should be
| plus one, _and_ the corresponding pid must be 1 or 0.

Right. do_fork_with_pids() checks if CLONE_NEWPID is specified with target_pids and returns -EINVAL for now.

| 
| And this consideration deserves fat comment :)
| 
| > +
| > +	if (!num_pids)
| > +		return NULL;
| > +
| > +	if (num_pids < 0 || num_pids > nesting)
| > +		return ERR_PTR(-EINVAL);
| > +
| > +	target_pids = kzalloc((nesting * sizeof(pid_t)), GFP_KERNEL);
| > +	if (!target_pids)
| > +		return ERR_PTR(-ENOMEM);
| > +
| > +	/*
| > +	 * A process running in a level-1 pid namespace has two pid
| > +	 * namespaces and hence two pid numbers. If this process is
| > +	 * checkpointed, information about these two namespaces are
| > +	 * saved. We refer to these namespaces as 'known namespaces'.
| > +	 *
| > +	 * If this checkpointed process is however restarted in a 
| > +	 * level-2 pid namespace, the restarted process has an extra
| > +	 * ancestor pid namespace (i.e 'unknown namespace').
| > +	 *
| > +	 * 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 end of target_pids[]
| > +	 * and copy zeroes to the beginning (so kernel can assign a pid for
| > +	 * the unknown namespaces).
| > +	 *
| > +	 * NOTE: The order of pids in target_pids[] is oldest pid namespace
| > +	 * 	 to youngest (i.e target_pids[0] corresponds to init_pid_ns). 
| > +	 */
| 
| Reads good!
| 
| [...]
| 
| Oren.

Sukadev Bhattiprolu [[email protected]] wrote:
| Oren Laadan [[email protected]] wrote:

I am not sure what the semantics should be for this case:

- checkpoint a process that is in level-3 pid namespace
- restart in a level-2 or level-1 pid namespace

clone_with_pids() will fail now since number of pids specified would be 4 but kernel expects only 2 or 3. mktree/restart program cannot figure out current nesting to trim the target-pids.

Should we remove the check of user-pids exceeding the current nesting level and simply ignore the pids from the older namespaces ?

Oren Laadan [[email protected]] wrote:

| 
| 
| Sukadev Bhattiprolu wrote:
| > Sukadev Bhattiprolu [[email protected]] wrote:
| > | Oren Laadan [[email protected]] wrote:
| > 
| > I am not sure what the semantics should be for this case:
| > 
| > 	- checkpoint a process that is in level-3 pid namespace
| > 	- restart in a level-2 or level-1 pid namespace
| > 
| 
| Meaning: a container root was at level-3, so tasks in the container
| were level-3 through level-(3+N), where N is the in-container depth
| so to speak. Then it was restarted such that the base became level-2
| or level-1.
| 
| I think we already covered this.
| 

Sorry, I meant level3 not 'level minus 3' :-) Restating, suppose init_pid_ns is L0, and L3 is a pid namespace 3 levels deep (i.e the process has 4 pids at checkpoint time). When restarting, if the process only needs 2 pids bc it is L1, current behavior is to return -EINVAL.

I have this check in copy_target_pids():

(quoted diff) +2 -0

+       if (num_pids < 0 || num_pids > nesting)
+               return ERR_PTR(-EINVAL);

Is it ok to return -EINVAL when num_pids > nesting ?