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

From: Janak Desai
Date: Sat Mar 18 2006 - 14:52:29 EST

Eric W. Biederman wrote:

Linus Torvalds <torvalds@xxxxxxxx> 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@xxxxxxxxxxxxxxx
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/