[Devel] [PATCH rh7 v2] ve/devpts: Support per-VE mount namespace

[Vladimir Davydov]

On Tue, Jul 21, 2015 at 08:23:10PM +0300, Cyrill Gorcunov wrote:
[...]
> --- linux-pcs7.git.orig/fs/devpts/inode.c
> +++ linux-pcs7.git/fs/devpts/inode.c
> @@ -24,7 +24,10 @@
>  #include <linux/parser.h>
>  #include <linux/fsnotify.h>
>  #include <linux/seq_file.h>
> +
> +#ifdef CONFIG_VE
>  #include <linux/ve.h>
> +#endif

Why?

>  
>  #define DEVPTS_DEFAULT_MODE 0600
>  /*
[...]
> @@ -436,43 +451,71 @@ static struct dentry *devpts_mount(struc
>  	int error;
>  	struct pts_mount_opts opts;
>  	struct super_block *s;
> -	struct dentry *root;
>  
>  	error = parse_mount_options(data, PARSE_MOUNT, &opts);
>  	if (error)
>  		return ERR_PTR(error);
>  
> +#ifndef CONFIG_VE
>  	/* Require newinstance for all user namespace mounts to ensure
>  	 * the mount options are not changed.
>  	 */
> -	if (!IS_ENABLED(CONFIG_VE) &&
> -	    (current_user_ns() != &init_user_ns) && !opts.newinstance)
> +	if ((current_user_ns() != &init_user_ns) && !opts.newinstance)

FWIW, once we switch to userns, which is going to be pretty soon I hope,
we have to tweak this again.

>  		return ERR_PTR(-EINVAL);
> +#endif
>  
> +#ifdef CONFIG_VE
> +	/*
> +	 * Each container has to have own devpts superblock for isolation
> +	 * sake but it makes a bad joke for us: in CRIU we test if devpts
> +	 * device in container is the same as on the node, to figure out
> +	 * if @newinstance option has to be passed (simply because in
> +	 * vanilla kernel there is no such devpts virtualization) on
> +	 * the restore. Thus every time we're restoring container
> +	 * we pass @newinstance option even if container has been
> +	 * started without this option initially.
> +	 *
> +	 * To workaround this situation here is an ugly hack: first
> +	 * mount of devpts inside container always runs without
> +	 * @newinstance option providing back virtualized superblock.
> +	 * The next mounts inside container go in a regular way.
> +	 *
> +	 * Note @devpts_once is always set for node. And be careful
> +	 * about @else branch below.
> +	 */
> +	if (!get_exec_env()->devpts_once && get_exec_env()->_devpts_mnt)
> +		s = sget(fs_type, compare_init_pts_sb, set_anon_super, flags, NULL);
> +	else
> +#endif

If we initialize ve->devpts_sb lazily (as we do now), we don't need this
hunk as well as devpts_once flag, do we? This would look cleaner IMO.

>  	if (opts.newinstance)
> -		root = mount_nodev(fs_type, flags, data, devpts_fill_super);
> +		s = sget(fs_type, NULL, set_anon_super, flags, NULL);
>  	else
> -		root = mount_ns(fs_type, flags, data, get_exec_env(), devpts_fill_super);
> +		s = sget(fs_type, compare_init_pts_sb, set_anon_super, flags,
> +			 NULL);
>  
> -	if (IS_ERR(root))
> -		return ERR_CAST(root);
> +	if (IS_ERR(s))
> +		return ERR_CAST(s);
> +
> +	if (!s->s_root) {
> +		error = devpts_fill_super(s, data, flags & MS_SILENT ? 1 : 0);
> +		if (error)
> +			goto out_undo_sget;
> +		s->s_flags |= MS_ACTIVE;
> +	}
>  
> -	s = root->d_sb;
>  	memcpy(&(DEVPTS_SB(s))->mount_opts, &opts, sizeof(opts));
>  
>  	error = mknod_ptmx(s);
>  	if (error)
>  		goto out_undo_sget;
>  
> -	if (!opts.newinstance) {
> -		atomic_inc(&s->s_active);
> -		get_exec_env()->devpts_sb = s;
> -	}
> -
> -	return root;
> +#ifdef CONFIG_VE
> +	if (!get_exec_env()->devpts_once && get_exec_env()->_devpts_mnt)
> +		get_exec_env()->devpts_once = true;
> +#endif
> +	return dget(s->s_root);
>  
>  out_undo_sget:
> -	dput(root);
>  	deactivate_locked_super(s);
>  	return ERR_PTR(error);
>  }
[...]