[CRIU] [PATCH 2/2] mount: don't clean up propogation options for the root mount (v2)

[Andrew Vagin]

On Thu, Mar 20, 2014 at 01:28:08PM +0400, Pavel Emelyanov wrote:
> On 03/18/2014 11:46 PM, Andrey Vagin wrote:
> > Currently we marks all mounts as private before restoring mntns. We do
> > these to avoid problem with pivot_root.
> > It's wrong, because the root mount can be slave for an external shared
> > group. The root mount is not mounted by CRIU, so here is nothing wrong.
> > 
> > Now look at the pivot_root code in kernel
> > if (IS_MNT_SHARED(old_mnt) ||
> > 	IS_MNT_SHARED(new_mnt->mnt_parent) ||
> > 	IS_MNT_SHARED(root_mnt->mnt_parent))
> > 	goto out4;
> > 
> > So we don't need to change options for all mounts. We need to remount
> > / and the parent of the new root. It's safe, because we already in another
> > mntns.
> > 
> > v2: simplify code
> > 
> > Signed-off-by: Andrey Vagin <avagin at openvz.org>
> > ---
> >  mount.c | 39 ++++++++++++++++++++++++++++++++-------
> >  1 file changed, 32 insertions(+), 7 deletions(-)
> > 
> > diff --git a/mount.c b/mount.c
> > index c6a110a..a0daee5 100644
> > --- a/mount.c
> > +++ b/mount.c
> > @@ -1238,6 +1238,11 @@ static int cr_pivot_root(void)
> >  		return -1;
> >  	}
> >  
> > +	if (mount("none", put_root, "none", MS_REC|MS_PRIVATE, NULL)) {
> > +		pr_perror("Can't remount root with MS_PRIVATE");
> > +		return -1;
> > +	}
> > +
> >  	if (umount2(put_root, MNT_DETACH)) {
> >  		pr_perror("Can't umount %s", put_root);
> >  		return -1;
> > @@ -1411,11 +1416,6 @@ int prepare_mnt_ns(int ns_pid)
> >  	if (!mis)
> >  		goto out;
> >  
> > -	if (mount("none", "/", "none", MS_REC|MS_PRIVATE, NULL)) {
> > -		pr_perror("Can't remount root with MS_PRIVATE");
> > -		return -1;
> > -	}
> > -
> >  	if (chdir(opts.root ? : "/")) {
> >  		pr_perror("chdir(%s) failed", opts.root ? : "/");
> >  		return -1;
> > @@ -1426,8 +1426,33 @@ int prepare_mnt_ns(int ns_pid)
> >  	 * clones from the original one. We have to umount them
> >  	 * prior to recreating new ones.
> >  	 */
> > -	if (!opts.root && clean_mnt_ns())
> > -		return -1;
> > +	if (!opts.root) {
> > +		if (clean_mnt_ns())
> > +			return -1;
> > +	} else {
> > +		struct mount_info *mi;
> > +
> > +		/* moving a mount residing under a shared mount is invalid. */
> > +		mi = mount_resolve_path(opts.root);
> > +		if (mi == NULL) {
> > +			pr_err("Unable to find mount point for %s\n", opts.root);
> > +			return -1;
> > +		}
> > +		if (mi->parent == NULL) {
> > +			pr_err("New root and old root are the same\n");
> > +			return -1;
> > +		}
> > +
> > +		if (!strcmp(mi->parent->mountpoint, mi->mountpoint)) {
> 
> Add some comment saying why mountpoints match means "parent of the root is unreachable".

Out root is mounted over the parent (in the same directory), so we can't
changes properties of the parent.

> 
> > +			pr_err("The parent of the new root is unreachable\n");
> > +			return -1;
> > +		}
> > +
> > +		if (mount("none", mi->parent->mountpoint, "none", MS_SLAVE, NULL)) {
> 
> Last time there were two MS_SLAVE marks ;) Where did the 2nd one disappear?

We don't need to mark "/" as a slave, I read a kernel code incorrectly.

> 
> > +			pr_perror("Can't remount the parent of the new root with MS_SLAVE");
> > +			return -1;
> > +		}
> > +	}
> >  
> >  	free_mounts();
> >  
> > 
> 
> 
> Thanks,
> Pavel