nested pid namespaces (v2)

Support checkpoint and restart of tasks in nested pid namespaces. At Oren's request here is an alternative to my previous implementation. In this one, we keep the original single pids_array to minimize memory allocations. The pids array entries are augmented with a pidns depth (relative to the container init's pidns, and an "rpid" which is the pid in the checkpointer's pidns (or 0 if no valid pid exists). The rpid will be used by userspace to gather more information (like /proc/$$/mountinfo) after the kernel sys_checkpoint. If any tasks are in nested pid namespace, another single array holds all of the vpids. At restart those are used by userspace to determine how to call eclone(). Kernel ignores them.

All cr_tests including the new pid_ns testcase pass.

Signed-off-by Serge E. Hallyn <serue-r/[email protected]>

---
 checkpoint/checkpoint.c          |  113 ++++++++++++++++++++++++++++++++++----
 checkpoint/process.c             |   18 +++++-
 checkpoint/restart.c             |   45 ++++++++++++++-
 checkpoint/sys.c                 |    2 +
 include/linux/checkpoint.h       |    2 +-
 include/linux/checkpoint_hdr.h   |   16 +++++
 include/linux/checkpoint_types.h |    3 +
 kernel/nsproxy.c                 |    9 ++-
 8 files changed, 186 insertions(+), 22 deletions(-)

checkpoint/checkpoint.c +50 -11

index f27af41..fe3546a 100644
@@ -27,6 +27,7 @@
 #include <linux/deferqueue.h>
 #include <linux/checkpoint.h>
 #include <linux/checkpoint_hdr.h>
+#include <linux/pid_namespace.h>
 
 /* unique checkpoint identifier (FIXME: should be per-container ?) */
 static atomic_t ctx_count = ATOMIC_INIT(0);

@@ -242,6 +243,7 @@ static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t)
 	struct task_struct *root = ctx->root_task;
 	struct nsproxy *nsproxy;
 	int ret = 0;
+	struct pid_namespace *pidns;
 
 	ckpt_debug("check %d\n", task_pid_nr_ns(t, ctx->root_nsproxy->pid_ns));
 

@@ -293,10 +295,15 @@ static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t)
 		_ckpt_err(ctx, -EPERM, "%(T)Nested net_ns unsupported\n");
 		ret = -EPERM;
 	}
-	/* no support for >1 private pidns */
-	if (nsproxy->pid_ns != ctx->root_nsproxy->pid_ns) {
-		_ckpt_err(ctx, -EPERM, "%(T)Nested pid_ns unsupported\n");
-		ret = -EPERM;
+	/* pidns must be descendent of root_nsproxy */
+	pidns = nsproxy->pid_ns;
+	while (pidns != ctx->root_nsproxy->pid_ns) {
+		if (pidns == &init_pid_ns) {
+			ret = -EPERM;
+			_ckpt_err(ctx, ret, "%(T)stranger pid_ns\n");
+			break;
+		}
+		pidns = pidns->parent;
 	}
 	rcu_read_unlock();
 

@@ -305,15 +312,19 @@ static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t)
 
 #define CKPT_HDR_PIDS_CHUNK	256
 
+/*
+ * Write the pids in ctx->root_nsproxy->pidns.  This info is
+ * needed at restart to unambiguously dereference tasks.
+ */
 static int checkpoint_pids(struct ckpt_ctx *ctx)
 {
 	struct ckpt_pids *h;
-	struct pid_namespace *ns;
+	struct pid_namespace *root_pidns;
 	struct task_struct *task;
 	struct task_struct **tasks_arr;
 	int nr_tasks, n, pos = 0, ret = 0;
 
-	ns = ctx->root_nsproxy->pid_ns;
+	root_pidns = ctx->root_nsproxy->pid_ns;
 	tasks_arr = ctx->tasks_arr;
 	nr_tasks = ctx->nr_tasks;
 	BUG_ON(nr_tasks <= 0);

@@ -331,15 +342,21 @@ static int checkpoint_pids(struct ckpt_ctx *ctx)
 	do {
 		rcu_read_lock();
 		for (n = 0; n < min(nr_tasks, CKPT_HDR_PIDS_CHUNK); n++) {
+			struct pid_namespace *task_pidns;
 			task = tasks_arr[pos];
 
-			h[n].vpid = task_pid_nr_ns(task, ns);
-			h[n].vtgid = task_tgid_nr_ns(task, ns);
-			h[n].vpgid = task_pgrp_nr_ns(task, ns);
-			h[n].vsid = task_session_nr_ns(task, ns);
-			h[n].vppid = task_tgid_nr_ns(task->real_parent, ns);
+			h[n].vpid = task_pid_nr_ns(task, root_pidns);
+			h[n].vtgid = task_tgid_nr_ns(task, root_pidns);
+			h[n].vpgid = task_pgrp_nr_ns(task, root_pidns);
+			h[n].vsid = task_session_nr_ns(task, root_pidns);
+			h[n].vppid = task_tgid_nr_ns(task->real_parent,
+					root_pidns);
+			task_pidns = task_nsproxy(task)->pid_ns;
+			h[n].rpid = task_pid_vnr(task);
+			h[n].depth = task_pidns->level - root_pidns->level;
 			ckpt_debug("task[%d]: vpid %d vtgid %d parent %d\n",
 				   pos, h[n].vpid, h[n].vtgid, h[n].vppid);
+			ctx->nr_vpids += h[n].depth;
 			pos++;
 		}
 		rcu_read_unlock();

@@ -356,6 +373,61 @@ static int checkpoint_pids(struct ckpt_ctx *ctx)
 	return ret;
 }
 
+static int checkpoint_vpids(struct ckpt_ctx *ctx)
+{
+	struct ckpt_vpid *h;
+	struct pid_namespace *root_pidns, *task_pidns;
+	struct task_struct *task;
+	int ret, nr_tasks = ctx->nr_tasks;
+	int tidx = 0, /* index into task array */
+		hidx = 0; /* pids written into current ckpt_vpids chunk */
+
+	root_pidns = ctx->root_nsproxy->pid_ns;
+	nr_tasks = ctx->nr_tasks;
+
+	ret = ckpt_write_obj_type(ctx, NULL,
+				  sizeof(*h) * ctx->nr_vpids,
+				  CKPT_HDR_BUFFER);
+	if (ret < 0)
+		return ret;
+
+	h = ckpt_hdr_get(ctx, sizeof(*h) * CKPT_HDR_PIDS_CHUNK);
+	if (!h)
+		return -ENOMEM;
+

checkpoint/checkpoint.c +8 -0

+				task_pidns = task_pidns->parent;
+			}
+
+			hidx += nsdelta;
+			tidx++;
+		}
+		rcu_read_unlock();
+

Quoted 12 lines · Serge E. Hallyn

Support checkpoint and restart of tasks in nested pid namespaces. At Oren's request here is an alternative to my previous implementation. In this one, we keep the original single pids_array to minimize memory allocations. The pids array entries are augmented with a pidns depth (relative to the container init's pidns, and an "rpid" which is the pid in the checkpointer's pidns (or 0 if no valid pid exists). The rpid will be used by userspace to gather more information (like /proc/$$/mountinfo) after the kernel sys_checkpoint. If any tasks are in nested pid namespace, another single array holds all of the vpids. At restart those are used by userspace to determine how to call eclone(). Kernel ignores them.

Quoted 2 lines

All cr_tests including the new pid_ns testcase pass.

IMHO this approach looks ok too. I just feel that checkpoint_vpids() could be re-worked a bit in order to not impose an artificial limit of CKPT_HDR_PIDS_CHUNK to the depth of pid namespaces, even if it is 256 (see suggested changes below).

It would probably be safer too to use task_active_pid_ns() instead of task->nsproxy->pid_ns, just in case some PID namespace unsharing like proposed by Eric makes it to mainline.

Thanks,

Louis

Quoted 127 lines

Signed-off-by Serge E. Hallyn <[email protected]>

---
 checkpoint/checkpoint.c          |  113 ++++++++++++++++++++++++++++++++++----
 checkpoint/process.c             |   18 +++++-
 checkpoint/restart.c             |   45 ++++++++++++++-
 checkpoint/sys.c                 |    2 +
 include/linux/checkpoint.h       |    2 +-
 include/linux/checkpoint_hdr.h   |   16 +++++
 include/linux/checkpoint_types.h |    3 +
 kernel/nsproxy.c                 |    9 ++-
 8 files changed, 186 insertions(+), 22 deletions(-)

checkpoint/checkpoint.c +50 -11

index f27af41..fe3546a 100644
@@ -27,6 +27,7 @@
 #include <linux/deferqueue.h>
 #include <linux/checkpoint.h>
 #include <linux/checkpoint_hdr.h>
+#include <linux/pid_namespace.h>
 
 /* unique checkpoint identifier (FIXME: should be per-container ?) */
 static atomic_t ctx_count = ATOMIC_INIT(0);

@@ -242,6 +243,7 @@ static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t)
 	struct task_struct *root = ctx->root_task;
 	struct nsproxy *nsproxy;
 	int ret = 0;
+	struct pid_namespace *pidns;
 
 	ckpt_debug("check %d\n", task_pid_nr_ns(t, ctx->root_nsproxy->pid_ns));
 

@@ -293,10 +295,15 @@ static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t)
 		_ckpt_err(ctx, -EPERM, "%(T)Nested net_ns unsupported\n");
 		ret = -EPERM;
 	}
-	/* no support for >1 private pidns */
-	if (nsproxy->pid_ns != ctx->root_nsproxy->pid_ns) {
-		_ckpt_err(ctx, -EPERM, "%(T)Nested pid_ns unsupported\n");
-		ret = -EPERM;
+	/* pidns must be descendent of root_nsproxy */
+	pidns = nsproxy->pid_ns;
+	while (pidns != ctx->root_nsproxy->pid_ns) {
+		if (pidns == &init_pid_ns) {
+			ret = -EPERM;
+			_ckpt_err(ctx, ret, "%(T)stranger pid_ns\n");
+			break;
+		}
+		pidns = pidns->parent;
 	}
 	rcu_read_unlock();
 

@@ -305,15 +312,19 @@ static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t)
 
 #define CKPT_HDR_PIDS_CHUNK	256
 
+/*
+ * Write the pids in ctx->root_nsproxy->pidns.  This info is
+ * needed at restart to unambiguously dereference tasks.
+ */
 static int checkpoint_pids(struct ckpt_ctx *ctx)
 {
 	struct ckpt_pids *h;
-	struct pid_namespace *ns;
+	struct pid_namespace *root_pidns;
 	struct task_struct *task;
 	struct task_struct **tasks_arr;
 	int nr_tasks, n, pos = 0, ret = 0;
 
-	ns = ctx->root_nsproxy->pid_ns;
+	root_pidns = ctx->root_nsproxy->pid_ns;
 	tasks_arr = ctx->tasks_arr;
 	nr_tasks = ctx->nr_tasks;
 	BUG_ON(nr_tasks <= 0);

@@ -331,15 +342,21 @@ static int checkpoint_pids(struct ckpt_ctx *ctx)
 	do {
 		rcu_read_lock();
 		for (n = 0; n < min(nr_tasks, CKPT_HDR_PIDS_CHUNK); n++) {
+			struct pid_namespace *task_pidns;
 			task = tasks_arr[pos];
 
-			h[n].vpid = task_pid_nr_ns(task, ns);
-			h[n].vtgid = task_tgid_nr_ns(task, ns);
-			h[n].vpgid = task_pgrp_nr_ns(task, ns);
-			h[n].vsid = task_session_nr_ns(task, ns);
-			h[n].vppid = task_tgid_nr_ns(task->real_parent, ns);
+			h[n].vpid = task_pid_nr_ns(task, root_pidns);
+			h[n].vtgid = task_tgid_nr_ns(task, root_pidns);
+			h[n].vpgid = task_pgrp_nr_ns(task, root_pidns);
+			h[n].vsid = task_session_nr_ns(task, root_pidns);
+			h[n].vppid = task_tgid_nr_ns(task->real_parent,
+					root_pidns);
+			task_pidns = task_nsproxy(task)->pid_ns;
+			h[n].rpid = task_pid_vnr(task);
+			h[n].depth = task_pidns->level - root_pidns->level;
 			ckpt_debug("task[%d]: vpid %d vtgid %d parent %d\n",
 				   pos, h[n].vpid, h[n].vtgid, h[n].vppid);
+			ctx->nr_vpids += h[n].depth;
 			pos++;
 		}
 		rcu_read_unlock();

@@ -356,6 +373,61 @@ static int checkpoint_pids(struct ckpt_ctx *ctx)
 	return ret;
 }
 
+static int checkpoint_vpids(struct ckpt_ctx *ctx)
+{
+	struct ckpt_vpid *h;
+	struct pid_namespace *root_pidns, *task_pidns;
+	struct task_struct *task;
+	int ret, nr_tasks = ctx->nr_tasks;
+	int tidx = 0, /* index into task array */
+		hidx = 0; /* pids written into current ckpt_vpids chunk */
+
+	root_pidns = ctx->root_nsproxy->pid_ns;
+	nr_tasks = ctx->nr_tasks;
+
+	ret = ckpt_write_obj_type(ctx, NULL,
+				  sizeof(*h) * ctx->nr_vpids,
+				  CKPT_HDR_BUFFER);
+	if (ret < 0)
+		return ret;
+
+	h = ckpt_hdr_get(ctx, sizeof(*h) * CKPT_HDR_PIDS_CHUNK);
+	if (!h)
+		return -ENOMEM;
+

checkpoint/checkpoint.c static int checkpoint_pids(struct ckpt_ctx *ctx) · line 407

Quoted 10 lines

checkpoint/checkpoint.c +10 -0

+	do {
+		rcu_read_lock();
+		while (tidx < nr_tasks) {
+			int vidx; /* vpid index */
+			int nsdelta;
+
+			task = ctx->tasks_arr[tidx];
+			task_pidns = task_nsproxy(task)->pid_ns;
+			nsdelta = task_pidns->level - root_pidns->level;
+			if (hidx + nsdelta >= CKPT_HDR_PIDS_CHUNK)

I think that (hidx + nsdelta > CKPT_HDR_PIDS_CHUNK) checks more accurately the limit.

checkpoint/checkpoint.c static int checkpoint_pids(struct ckpt_ctx *ctx) · line 411

Quoted 4 lines

checkpoint/checkpoint.c +4 -0

+				break;
+
+			for (vidx = 0; vidx < nsdelta; vidx++) {
+				h[vidx].pid = task_pid_nr_ns(task, task_pidns);

Here:
h[hidx + vidx]

Quoted 8 lines

checkpoint/checkpoint.c +8 -0

+				task_pidns = task_pidns->parent;
+			}
+
+			hidx += nsdelta;
+			tidx++;
+		}
+		rcu_read_unlock();
+

checkpoint/checkpoint.c static int checkpoint_pids(struct ckpt_ctx *ctx) · line 429

Quoted 10 lines

checkpoint/checkpoint.c +10 -0

+		ret = ckpt_kwrite(ctx, h, hidx * sizeof(*h));
+		if (ret < 0)
+			break;
+
+		hidx = 0;
+	} while (tidx < nr_tasks);
+
+	_ckpt_hdr_put(ctx, h, sizeof(*h) * CKPT_HDR_PIDS_CHUNK);
+	return ret;
+}

Maybe re-work it this way:

static int checkpoint_vpids(struct ckpt_ctx *ctx)

{
	struct ckpt_vpid *h;
	struct pid_namespace *root_pidns, *task_pidns, *active_pidns;
	struct task_struct *task;
	int ret, nr_tasks = ctx->nr_tasks;
	int tidx = 0, /* index into task array */
		hidx = 0; /* pids written into current ckpt_vpids chunk */
		vidx = 0; /* vpid index for current task */

	root_pidns = ctx->root_nsproxy->pid_ns;
	nr_tasks = ctx->nr_tasks;

	ret = ckpt_write_obj_type(ctx, NULL,
				  sizeof(*h) * ctx->nr_vpids,
				  CKPT_HDR_BUFFER);
	if (ret < 0)
		return ret;

	h = ckpt_hdr_get(ctx, sizeof(*h) * CKPT_HDR_PIDS_CHUNK);
	if (!h)
		return -ENOMEM;

	do {
		rcu_read_lock();
		while (tidx < nr_tasks && hidx < CKPT_HDR_PIDS_CHUNK) {
			int nsdelta;

			task = ctx->tasks_arr[tidx];
			active_pidns = task_active_pid_ns(task);

			nsdelta = active_pidns->level - root_pidns->level;
			if (hidx + nsdelta - vidx > CKPT_HDR_PIDS_CHUNK)
				/*
				 * We will release rcu before recording the
				 * remaining vpids, but neither task nor its
				 * pid can disappear.
				 */
				nsdelta = CKPT_HDR_PIDS_CHUNK - hidx + vidx;

			if (vidx == 0)
				task_pidns = active_pidns;
			for (; vidx < nsdelta; vidx++) {
				h[hidx].pid = task_pid_nr_ns(task, task_pidns);
				hidx++;
				task_pidns = task_pidns->parent;
			}

			if (task_pidns == root_pidns) {
				tidx++;
				vidx = 0;
			}
		}
		rcu_read_unlock();

		ret = ckpt_kwrite(ctx, h, hidx * sizeof(*h));
		if (ret < 0)
			break;

		hidx = 0;
	} while (tidx < nr_tasks);

	_ckpt_hdr_put(ctx, h, sizeof(*h) * CKPT_HDR_PIDS_CHUNK);
	return ret;
}

[...]

Signature · 4 lines

Dr Louis Rilling			Kerlabs
Skype: louis.rilling			Batiment Germanium
Phone: (+33|0) 6 80 89 08 23		80 avenue des Buttes de Coesmes
http://www.kerlabs.com/			35700 Rennes

Quoted 2 lines · Louis Rilling

Quoted 1 line · Serge E. Hallyn

Support checkpoint and restart of tasks in nested pid namespaces. At

Quoted 3 lines

It would probably be safer too to use task_active_pid_ns() instead of task->nsproxy->pid_ns, just in case some PID namespace unsharing like proposed by Eric makes it to mainline.

The task is frozen though so it shouldn't be able to unshare while being checkpointed, right? But it's probably better code anyway.

Quoted 3 lines · Serge E. Hallyn

Quoted 2 lines · Louis Rilling

Quoted 1 line · Serge E. Hallyn

Support checkpoint and restart of tasks in nested pid namespaces. At

<snip>

Quoted 4 lines

Quoted 3 lines

It would probably be safer too to use task_active_pid_ns() instead of task->nsproxy->pid_ns, just in case some PID namespace unsharing like proposed by Eric makes it to mainline.

Quoted 2 lines

The task is frozen though so it shouldn't be able to unshare while being checkpointed, right? But it's probably better code anyway.

By the time it reaches checkpoint a frozen task is in the refrigerator -- most often in the signal delivery portion of syscall return. So it can't be making any new unshare/setns syscalls and any changes to the namespaces should be visible.

Support checkpoint and restart of tasks in nested pid namespaces. At Oren's request here is an alternative to my previous implementation. In this one, we keep the original single pids_array to minimize memory allocations. The pids array entries are augmented with a pidns depth

(relative to the container init's pidns, and an "rpid" which is the pid in the checkpointer's pidns (or 0 if no valid pid exists). The rpid will be used by userspace to gather more information (like /proc/$$/mountinfo) after the kernel sys_checkpoint. If any tasks are in nested pid namespace, another single array holds all of the vpids. At restart those are used by userspace to determine how to call eclone(). Kernel ignores them.

All cr_tests including the new pid_ns testcase pass.

Signed-off-by Serge E. Hallyn <[email protected]>

---

checkpoint/checkpoint.c +0 -3

@@ -293,10 +295,15 @@ static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t)
 		_ckpt_err(ctx, -EPERM, "%(T)Nested net_ns unsupported\n");
 		ret = -EPERM;
 	}
-	/* no support for >1 private pidns */
-	if (nsproxy->pid_ns != ctx->root_nsproxy->pid_ns) {
-		_ckpt_err(ctx, -EPERM, "%(T)Nested pid_ns unsupported\n");

checkpoint/process.c +2 -0

+		if (!nsproxy->pid_ns)
+			nsproxy->pid_ns = get_pid_ns(current->nsproxy->pid_ns);
 		get_nsproxy(nsproxy);
 		switch_task_namespaces(current, nsproxy);
 	}

+/*
+ * read all the vpids - we don't actually care about them,
+ * userspace did
+ */

checkpoint/restart.c +29 -0

+static int restore_slurp_vpids(struct ckpt_ctx *ctx)
+{
+	struct ckpt_hdr_vpids *h;
+	int size, ret;
+	void *junk;
+
+	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_VPIDS);
+	if (IS_ERR(h))
+		return PTR_ERR(h);
+	ctx->nr_vpids = h->nr_vpids;
+	ckpt_hdr_put(ctx, h);
+
+	if (!ctx->nr_vpids)
+		return 0;
+
+	size = sizeof(struct ckpt_vpid) * ctx->nr_vpids;
+	if (size < 0)		/* overflow ? */
+		return -EINVAL;
+
+	junk = kmalloc(size, GFP_KERNEL);
+	if (!junk)
+		return -ENOMEM;
+
+	ret = _ckpt_read_buffer(ctx, junk, size);
+	kfree(junk);
+
+	return ret;
+}
+

} __attribute__((aligned(8)));

/* pids */

include/linux/checkpoint_types.h +3 -0

index ecd3e91..2fb79cf 100644
@@ -72,6 +72,9 @@ struct ckpt_ctx {
 	struct task_struct **tasks_arr; /* array of all tasks [checkpoint] */
 	int nr_tasks;                   /* size of tasks array */
 
+	int nr_vpids;
+	struct pid_namespace *coord_pidns;	/* coordinator pid_ns */
+
 	/* [multi-process restart] */
 	struct ckpt_pids *pids_arr;	/* array of all pids [restart] */
 	int nr_pids;			/* size of pids array */

kernel/nsproxy.c +0 -0

index 0da0d83..6d86240 100644
@@ -364,8 +364,13 @@ static struct nsproxy *do_restore_ns(struct ckpt_ctx *ctx)
 		get_net(net_ns);
 		nsproxy->net_ns = net_ns;

Quoted 4 lines · Serge E. Hallyn

Support checkpoint and restart of tasks in nested pid namespaces. At Oren's request here is an alternative to my previous implementation. In this one, we keep the original single pids_array to minimize memory allocations. The pids array entries are augmented with a pidns depth

Thanks for adapting the patch.

FWIW, not only minimize memory allocations, but also permit a more regular structure of the image data (array of fixed size elements followed by an array of vpids), which simplifies the code that needs to read/write/access this data.

Quoted 8 lines

(relative to the container init's pidns, and an "rpid" which is the pid in the checkpointer's pidns (or 0 if no valid pid exists). The rpid will be used by userspace to gather more information (like /proc/$$/mountinfo) after the kernel sys_checkpoint. If any tasks are in nested pid namespace, another single array holds all of the vpids. At restart those are used by userspace to determine how to call eclone(). Kernel ignores them.

Quoted 4 lines

All cr_tests including the new pid_ns testcase pass.

Signed-off-by Serge E. Hallyn <[email protected]>

---

[...]

Quoted 14 lines

checkpoint/checkpoint.c +6 -4

@@ -293,10 +295,15 @@ static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t)
 		_ckpt_err(ctx, -EPERM, "%(T)Nested net_ns unsupported\n");
 		ret = -EPERM;
 	}
-	/* no support for >1 private pidns */
-	if (nsproxy->pid_ns != ctx->root_nsproxy->pid_ns) {
-		_ckpt_err(ctx, -EPERM, "%(T)Nested pid_ns unsupported\n");
-		ret = -EPERM;
+	/* pidns must be descendent of root_nsproxy */
+	pidns = nsproxy->pid_ns;
+	while (pidns != ctx->root_nsproxy->pid_ns) {
+		if (pidns == &init_pid_ns) {
+			ret = -EPERM;
+			_ckpt_err(ctx, ret, "%(T)stranger pid_ns\n");

checkpoint/checkpoint.c static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t) · line 306

Quoted 3 lines

checkpoint/checkpoint.c +3 -0

+			break;
+		}
+		pidns = pidns->parent;

Currently we do this while() loop twice - once here and once when we collect the vpids. While I doubt if this has any performance impact, is there an advantage to doing it also here ? (a violation will be observed there too).

Quoted 6 lines

kernel/nsproxy.c +0 -0

index 0da0d83..6d86240 100644
@@ -364,8 +364,13 @@ static struct nsproxy *do_restore_ns(struct ckpt_ctx *ctx)
 		get_net(net_ns);

kernel/nsproxy.c static struct nsproxy *do_restore_ns(struct ckpt_ctx *ctx) · line 372

Quoted 10 lines

kernel/nsproxy.c +7 -2

 		nsproxy->net_ns = net_ns;
-		get_pid_ns(current->nsproxy->pid_ns);
-		nsproxy->pid_ns = current->nsproxy->pid_ns;
+		/*
+		 * The pid_ns will get assigned the first time that we
+		 * assign the nsproxy to a task.  The task had unshared
+		 * its pid_ns in userspace before calling restart, and
+		 * we want to keep using that pid_ns.
+		 */
+		nsproxy->pid_ns = NULL;

This doesn't look healthy.

If it is (or will be) possible for another process to look at the restarting process, not having a pid-ns may confuse other code in the kernel ?

Quoted 21 lines · Oren Laadan

Quoted 4 lines · Serge E. Hallyn

Support checkpoint and restart of tasks in nested pid namespaces. At Oren's request here is an alternative to my previous implementation. In this one, we keep the original single pids_array to minimize memory allocations. The pids array entries are augmented with a pidns depth

Thanks for adapting the patch.

FWIW, not only minimize memory allocations, but also permit a more regular structure of the image data (array of fixed size elements followed by an array of vpids), which simplifies the code that needs to read/write/access this data.

Quoted 8 lines

(relative to the container init's pidns, and an "rpid" which is the pid in the checkpointer's pidns (or 0 if no valid pid exists). The rpid will be used by userspace to gather more information (like /proc/$$/mountinfo) after the kernel sys_checkpoint. If any tasks are in nested pid namespace, another single array holds all of the vpids. At restart those are used by userspace to determine how to call eclone(). Kernel ignores them.

Quoted 5 lines

Quoted 4 lines

All cr_tests including the new pid_ns testcase pass.

Signed-off-by Serge E. Hallyn <[email protected]>

---

[...]

Thanks, Oren - all other input is taken into what I'm about to post, except:

Quoted 13 lines

Quoted 13 lines

checkpoint/checkpoint.c +5 -4

@@ -293,10 +295,15 @@ static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t)
 		_ckpt_err(ctx, -EPERM, "%(T)Nested net_ns unsupported\n");
 		ret = -EPERM;
 	}
-	/* no support for >1 private pidns */
-	if (nsproxy->pid_ns != ctx->root_nsproxy->pid_ns) {
-		_ckpt_err(ctx, -EPERM, "%(T)Nested pid_ns unsupported\n");
-		ret = -EPERM;
+	/* pidns must be descendent of root_nsproxy */
+	pidns = nsproxy->pid_ns;
+	while (pidns != ctx->root_nsproxy->pid_ns) {
+		if (pidns == &init_pid_ns) {
+			ret = -EPERM;

Quoted 8 lines

checkpoint/checkpoint.c static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t) · line 306

Quoted 4 lines

checkpoint/checkpoint.c +4 -0

+			_ckpt_err(ctx, ret, "%(T)stranger pid_ns\n");
+			break;
+		}
+		pidns = pidns->parent;

Currently we do this while() loop twice - once here and once when we collect the vpids. While I doubt if this has any performance impact, is there an advantage to doing it also here ? (a violation will be observed there too).

With the new logic (ripped verbatim from Louis' email) such a move would make the checkpoint_vpids() code a bit uglier. I'm about to resend, please let me know if you still want the code moved.

...

Quoted 18 lines

Quoted 6 lines

kernel/nsproxy.c +0 -0

index 0da0d83..6d86240 100644
@@ -364,8 +364,13 @@ static struct nsproxy *do_restore_ns(struct ckpt_ctx *ctx)
 		get_net(net_ns);

kernel/nsproxy.c static struct nsproxy *do_restore_ns(struct ckpt_ctx *ctx) · line 372

Quoted 10 lines

kernel/nsproxy.c +7 -2

 		nsproxy->net_ns = net_ns;
-		get_pid_ns(current->nsproxy->pid_ns);
-		nsproxy->pid_ns = current->nsproxy->pid_ns;
+		/*
+		 * The pid_ns will get assigned the first time that we
+		 * assign the nsproxy to a task.  The task had unshared
+		 * its pid_ns in userspace before calling restart, and
+		 * we want to keep using that pid_ns.
+		 */
+		nsproxy->pid_ns = NULL;

This doesn't look healthy.

Quoted 3 lines

If it is (or will be) possible for another process to look at the restarting process, not having a pid-ns may confuse other code in the kernel ?

No task will have this nproxy attached before we assign a valid pid_ns. The NULL pid_ns is only while it is in the objhash but not attached to a task.