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

[Pavel Emelyanov]

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?

> +			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?

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

>  			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;
>  
>  		/*
>