[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)
> +			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))

Why do we compare mountpoint with root?

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

is_subpath is to be used here?
Can we have a helper finding the mp by mp->mountpoint? This would
simplify this whole loop and the subsequent if () like this

cm = find_mp_by_mountpoint();
if (!cm || !mounts_equal()) {
	pr_err("Bla-bla-bla");
	return -1;
}

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