<div dir="ltr">Sorry for the delay in getting back. Here are the answers I owe you:<div><div><br></div><div>1. Re kernel, I am running 3.13 kernel. I also have a 3.16 kernel, but nothing higher.</div><div><br></div><div>2. Re breaking things, we won't break anything to the best of my knowledge. So it's safe to disallow image directory inside root.</div></div><div><br></div><div>3. Re umount_cgyard(), it works with the following patch. It's a quick hack because it assumes root is a subset of imgs_dir. What's the best way to generalize this?</div><div><br></div><div><div>diff --git a/cgroup.c b/cgroup.c</div><div>index b1a2f92..913f480 100644</div><div>--- a/cgroup.c</div><div>+++ b/cgroup.c</div><div>@@ -939,6 +939,22 @@ int prepare_task_cgroup(struct pstree_item *me)</div><div> return move_in_cgroup(se);</div><div> }</div><div> </div><div>+void umount_cgyard(void)</div><div>+{</div><div>+ char path[PATH_MAX];</div><div>+ int n;</div><div>+</div><div>+ if (!cg_yard)</div><div>+ return;</div><div>+</div><div>+ n = strlen(opts.root);</div><div>+ if (!strncmp(opts.imgs_dir, opts.root, n)) {</div><div>+ snprintf(path, PATH_MAX, "%s/%s", &opts.imgs_dir[n], cg_yard);</div><div>+ if (umount2(path, MNT_DETACH) == -1)</div><div>+ pr_perror("Unable to unmount cg_yard %s", path);</div><div>+ }</div><div>+}</div><div>+</div><div> void fini_cgroup(void)</div><div> {</div><div> if (!cg_yard)</div><div>diff --git a/crtools.c b/crtools.c</div><div>index 0b3c497..202acd8 100644</div><div>--- a/crtools.c</div><div>+++ b/crtools.c</div><div>@@ -278,6 +278,7 @@ int main(int argc, char *argv[], char *envp[])</div><div> opts.restore_sibling = true;</div><div> break;</div><div> case 'D':</div><div>+ opts.imgs_dir = optarg;</div><div> imgs_dir = optarg;</div><div> break;</div><div> case 'W':</div><div>diff --git a/include/cr_options.h b/include/cr_options.h</div><div>index d6ce2ae..90eb0ed 100644</div><div>--- a/include/cr_options.h</div><div>+++ b/include/cr_options.h</div><div>@@ -60,6 +60,7 @@ struct cr_options {</div><div> char *new_global_cg_root;</div><div> struct list_head new_cgroup_roots;</div><div> bool aufs; /* auto-deteced, not via cli */</div><div>+ char *imgs_dir;</div><div> };</div><div> </div><div> extern struct cr_options opts;</div><div>diff --git a/mount.c b/mount.c</div><div>index e59cb0d..df668ce 100644</div><div>--- a/mount.c</div><div>+++ b/mount.c</div><div>@@ -2013,12 +2013,16 @@ static int do_restore_task_mnt_ns(struct ns_id *nsid)</div><div> </div><div> int restore_task_mnt_ns(struct pstree_item *current)</div><div> {</div><div>+ extern void umount_cgyard(void);</div><div>+</div><div> if (current->ids && current->ids->has_mnt_ns_id) {</div><div> unsigned int id = current->ids->mnt_ns_id;</div><div> struct ns_id *nsid;</div><div> </div><div>- if (root_item->ids->mnt_ns_id == id)</div><div>+ if (root_item->ids->mnt_ns_id == id) {</div><div>+ umount_cgyard();</div><div> return 0;</div><div>+ }</div><div> </div><div> nsid = lookup_ns_by_id(id, &mnt_ns_desc);</div><div> if (nsid == NULL) {</div><div>@@ -2026,6 +2030,7 @@ int restore_task_mnt_ns(struct pstree_item *current)</div><div> return -1;</div><div> }</div><div> </div><div>+ umount_cgyard();</div><div> if (do_restore_task_mnt_ns(nsid))</div><div> return -1;</div><div> }</div></div><div><br></div><div>--Saied</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 24, 2015 at 11:52 AM, Pavel Emelyanov <span dir="ltr"><<a href="mailto:xemul@parallels.com" target="_blank">xemul@parallels.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 03/24/2015 09:30 PM, Saied Kazemi wrote:<br>
<span class="">><br>
><br>
> On Tue, Mar 24, 2015 at 10:57 AM, Pavel Emelyanov <<a href="mailto:xemul@parallels.com">xemul@parallels.com</a> <mailto:<a href="mailto:xemul@parallels.com">xemul@parallels.com</a>>> wrote:<br>
><br>
> On 03/24/2015 08:25 PM, Saied Kazemi wrote:<br>
> > For the easy case of comparing pathname strings on the command line (-D and --root), I<br>
> > think it's better to error and exit instead of restoring with all the cgyard mounts<br>
> > leaked into the restored process tree.<br>
><br>
> I rather agree with this. It's quite uncommon (from my perspective) that images sit<br>
> in the container's root, so preventing this usage doesn't (shouldn't) break anything<br>
> critical. Does it?<br>
><br>
><br>
> It wouldn't break anything because nothing has been merged yet.<br>
<br>
</span>I meant -- right now we allow for images to be under root, but we plan<br>
to ban this and if we do, nothing should get broken.<br>
<span class=""><br>
> > For a more general solution, is it possible to make sure that after fini_cgroup() the<br>
> > cgyard mount points have been successfully removed from the restored tree?<br>
><br>
> What if we call umount with detach flag on cgyard dir inside restore_task_mnt_ns()?<br>
> Would this resolve this?<br>
><br>
><br>
> I just quickly tried the following patch, first calling umount_cgyard() before if and then<br>
> after if. Neither case worked :(<br>
<br>
</span>The cg_yard variable is just a ".criu.cgyard.XXXXX" relative path. When<br>
calling umount2 on it kernel would try to resolve one relative to task's<br>
cwd.<br>
<br>
As far "calling this after if" is concerned -- yes, it's supposed to fail<br>
since restoring task's mntns implies calling pivot_roo() which moves task's<br>
cwd into new root and thus the cg_yard starts pointing to non-existing<br>
directory.<br>
<br>
But "before if"... I have no explanation for it. With what error does the<br>
umount fail in this case?<br>
<div class="HOEnZb"><div class="h5"><br>
> --Saied<br>
><br>
> diff --git a/cgroup.c b/cgroup.c<br>
> index b1a2f92..fa94853 100644<br>
> --- a/cgroup.c<br>
> +++ b/cgroup.c<br>
> @@ -951,6 +951,12 @@ void fini_cgroup(void)<br>
> cg_yard = NULL;<br>
> }<br>
><br>
> +void umount_cgyard(void)<br>
> +{<br>
> + if (cg_yard)<br>
> + umount2(cg_yard, MNT_DETACH);<br>
> +}<br>
> +<br>
> static int restore_cgroup_prop(const CgroupPropEntry * cg_prop_entry_p,<br>
> char *path, int off)<br>
> {<br>
> diff --git a/include/cgroup.h b/include/cgroup.h<br>
> index 14fef67..5bcf028 100644<br>
> --- a/include/cgroup.h<br>
> +++ b/include/cgroup.h<br>
> @@ -10,6 +10,7 @@ int prepare_cgroup(void);<br>
> /* Restore things like cpu_limit in known cgroups. */<br>
> int prepare_cgroup_properties(void);<br>
> void fini_cgroup(void);<br>
> +void umount_cgyard(void);<br>
><br>
> struct cg_controller;<br>
><br>
> diff --git a/mount.c b/mount.c<br>
> index e59cb0d..b0b34cb 100644<br>
> --- a/mount.c<br>
> +++ b/mount.c<br>
> @@ -2028,6 +2028,7 @@ int restore_task_mnt_ns(struct pstree_item *current)<br>
><br>
> if (do_restore_task_mnt_ns(nsid))<br>
> return -1;<br>
> + umount_cgyard();<br>
> }<br>
><br>
> return 0;<br>
><br>
<br>
</div></div></blockquote></div><br></div>