Re: [PATCH] unshare: Cleanup up the sys_unshare interface before we are committed.

[Janak Desai <janak@us.ibm.com>]

Eric W. Biederman wrote:

>Linus Torvalds <torvalds@osdl.org> writes:
>
>  
>
>>On Fri, 17 Mar 2006, Michael Kerrisk wrote:
>>    
>>
>>>> - it's all the same issues that clone() has
>>>>        
>>>>
>>>At the moment, but possibly not in the future (if one day
>>>usnhare() needs a flag that has no analogue in clone()).
>>>      
>>>
>>I don't believe that.
>>
>>If we have something we might want to unshare, that implies by definition 
>>that it was something we wanted to conditionally share in the first place.
>>
>>IOW, it ends up being something that would be a clone() flag.
>>
>>So I really do believe that there is a fundamental 1:1 between the flags. 
>>They aren't just "similar". They are very fundamentally about the same 
>>thing, and giving two different names to the same thing is CONFUSING.
>>
>>		Linus
>>    
>>
>
>Was there ever a good reason why we decided to flip the sense of
>the bits?
>
>I put a together a patch to see what the code would look like:
>
>- We actually can reuse between clone and unshare.
>- We don't need the confusing case of when to add additional resources
>  to unshare.
>- There is less total code.
>- We don't confuse users and developers about the inverted values of
>  the clone bits.
>  
>
I guess confusion is subjective. With this patch if I want to unshare 
files and
leave the rest as is, I would have to call

unshare(CLONE_VM | CLONE_FS | CLONE_SIGHAND | ...)

That is, to unshare one type of context, I have to remember and use flags
corresponding to all other contexts. If I forget to include one of them, I
might unwittingly unshare it. Unless I am reading the patch incorrectly,
this to me is more confusing than the current scheme.

-Janak

> kernel/fork.c |  105 ++++++++++++++++++++++++---------------------------------
> 1 files changed, 45 insertions(+), 60 deletions(-)
>
>28e4502c6d2ca48e0b4a08581123b2c3cf94454e
>diff --git a/kernel/fork.c b/kernel/fork.c
>index 411b10d..0eb0a37 100644
>--- a/kernel/fork.c
>+++ b/kernel/fork.c
>@@ -900,6 +900,36 @@ asmlinkage long sys_set_tid_address(int 
> }
> 
> /*
>+ * Check constraints on flags passed to the clone or unshare system call.
>+ */
>+static int check_clone_flags(unsigned long clone_flags)
>+{
>+	int err = -EINVAL;
>+	if ((clone_flags & (CLONE_NEWNS|CLONE_FS)) == (CLONE_NEWNS|CLONE_FS))
>+		goto out;
>+
>+	/*
>+	 * Thread groups must share signals as well, and detached threads
>+	 * can only be started up within the thread group.
>+	 */
>+	if ((clone_flags & CLONE_THREAD) && !(clone_flags & CLONE_SIGHAND))
>+		goto out;
>+
>+	/*
>+	 * Shared signal handlers imply shared VM. By way of the above,
>+	 * thread groups also imply shared VM. Blocking this case allows
>+	 * for various simplifications in other code.
>+	 */
>+	if ((clone_flags & CLONE_SIGHAND) && !(clone_flags & CLONE_VM))
>+		goto out;
>+	
>+	/* We made it here without problems */
>+	err = 0;
>+out:	
>+	return err;
>+}
>+
>+/*
>  * This creates a new process as a copy of the old one,
>  * but does not actually start it yet.
>  *
>@@ -918,23 +948,9 @@ static task_t *copy_process(unsigned lon
> 	int retval;
> 	struct task_struct *p = NULL;
> 
>-	if ((clone_flags & (CLONE_NEWNS|CLONE_FS)) == (CLONE_NEWNS|CLONE_FS))
>-		return ERR_PTR(-EINVAL);
>-
>-	/*
>-	 * Thread groups must share signals as well, and detached threads
>-	 * can only be started up within the thread group.
>-	 */
>-	if ((clone_flags & CLONE_THREAD) && !(clone_flags & CLONE_SIGHAND))
>-		return ERR_PTR(-EINVAL);
>-
>-	/*
>-	 * Shared signal handlers imply shared VM. By way of the above,
>-	 * thread groups also imply shared VM. Blocking this case allows
>-	 * for various simplifications in other code.
>-	 */
>-	if ((clone_flags & CLONE_SIGHAND) && !(clone_flags & CLONE_VM))
>-		return ERR_PTR(-EINVAL);
>+	retval = check_clone_flags(clone_flags);
>+	if (retval)
>+		return ERR_PTR(retval);
> 
> 	retval = security_task_create(clone_flags);
> 	if (retval)
>@@ -1371,47 +1387,12 @@ void __init proc_caches_init(void)
> 			SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL, NULL);
> }
> 
>-
>-/*
>- * Check constraints on flags passed to the unshare system call and
>- * force unsharing of additional process context as appropriate.
>- */
>-static inline void check_unshare_flags(unsigned long *flags_ptr)
>-{
>-	/*
>-	 * If unsharing a thread from a thread group, must also
>-	 * unshare vm.
>-	 */
>-	if (*flags_ptr & CLONE_THREAD)
>-		*flags_ptr |= CLONE_VM;
>-
>-	/*
>-	 * If unsharing vm, must also unshare signal handlers.
>-	 */
>-	if (*flags_ptr & CLONE_VM)
>-		*flags_ptr |= CLONE_SIGHAND;
>-
>-	/*
>-	 * If unsharing signal handlers and the task was created
>-	 * using CLONE_THREAD, then must unshare the thread
>-	 */
>-	if ((*flags_ptr & CLONE_SIGHAND) &&
>-	    (atomic_read(&current->signal->count) > 1))
>-		*flags_ptr |= CLONE_THREAD;
>-
>-	/*
>-	 * If unsharing namespace, must also unshare filesystem information.
>-	 */
>-	if (*flags_ptr & CLONE_NEWNS)
>-		*flags_ptr |= CLONE_FS;
>-}
>-
> /*
>  * Unsharing of tasks created with CLONE_THREAD is not supported yet
>  */
> static int unshare_thread(unsigned long unshare_flags)
> {
>-	if (unshare_flags & CLONE_THREAD)
>+	if (!(unshare_flags & CLONE_THREAD) && !thread_group_empty(current))
> 		return -EINVAL;
> 
> 	return 0;
>@@ -1424,7 +1405,7 @@ static int unshare_fs(unsigned long unsh
> {
> 	struct fs_struct *fs = current->fs;
> 
>-	if ((unshare_flags & CLONE_FS) &&
>+	if (!(unshare_flags & CLONE_FS) &&
> 	    (fs && atomic_read(&fs->count) > 1)) {
> 		*new_fsp = __copy_fs_struct(current->fs);
> 		if (!*new_fsp)
>@@ -1441,7 +1422,7 @@ static int unshare_namespace(unsigned lo
> {
> 	struct namespace *ns = current->namespace;
> 
>-	if ((unshare_flags & CLONE_NEWNS) &&
>+	if (!(unshare_flags & CLONE_NEWNS) &&
> 	    (ns && atomic_read(&ns->count) > 1)) {
> 		if (!capable(CAP_SYS_ADMIN))
> 			return -EPERM;
>@@ -1462,7 +1443,7 @@ static int unshare_sighand(unsigned long
> {
> 	struct sighand_struct *sigh = current->sighand;
> 
>-	if ((unshare_flags & CLONE_SIGHAND) &&
>+	if (!(unshare_flags & CLONE_SIGHAND) &&
> 	    (sigh && atomic_read(&sigh->count) > 1))
> 		return -EINVAL;
> 	else
>@@ -1476,7 +1457,7 @@ static int unshare_vm(unsigned long unsh
> {
> 	struct mm_struct *mm = current->mm;
> 
>-	if ((unshare_flags & CLONE_VM) &&
>+	if (!(unshare_flags & CLONE_VM) &&
> 	    (mm && atomic_read(&mm->mm_users) > 1)) {
> 		*new_mmp = dup_mm(current);
> 		if (!*new_mmp)
>@@ -1494,7 +1475,7 @@ static int unshare_fd(unsigned long unsh
> 	struct files_struct *fd = current->files;
> 	int error = 0;
> 
>-	if ((unshare_flags & CLONE_FILES) &&
>+	if (!(unshare_flags & CLONE_FILES) &&
> 	    (fd && atomic_read(&fd->count) > 1)) {
> 		*new_fdp = dup_fd(fd, &error);
> 		if (!*new_fdp)
>@@ -1510,7 +1491,9 @@ static int unshare_fd(unsigned long unsh
>  */
> static int unshare_semundo(unsigned long unshare_flags, struct sem_undo_list **new_ulistp)
> {
>-	if (unshare_flags & CLONE_SYSVSEM)
>+	struct sem_undo_list *undo_list = current->sysvsem.undo_list;
>+	if (!(unshare_flags & CLONE_SYSVSEM) && 
>+	    undo_list && (atomic_read(&undo_list->refcnt) > 1))
> 		return -EINVAL;
> 
> 	return 0;
>@@ -1534,7 +1517,9 @@ asmlinkage long sys_unshare(unsigned lon
> 	struct files_struct *fd, *new_fd = NULL;
> 	struct sem_undo_list *new_ulist = NULL;
> 
>-	check_unshare_flags(&unshare_flags);
>+	err = check_clone_flags(unshare_flags);
>+	if (err)
>+		goto bad_unshare_out;
>        
> 	/* Return -EINVAL for all unsupported flags */
> 	err = -EINVAL;
>
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>
>  
>