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

[Andrew Vagin]

On Wed, Nov 05, 2014 at 08:52:49PM +0400, Pavel Emelyanov wrote:
> On 10/31/2014 04:12 PM, Andrey Vagin wrote:
> > A problem which is solved in this path is that some children can be
> > unaccessiable (unvisiable) for non-root bind-mounts
> > 
> > root	mount point
> > -------------------
> > /	/a
> > /	/a/x
> > /	/a/x/y
> > /	/a/z
> > /x	/b
> > /	/b/y
> > 
> > /b is a non-root bind-mount of /a
> > /y is visiable to both mounts
> > /z is vidiable only for /a
> > 
> > Before this patch we checked that the set of children is the same for
> > all mount in a shared group. Now we check that a visiable set of mounts
> > is the same for all mounts in a shared group.
> > 
> > Now we take the next mount in the shared group, which is wider or equal
> > to current and compare children between them.
> > 
> > Before this patch validate_shared(m) validates the m->parent mount.
> > Now it validates the "m" mount. So you can find following lines in the
> > patch:
> > -               if (m->parent->shared_id && validate_shared(m))
> > +               if (m->shared_id && validate_shared(m))
> > 
> > We doesn't support shared mounts with different set of children.
> > Here is an example of such case can be created:
> > mount tmpfs a /a
> > mount --make-shared /a
> > mkdir /a/b
> > mount tmpfs b /a/b
> > mount --bind /a /c
> > 
> > In this case /c doesn't have the /b child. To support such cases,
> > we need to sort all shared mounts accoding with a set of children.
> > 
> > Signed-off-by: Andrey Vagin <avagin at openvz.org>
> > ---
> >  mount.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 95 insertions(+), 12 deletions(-)
> > 
> > diff --git a/mount.c b/mount.c
> > index edc69f9..d982e75 100644
> > --- a/mount.c
> > +++ b/mount.c
> > @@ -372,28 +372,111 @@ 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);
> > +
> > +	/*
> > +	 * Check that all mounts in one shared group has the same set of
> > +	 * children. Only visible children are accounted. A non-root bind-mount
> > +	 * doesn't see children out of its root and it's excpected case.
> > +	 *
> > +	 * Here is a few conditions:
> > +	 * 1. t is wider than m
> > +	 * 2. We search a wider mount in the same direction, so when we
> > +	 *    enumirate all mounts, we can't be sure that all of them
> > +	 *    has the same set of children.
> > +	 */
> > +
> > +	/*
> > +	 * Try to find a mount, which is wider or equal.
> > +	 * 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)
> 
> What if t->root is "/foo/abc" and m->root is "/foo/abcd"? This strncmp
> would return 0, but probably t should not even be considered as m's wider
> mount, shouldn't it?

No, it should not. Thanks.

> 
> > +			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.
> > +	 */
> > +	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
> > +	 *			\cm->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");
> > +				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);
> 
> OK, so for each mount in t (widest) we try to find a mount in m (the one being
> validated now), that is a) equal and b) has path visible in t, right?
> 
> If so, two questions:
> 
> a) why the logic is inverted? Shouldn't we scan for m's mounts and find the
>    respective image in t?

Look at the answer on the second question. We want to get an empty
children list at the end.

> 
> b) why tossing the m's children list? Why not just find the respective mount
>    and leave it in?

We need to compare two unsorted lists. We enumirate all elements from
one of them, then find a proper element in a second list and remove it
from there.

At the end we check that a second list is empty. Otherwise it contains
elements, which don't exist in a first list.



> 
> >  			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;
> > +		}
> >  	}
> > -	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 +489,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))
> >  			return -1;
> >  
> >  		/*
> > 
>