lkml.org 
[lkml]   [2006]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] unshare: Cleanup up the sys_unshare interface before we are committed.
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/
>
>
>

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

\
 
 \ /
  Last update: 2006-03-18 20:57    [from the cache]
©2003-2011 Jasper Spaans