[CRIU] [PATCH 6/9] mntns: rework validation to support non-root shared bind-mounts

[Andrew Vagin]

On Wed, Oct 29, 2014 at 04:25:27PM +0400, Pavel Emelyanov wrote:
> On 10/23/2014 05:49 PM, Andrey Vagin wrote:
> > Currently we can restore shared bind-mounts only if they have the same
> > set of children. But if we have a non-root bind-mount, we should compare
> > children, which are accessiable from both mounts.
> > 
> > Now we take the next mount in the shared group, which is wider or equal
> > to current and compare children between them.
> > 
> > Signed-off-by: Andrey Vagin <avagin at openvz.org>
> > ---
> >  mount.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 80 insertions(+), 12 deletions(-)
> > 
> > diff --git a/mount.c b/mount.c
> > index bd51686..7ccdef3 100644
> > --- a/mount.c
> > +++ b/mount.c
> > @@ -372,28 +372,96 @@ static int try_resolve_ext_mount(struct mount_info *info)
> >  	info->external = em;
> >  	return 0;
> >  }
> > +
> >  static int validate_shared(struct mount_info *m)
> >  {
> > -	struct mount_info *ct, *t;
> > +	struct mount_info *t, *ct, *cm, *tmp;
> > +	int t_root_len, m_root_len, tpm, mpm;
> > +	LIST_HEAD(children);
> > +
> > +	/* Try to find a mount, which is wider or equal */
> 
> What does "wide mount" mean? And why should we search for it?

Lets imagine that we have two mounts A and B. A is wider than B. if
A->root is a substring of B->root.

> 
> > +	list_for_each_entry(t, &m->mnt_share, mnt_share) {
> > +		if (strncmp(t->root, m->root, strlen(t->root)) == 0)
> > +			break;
> > +	}
> >  
> > -	if (!list_empty(&m->parent->mnt_share))
> > +	/*
> > +	 * The current mount is widest one in its shared group,
> > +	 * all others should be compared with it.
> 
> Why, if they should, the function returns?

m is compared with t, where t is always wider that m.

When we will compare other mounts from this group, we will compare them
with a mount, which is wider. We takes the first wider mount from the
list starting with a current position, so we can garantee that all
mounts has the same set of children.

> 
> > +	 */
> > +	if (&t->mnt_share == &m->mnt_share)
> >  		return 0;
> >  
> > -	t = list_first_entry(&m->parent->mnt_share, struct mount_info, mnt_share);
> > +	/* A set of childrent which ar visiable for both should be the same */
> >  
> > +	t_root_len = strlen(t->root);
> > +	m_root_len = strlen(m->root);
> > +	tpm = strlen(t->mountpoint);
> > +	mpm = strlen(m->mountpoint);
> > +	if (t->mountpoint[tpm - 1] == '/')
> > +		tpm--;
> > +	if (m->mountpoint[mpm - 1] == '/')
> > +		mpm--;
> > +
> > +	/*
> > +	 * ct:	| t->root	|        /reletive mount point	|
> > +	 * cm:	|	m->root		| /reletive mount point	|
> > +	 * 			| A     | B			|
> > +	 *      \ct->mountpoint + tpm
> > +	 *		     \m->root + strlen(t->root)
> > +	 */
> >  	list_for_each_entry(ct, &t->children, siblings) {
> > -		if (mounts_equal(m, ct, false))
> > +		char *tp, *mp;
> > +		int len;
> > +
> > +		if (ct->is_ns_root)
> > +			continue;
> > +
> > +		tp = ct->mountpoint + tpm;
> > +		mp = m->root + t_root_len;
> > +		len = m_root_len - t_root_len;
> > +
> > +		/* A */
> > +		if (strncmp(tp, mp, len))
> > +			continue;
> > +		if (tp[len] != '/')
> > +			continue;
> > +
> > +		list_for_each_entry_safe(cm, tmp, &m->children, siblings) {
> > +			/* B */
> > +			if (strcmp(ct->mountpoint + tpm + len, cm->mountpoint + mpm))
> > +				continue;
> > +
> > +			if (!mounts_equal(cm, ct, false)) {
> > +				pr_err("Two shared mounts on same spaces have different mounts\n");
> 
> What's the problem with supporting such? Just MS_MOVE at the end.

I don't understand what do you mean. If you will move something, it will
be propagated to other mounts in this group.

Here is an example how to get two shared mounts with different set of
children.

mount -t tmpfs xxx xxx
mount --make-shared xxx
mkdir xxx/yyy
mount -t tmpfs yyy xxx/yyy
mkdir zzz
mount --bind xxx zzz

In this case the "yyy" mount is not propogated into "zzz".

> 
> > +				pr_err("%d:%s\n", cm->mnt_id, cm->mountpoint);
> > +				pr_err("%d:%s\n", ct->mnt_id, ct->mountpoint);
> > +				return -1;
> > +			}
> > +
> > +			list_move(&cm->siblings, &children);
> >  			break;
> > +		}
> > +		if (&cm->siblings == &m->children) {
> > +			list_splice(&children, &m->children);
> > +
> > +			pr_err("%d:%s and %d:%s have different set of mounts\n",
> > +				m->mnt_id, m->mountpoint, t->mnt_id, t->mountpoint);
> > +			pr_err("For example %d:%s\n", ct->mnt_id, ct->mountpoint);
> > +			return -1;
> > +		}
> 
> In theory, at the end we should support any combination of mountpoints in CRIU,
> which in turn means, that the validation function, in this extreme case, should
> be empty. You say, that we're about to support more cases, but the validation
> routine becomes larger as well.

The difference between supporting everything and nothing is only in two
symbols.

return 0; /* Everything */
return -1; /* Nothing */

In this function we compare a children set for two shared mounts. Before we
compare all children. Now we compare children and take into account root
for the m mount. A conditions becomes more complicated and we need more
code.


> 
> Now the question -- if we support "non-root shared bind-mounts", what this function
> checks? I.e. -- what do we still NOT support and why?
> 
> >  	}
> > -	if (&ct->siblings == &t->children) {
> > -		pr_err("Two shared mounts %d, %d have different sets of children\n",
> > -			m->parent->mnt_id, t->mnt_id);
> > -		pr_err("%d:%s doesn't have a proper point for %d:%s\n",
> > -			t->mnt_id, t->mountpoint,
> > -			m->mnt_id, m->mountpoint);
> > +
> > +	if (!list_empty(&m->children)) {
> > +		list_splice(&children, &m->children);
> > +
> > +		cm = list_first_entry(&m->children, struct mount_info, siblings);
> > +		pr_err("%d:%s and %d:%s have different set of mounts\n",
> > +			m->mnt_id, m->mountpoint, t->mnt_id, t->mountpoint);
> > +		pr_err("For example %d:%s\n", cm->mnt_id, cm->mountpoint);
> >  		return -1;
> >  	}
> > -
> > +	list_splice(&children, &m->children);
> >  	return 0;
> >  }
> >  
> > @@ -406,7 +474,7 @@ static int validate_mounts(struct mount_info *info, bool for_dump)
> >  			/* root mount can be any */
> >  			continue;
> >  
> > -		if (m->parent->shared_id && validate_shared(m))
> > +		if (m->shared_id && validate_shared(m))
> 
> This hunk be explained.
> 
> >  			return -1;
> >  
> >  		/*
> > 
>