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

[Pavel Emelyanov]

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?

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

> +	 */
> +	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.

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

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