Re: [PATCH linux-cr] nested pid namespaces (v2)

["Serge E. Hallyn" <serue@us.ibm.com>]

Quoting Oren Laadan (orenl@cs.columbia.edu):
> 
> 
> Serge E. Hallyn wrote:
> >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.
> 
> >(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@us.ibm.com>
> >---
> 
> [...]

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

> >@@ -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;
> 
> 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.

...

> >diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> >index 0da0d83..6d86240 100644
> >--- a/kernel/nsproxy.c
> >+++ b/kernel/nsproxy.c
> >@@ -364,8 +364,13 @@ static struct nsproxy *do_restore_ns(struct ckpt_ctx *ctx)
> > 		get_net(net_ns);
> > 		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 ?

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.

thanks,
-serge