Re: [PATCH -mm 7/7] forbid the use of the unshare syscall on ipc namespaces

[Kirill Korotaev <dev@sw.ru>]

This patch looks as an overkill for me.

If you really care about things you describe, you can forbid unsharing in cases:

1.
        undo_list = tsk->sysvsem.undo_list;
        if (undo_list)
                REFUSE_UNSHARE;
2. vma exists with vma->vm_ops == &shm_vm_ops;
3. file opened with f_op == &shm_file_operations

I also dislike exec() operation for such sort of things since you can have no executable
at hands due to changed fs namespace.

Thanks,
Kirill


> This patch forbids the use of the unshare() syscall on ipc namespaces.
> 
> The purpose of this restriction is to protect the system from
> inconsistencies when the namespace is unshared. e.g. shared memory ids
> will be removed but not the memory mappings, semaphore ids will be
> removed but the semundos not cleared.
> 
> 
> Signed-off-by: Cedric Le Goater <clg@fr.ibm.com>
> Cc: Andrew Morton <akpm@osdl.org>
> Cc: Pavel Emelianov <xemul@openvz.org>
> Cc: Kirill Korotaev <dev@openvz.org>
> Cc: Andrey Savochkin <saw@sw.ru>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Herbert Poetzl <herbert@13thfloor.at>
> Cc: Sam Vilain <sam.vilain@catalyst.net.nz>
> Cc: Serge E. Hallyn <serue@us.ibm.com>
> Cc: Dave Hansen <haveblue@us.ibm.com>
> 
> ---
>  kernel/fork.c |   23 +++++------------------
>  1 file changed, 5 insertions(+), 18 deletions(-)
> 
> Index: 2.6.18-rc1-mm1/kernel/fork.c
> ===================================================================
> --- 2.6.18-rc1-mm1.orig/kernel/fork.c
> +++ 2.6.18-rc1-mm1/kernel/fork.c
> @@ -1604,7 +1604,6 @@ asmlinkage long sys_unshare(unsigned lon
>  	struct sem_undo_list *new_ulist = NULL;
>  	struct nsproxy *new_nsproxy = NULL, *old_nsproxy = NULL;
>  	struct uts_namespace *uts, *new_uts = NULL;
> -	struct ipc_namespace *ipc, *new_ipc = NULL;
>  
>  	check_unshare_flags(&unshare_flags);
>  
> @@ -1612,12 +1611,12 @@ asmlinkage long sys_unshare(unsigned lon
>  	err = -EINVAL;
>  	if (unshare_flags & ~(CLONE_THREAD|CLONE_FS|CLONE_NEWNS|CLONE_SIGHAND|
>  				CLONE_VM|CLONE_FILES|CLONE_SYSVSEM|
> -				CLONE_NEWUTS|CLONE_NEWIPC))
> +				CLONE_NEWUTS))
>  		goto bad_unshare_out;
>  
>  	/* Also return -EINVAL for all unsharable namespaces. May be a
>  	 * -EACCES would be more appropriate ? */
> -	if (unshare_flags & CLONE_NEWUSER)
> +	if (unshare_flags & (CLONE_NEWUSER|CLONE_NEWIPC))
>  		goto bad_unshare_out;
>  
>  	if ((err = unshare_thread(unshare_flags)))
> @@ -1636,20 +1635,18 @@ asmlinkage long sys_unshare(unsigned lon
>  		goto bad_unshare_cleanup_fd;
>  	if ((err = unshare_utsname(unshare_flags, &new_uts)))
>  		goto bad_unshare_cleanup_semundo;
> -	if ((err = unshare_ipcs(unshare_flags, &new_ipc)))
> -		goto bad_unshare_cleanup_uts;
>  
> -	if (new_ns || new_uts || new_ipc) {
> +	if (new_ns || new_uts) {
>  		old_nsproxy = current->nsproxy;
>  		new_nsproxy = dup_namespaces(old_nsproxy);
>  		if (!new_nsproxy) {
>  			err = -ENOMEM;
> -			goto bad_unshare_cleanup_ipc;
> +			goto bad_unshare_cleanup_uts;
>  		}
>  	}
>  
>  	if (new_fs || new_ns || new_sigh || new_mm || new_fd || new_ulist ||
> -				new_uts || new_ipc) {
> +				new_uts) {
>  
>  		task_lock(current);
>  
> @@ -1697,22 +1694,12 @@ asmlinkage long sys_unshare(unsigned lon
>  			new_uts = uts;
>  		}
>  
> -		if (new_ipc) {
> -			ipc = current->nsproxy->ipc_ns;
> -			current->nsproxy->ipc_ns = new_ipc;
> -			new_ipc = ipc;
> -		}
> -
>  		task_unlock(current);
>  	}
>  
>  	if (new_nsproxy)
>  		put_nsproxy(new_nsproxy);
>  
> -bad_unshare_cleanup_ipc:
> -	if (new_ipc)
> -		put_ipc_ns(new_ipc);
> -
>  bad_unshare_cleanup_uts:
>  	if (new_uts)
>  		put_uts_ns(new_uts);
> 
> --
>