[Devel] [PATCH rh7 v2] ve/devpts: Force devpts mounting to use @newinstance inside VE

[Vladimir Davydov]

On Tue, Jul 21, 2015 at 01:23:51PM +0300, Cyrill Gorcunov wrote:
> Modern systemd based containers (such as fedora-21, centos-7) already
> mounting initial devpts filesystem with @newinstance option but it
> turned out that ubuntu-14 lts doesn't, which makes restore procedure
> to fail because we're using get_exec_env as a namespace mark and the
> kernel mounts new superblock for container internally. This is done
> to isolate devpts between containers but somehow incomplete: @devpts_sb
> should server as a root superblock for VE mount namespace. Fix it
> adjusting first mount if no @devpts_sb has been assigned.
> 
> https://jira.sw.ru/browse/PSBM-34931
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov at virtuozzo.com>
> CC: Andrey Vagin <avagin at virtuozzo.com>
> CC: Vladimir Davydov <vdavydov at virtuozzo.com>
> CC: Konstantin Khorenko <khorenko at virtuozzo.com>
> CC: Pavel Emelyanov <xemul at virtuozzo.com>
> ---
> 
> Andrew, would this be more suitable?
> 
>  fs/devpts/inode.c |   25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> Index: linux-pcs7.git/fs/devpts/inode.c
> ===================================================================
> --- linux-pcs7.git.orig/fs/devpts/inode.c
> +++ linux-pcs7.git/fs/devpts/inode.c
> @@ -449,10 +449,29 @@ static struct dentry *devpts_mount(struc
>  	    (current_user_ns() != &init_user_ns) && !opts.newinstance)
>  		return ERR_PTR(-EINVAL);
>  
> -	if (opts.newinstance)
> +	/*
> +	 * When we mount devpts it is somehow tricky because of ours
> +	 * virtualization for containers sake.
> +	 *
> +	 * 1) If container mounts devpts without @newinstance option
> +	 *    the superblock should point to virtualized one.
> +	 *
> +	 * 2) If container mounts devpts with @newinstance option
> +	 *    the superblock should point into a newly created one.
> +	 *
> +	 * Thus we use @devpts_sb as a global root superblock for
> +	 * each container (including VE0) so if there is no
> +	 * @devpts_sb exist we drop @newinstance option and
> +	 * mount it in context of container's mount namespace,
> +	 * otherwise the old logic remains -- next calls to
> +	 * mount with @newinstance allocate new superblocks.
> +	 */
> +	if (opts.newinstance && get_exec_env()->devpts_sb) {
>  		root = mount_nodev(fs_type, flags, data, devpts_fill_super);
> -	else
> +	} else {
> +		opts.newinstance = 0;
>  		root = mount_ns(fs_type, flags, data, get_exec_env(), devpts_fill_super);
> +	}

This looks like an abuse of mount_ns to me...

May be, we'd better simply revert to what we have in PCS6? I mean
reverting commits 2c27d20125f51 and c77f3df733bfa. Less intrusive and
more understandable if you ask me.

>  
>  	if (IS_ERR(root))
>  		return ERR_CAST(root);
> @@ -464,7 +483,7 @@ static struct dentry *devpts_mount(struc
>  	if (error)
>  		goto out_undo_sget;
>  
> -	if (!opts.newinstance) {
> +	if (!opts.newinstance && !get_exec_env()->devpts_sb) {
>  		atomic_inc(&s->s_active);
>  		get_exec_env()->devpts_sb = s;
>  	}
>