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

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

[Eric W. Biederman]

Since we have not crossed the magic 2.6.16 line can we please
include this patch.  My apologies for catching this so late in the
cycle.

- Error if we are passed any flags we don't expect.

  This preserves forward compatibility so programs that use new flags that
  run on old kernels will fail instead of silently doing the wrong thing.

- Use separate defines from sys_clone.

  sys_unshare can't implement half of the clone flags under any circumstances
  and those that it does implement have subtlely different semantics than
  the clone flags.  Using a different set of flags sets the
  expectation that things will be different.

  Binary compatibility with current users of the is still maintained
  as the unshare flags and the clone flags have the same values.


Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 include/linux/sched.h |   17 +++++++++++++++++
 kernel/fork.c         |   34 +++++++++++++++++++---------------
 2 files changed, 36 insertions(+), 15 deletions(-)

1b9e67b9696f1e828ba789d3f6c8247d1171f367
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 62e6314..8e46f05 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -69,6 +69,23 @@ struct exec_domain;
 #define CLONE_KERNEL	(CLONE_FS | CLONE_FILES | CLONE_SIGHAND)
 
 /*
+ * unsharing flags: (Keep in sync with the clone flags if possible)
+ */
+#define UNSHARE_VM	0x00000100	/* stop sharing the VM between processes */
+#define UNSHARE_FS	0x00000200	/* stop sharing the fs info between processes */
+#define UNSHARE_FILES	0x00000400	/* stop sharing open files between processes */
+#define UNSHARE_SIGHAND	0x00000800	/* stop sharing signal handlers and blocked signals */
+#define UNSHARE_THREAD	0x00010000	/* stop sharing a thread group */
+#define UNSHARE_NS	0x00020000	/* stop sharing the mount namespace */
+#define UNSHARE_SYSVSEM	0x00040000	/* stop sharing system V SEM_UNDO semantics */
+
+/*
+ * Helper so sys_unshare can check if it is passed valid flags.
+ */
+#define UNSHARE_VALID	(UNSHARE_VM|UNSHARE_FS|UNSHARE_FILES|UNSHARE_SIGHAND| \
+				UNSHARE_THREAD|UNSHARE_NS|UNSHARE_SYSVSEM)
+
+/*
  * These are the constant used to fake the fixed-point load-average
  * counting. Some notes:
  *  - 11 bit fractions expand to 22 bits by the multiplies: this gives
diff --git a/kernel/fork.c b/kernel/fork.c
index ccdfbb1..d2706e9 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1382,28 +1382,28 @@ static inline void check_unshare_flags(u
 	 * If unsharing a thread from a thread group, must also
 	 * unshare vm.
 	 */
-	if (*flags_ptr & CLONE_THREAD)
-		*flags_ptr |= CLONE_VM;
+	if (*flags_ptr & UNSHARE_THREAD)
+		*flags_ptr |= UNSHARE_VM;
 
 	/*
 	 * If unsharing vm, must also unshare signal handlers.
 	 */
-	if (*flags_ptr & CLONE_VM)
-		*flags_ptr |= CLONE_SIGHAND;
+	if (*flags_ptr & UNSHARE_VM)
+		*flags_ptr |= UNSHARE_SIGHAND;
 
 	/*
 	 * If unsharing signal handlers and the task was created
 	 * using CLONE_THREAD, then must unshare the thread
 	 */
-	if ((*flags_ptr & CLONE_SIGHAND) &&
+	if ((*flags_ptr & UNSHARE_SIGHAND) &&
 	    (atomic_read(&current->signal->count) > 1))
-		*flags_ptr |= CLONE_THREAD;
+		*flags_ptr |= UNSHARE_THREAD;
 
 	/*
 	 * If unsharing namespace, must also unshare filesystem information.
 	 */
-	if (*flags_ptr & CLONE_NEWNS)
-		*flags_ptr |= CLONE_FS;
+	if (*flags_ptr & UNSHARE_NS)
+		*flags_ptr |= UNSHARE_FS;
 }
 
 /*
@@ -1411,7 +1411,7 @@ static inline void check_unshare_flags(u
  */
 static int unshare_thread(unsigned long unshare_flags)
 {
-	if (unshare_flags & CLONE_THREAD)
+	if (unshare_flags & UNSHARE_THREAD)
 		return -EINVAL;
 
 	return 0;
@@ -1424,7 +1424,7 @@ static int unshare_fs(unsigned long unsh
 {
 	struct fs_struct *fs = current->fs;
 
-	if ((unshare_flags & CLONE_FS) &&
+	if ((unshare_flags & UNSHARE_FS) &&
 	    (fs && atomic_read(&fs->count) > 1)) {
 		*new_fsp = __copy_fs_struct(current->fs);
 		if (!*new_fsp)
@@ -1441,7 +1441,7 @@ static int unshare_namespace(unsigned lo
 {
 	struct namespace *ns = current->namespace;
 
-	if ((unshare_flags & CLONE_NEWNS) &&
+	if ((unshare_flags & UNSHARE_NS) &&
 	    (ns && atomic_read(&ns->count) > 1)) {
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
@@ -1462,7 +1462,7 @@ static int unshare_sighand(unsigned long
 {
 	struct sighand_struct *sigh = current->sighand;
 
-	if ((unshare_flags & CLONE_SIGHAND) &&
+	if ((unshare_flags & UNSHARE_SIGHAND) &&
 	    (sigh && atomic_read(&sigh->count) > 1))
 		return -EINVAL;
 	else
@@ -1476,7 +1476,7 @@ static int unshare_vm(unsigned long unsh
 {
 	struct mm_struct *mm = current->mm;
 
-	if ((unshare_flags & CLONE_VM) &&
+	if ((unshare_flags & UNSHARE_VM) &&
 	    (mm && atomic_read(&mm->mm_users) > 1)) {
 		*new_mmp = dup_mm(current);
 		if (!*new_mmp)
@@ -1494,7 +1494,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 & UNSHARE_FILES) &&
 	    (fd && atomic_read(&fd->count) > 1)) {
 		*new_fdp = dup_fd(fd, &error);
 		if (!*new_fdp)
@@ -1510,7 +1510,7 @@ 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)
+	if (unshare_flags & UNSHARE_SYSVSEM)
 		return -EINVAL;
 
 	return 0;
@@ -1534,6 +1534,10 @@ asmlinkage long sys_unshare(unsigned lon
 	struct files_struct *fd, *new_fd = NULL;
 	struct sem_undo_list *new_ulist = NULL;
 
+	/* Only accept valid unshare flags */
+	if (unshare_flags & ~UNSHARE_VALID)
+		return -EINVAL;
+
 	check_unshare_flags(&unshare_flags);
 
 	if ((err = unshare_thread(unshare_flags)))
-- 
1.2.4.g2d33

[PATCH] unshare: Use rcu_assign_pointer when setting sighand

[Eric W. Biederman]

The sighand pointer only needs the rcu_read_lock on the
read side.  So only depending on task_lock protection
when setting this pointer is not enough.  We also need
a memory barrier to ensure the initialization is seen first.

Use rcu_assign_pointer as it does this for us, and clearly
documents that we are setting an rcu readable pointer.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 kernel/fork.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

f0cdb649b7140927777f4355631648b396ee235b
diff --git a/kernel/fork.c b/kernel/fork.c
index d2706e9..2f24553 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1573,7 +1573,7 @@ asmlinkage long sys_unshare(unsigned lon
 
 		if (new_sigh) {
 			sigh = current->sighand;
-			current->sighand = new_sigh;
+			rcu_assign_pointer(current->sighand, new_sigh);
 			new_sigh = sigh;
 		}
 
-- 
1.2.4.g2d33

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

[Michael Kerrisk]

Eric,

> Since we have not crossed the magic 2.6.16 line can we please
> include this patch.  My apologies for catching this so late in the
> cycle.
> 
> - Error if we are passed any flags we don't expect.
> 
>   This preserves forward compatibility so programs that use new flags 
>   that
>   run on old kernels will fail instead of silently doing the wrong thing.
> 
> - Use separate defines from sys_clone.
> 
>   sys_unshare can't implement half of the clone flags under any
> circumstances
>   and those that it does implement have subtlely different semantics than
>   the clone flags.  Using a different set of flags sets the
>   expectation that things will be different.
> 
>   Binary compatibility with current users of the is still maintained
>   as the unshare flags and the clone flags have the same values.

Thanks for this.  I had begun to think I must simply be dense
for thinking there was a problem here...

I will update my draft manual page accordingly.

Cheers,

Michael

-- 
Michael Kerrisk
maintainer of Linux man pages Sections 2, 3, 4, 5, and 7 

Want to help with man page maintenance?  
Grab the latest tarball at
ftp://ftp.win.tue.nl/pub/linux-local/manpages/, 
read the HOWTOHELP file and grep the source 
files for 'FIXME'.

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

[Andrew Morton]

ebiederm@xmission.com (Eric W. Biederman) wrote:
>
> Since we have not crossed the magic 2.6.16 line can we please
>  include this patch.  My apologies for catching this so late in the
>  cycle.
> 
>  - Error if we are passed any flags we don't expect.
> 
>    This preserves forward compatibility so programs that use new flags that
>    run on old kernels will fail instead of silently doing the wrong thing.

Makes sense.

>  - Use separate defines from sys_clone.
> 
>    sys_unshare can't implement half of the clone flags under any circumstances
>    and those that it does implement have subtlely different semantics than
>    the clone flags.  Using a different set of flags sets the
>    expectation that things will be different.

iirc there was some discussion about this and it was explicitly decided to
keep the CLONE flags.

Maybe Janak or Linus can comment?

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

[Linus Torvalds]

On Thu, 16 Mar 2006, Andrew Morton wrote:
> 
> iirc there was some discussion about this and it was explicitly decided to
> keep the CLONE flags.
> 
> Maybe Janak or Linus can comment?

My personal opinion is that having a different set of flags is more 
confusing and likely to result in problems later than having the same 
ones. Regardless, I'm not touching this for 2.6.16 any more, 

		Linus

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

[Janak Desai]

Andrew Morton wrote:

>ebiederm@xmission.com (Eric W. Biederman) wrote:
>  
>
>>Since we have not crossed the magic 2.6.16 line can we please
>> include this patch.  My apologies for catching this so late in the
>> cycle.
>>
>> - Error if we are passed any flags we don't expect.
>>
>>   This preserves forward compatibility so programs that use new flags that
>>   run on old kernels will fail instead of silently doing the wrong thing.
>>    
>>
>
>Makes sense.
>
>  
>
>> - Use separate defines from sys_clone.
>>
>>   sys_unshare can't implement half of the clone flags under any circumstances
>>   and those that it does implement have subtlely different semantics than
>>   the clone flags.  Using a different set of flags sets the
>>   expectation that things will be different.
>>    
>>
>
>iirc there was some discussion about this and it was explicitly decided to
>keep the CLONE flags.
>
>Maybe Janak or Linus can comment?
>
>  
>
In the two prior discussions on this, the disagreement was on how much 
confusion
(if any) the use of CLONE_* flags would generate. I personally did not 
think that
it was confusing enough to add new flags, with the same values as CLONE_*
flags, in the kernel. Linus's last email (3/1/06) on the subject seemed 
to lean in that
direction as well. That's why I didn't take any action on it.

-Janak

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

[Eric W. Biederman]

Linus Torvalds <torvalds@osdl.org> writes:

> On Thu, 16 Mar 2006, Andrew Morton wrote:
>> 
>> iirc there was some discussion about this and it was explicitly decided to
>> keep the CLONE flags.
>> 
>> Maybe Janak or Linus can comment?
>
> My personal opinion is that having a different set of flags is more 
> confusing and likely to result in problems later than having the same 
> ones. Regardless, I'm not touching this for 2.6.16 any more, 

I am actually a lot more concerned with the fact that we don't test
for invalid bits.  So we have an ABI that will change in the future,
and that doesn't allow us to have a program that runs on old and new
kernels.

I guess I can resend some version of my patch after 2.6.16 is out and
break the ABI for the undefined bits then.  Correct programs shouldn't
care.  But it sure would be nice if they could care.

Eric

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

[Andrew Morton]

ebiederm@xmission.com (Eric W. Biederman) wrote:
>
> Linus Torvalds <torvalds@osdl.org> writes:
> 
> > On Thu, 16 Mar 2006, Andrew Morton wrote:
> >> 
> >> iirc there was some discussion about this and it was explicitly decided to
> >> keep the CLONE flags.
> >> 
> >> Maybe Janak or Linus can comment?
> >
> > My personal opinion is that having a different set of flags is more 
> > confusing and likely to result in problems later than having the same 
> > ones. Regardless, I'm not touching this for 2.6.16 any more, 
> 
> I am actually a lot more concerned with the fact that we don't test
> for invalid bits.  So we have an ABI that will change in the future,
> and that doesn't allow us to have a program that runs on old and new
> kernels.

The risk of breaking things is small - it would require someone to write a
sys_unshare-using app which a) they care about and b) has a particular bug
in it.  But yes, we should check.

> I guess I can resend some version of my patch after 2.6.16 is out and
> break the ABI for the undefined bits then.  Correct programs shouldn't
> care.  But it sure would be nice if they could care.
> 

Your single patch did two different things - there's a lesson here ;)

Re: [PATCH] unshare: Use rcu_assign_pointer when setting sighand

[Paul E. McKenney]

On Thu, Mar 16, 2006 at 10:31:38AM -0700, Eric W. Biederman wrote:
> 
> The sighand pointer only needs the rcu_read_lock on the
> read side.  So only depending on task_lock protection
> when setting this pointer is not enough.  We also need
> a memory barrier to ensure the initialization is seen first.
> 
> Use rcu_assign_pointer as it does this for us, and clearly
> documents that we are setting an rcu readable pointer.

Good catch!

Acked-by: Paul E. McKenney <paulmck@us.ibm.com>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> 
> 
> ---
> 
>  kernel/fork.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> f0cdb649b7140927777f4355631648b396ee235b
> diff --git a/kernel/fork.c b/kernel/fork.c
> index d2706e9..2f24553 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1573,7 +1573,7 @@ asmlinkage long sys_unshare(unsigned lon
>  
>  		if (new_sigh) {
>  			sigh = current->sighand;
> -			current->sighand = new_sigh;
> +			rcu_assign_pointer(current->sighand, new_sigh);
>  			new_sigh = sigh;
>  		}
>  
> -- 
> 1.2.4.g2d33
> 
> -
> 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/
> 

Re: [PATCH] unshare: Use rcu_assign_pointer when setting sighand

[Oleg Nesterov]

"Eric W. Biederman" wrote:
> 
> @@ -1573,7 +1573,7 @@ asmlinkage long sys_unshare(unsigned lon
> 
>                 if (new_sigh) {
>                         sigh = current->sighand;
> -                       current->sighand = new_sigh;
> +                       rcu_assign_pointer(current->sighand, new_sigh);
>                         new_sigh = sigh;
>                 }

Isn't it better to just replace this code with
'BUG_ON(new_sigh != NULL)' ?

It is never executed, but totally broken, afaics.
task_lock() has nothing to do with ->sighand changing.

Oleg.

Re: [PATCH] unshare: Use rcu_assign_pointer when setting sighand

[Andrew Morton]

Oleg Nesterov <oleg@tv-sign.ru> wrote:
>
> "Eric W. Biederman" wrote:
> > 
> > @@ -1573,7 +1573,7 @@ asmlinkage long sys_unshare(unsigned lon
> > 
> >                 if (new_sigh) {
> >                         sigh = current->sighand;
> > -                       current->sighand = new_sigh;
> > +                       rcu_assign_pointer(current->sighand, new_sigh);
> >                         new_sigh = sigh;
> >                 }
> 
> Isn't it better to just replace this code with
> 'BUG_ON(new_sigh != NULL)' ?
> 
> It is never executed, but totally broken, afaics.
> task_lock() has nothing to do with ->sighand changing.
> 

/*
 * Unsharing of sighand for tasks created with CLONE_SIGHAND is not
 * supported yet
 */
static int unshare_sighand(unsigned long unshare_flags, struct sighand_struct **new_sighp)

It's all just a place-holder at present.

If we don't plan on ever supporting unshare(CLONE_SIGHAND) we should take
that code out and make it return EINVAL.  Right now.

And because we don't presently support CLONE_SIGHAND we should return
EINVAL if it's set.  Right now.

And we should change sys_unshare() to reject not-understood flags.  Right
now.

If we don't do these things we'll silently break 2.6.16-back-compatibility
of applications which are coded for future kernels.

Re: [PATCH] unshare: Use rcu_assign_pointer when setting sighand

[Oleg Nesterov]

Andrew Morton wrote:
> 
> Oleg Nesterov <oleg@tv-sign.ru> wrote:
> >
> > Isn't it better to just replace this code with
> > 'BUG_ON(new_sigh != NULL)' ?
> >
> > It is never executed, but totally broken, afaics.
> > task_lock() has nothing to do with ->sighand changing.
> >
> 
> /*
>  * Unsharing of sighand for tasks created with CLONE_SIGHAND is not
>  * supported yet
>  */
> static int unshare_sighand(unsigned long unshare_flags, struct sighand_struct **new_sighp)
> 
> It's all just a place-holder at present.
> 
> If we don't plan on ever supporting unshare(CLONE_SIGHAND) we should take
> that code out and make it return EINVAL.  Right now.
> 
> And because we don't presently support CLONE_SIGHAND we should return
> EINVAL if it's set.  Right now.
> 
> And we should change sys_unshare() to reject not-understood flags.  Right
> now.
> 
> If we don't do these things we'll silently break 2.6.16-back-compatibility
> of applications which are coded for future kernels.

unshare_sighand() is ok, it never populates *new_sighp, it just returns
errror code: 0 when ->sighand is not shared, EINVAL otherwise.

I argued about 'if (new_sigh)' code in sys_unshare() because it lies about
locking rules.

Btw, copy_process() forbids CLONE_SIGHAND without CLONE_VM (is there a
good reason for that?), but one can do unshare(CLONE_VM). This is odd.

Oleg.

[PATCH] implement unshare(CLONE_SIGHAND) for single-thread case

[Oleg Nesterov]

on top of (already merged)
	unshare-use-rcu_assign_pointer-when-setting-sighand.patch
depends on
	convert-sighand_cache-to-use-slab_destroy_by_rcu.patch

Implement unshare(CLONE_SIGHAND) for single-thread case.

I think it is not possible to do it for multithread case without
complication of copy_process().

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- MM/kernel/fork.c~	2006-03-18 16:23:23.000000000 +0300
+++ MM/kernel/fork.c	2006-03-18 17:00:39.000000000 +0300
@@ -1483,18 +1483,24 @@ static int unshare_namespace(unsigned lo
 }
 
 /*
- * Unsharing of sighand for tasks created with CLONE_SIGHAND is not
- * supported yet
+ * Unsharing of sighand is only supported for single-thread applications
  */
 static int unshare_sighand(unsigned long unshare_flags, struct sighand_struct **new_sighp)
 {
-	struct sighand_struct *sigh = current->sighand;
-
 	if ((unshare_flags & CLONE_SIGHAND) &&
-	    (sigh && atomic_read(&sigh->count) > 1))
-		return -EINVAL;
-	else
-		return 0;
+			atomic_read(&current->sighand->count) > 1) {
+
+		if (!thread_group_empty(current))
+			return -EINVAL;
+
+		*new_sighp = kmem_cache_alloc(sighand_cachep, GFP_KERNEL);
+		if (unlikely(*new_sighp == NULL))
+			return -ENOMEM;
+
+		atomic_set(&(*new_sighp)->count, 1);
+	}
+
+	return 0;
 }
 
 /*
@@ -1579,7 +1585,18 @@ asmlinkage long sys_unshare(unsigned lon
 	if ((err = unshare_semundo(unshare_flags, &new_ulist)))
 		goto bad_unshare_cleanup_fd;
 
-	if (new_fs || new_ns || new_sigh || new_mm || new_fd || new_ulist) {
+	if (new_sigh) {
+		sigh = current->sighand;
+		write_lock_irq(&tasklist_lock);
+		spin_lock(&sigh->siglock);
+		memcpy(new_sigh->action, sigh->action, sizeof(sigh->action));
+		rcu_assign_pointer(current->sighand, new_sigh);
+		spin_unlock(&sigh->siglock);
+		write_unlock_irq(&tasklist_lock);
+		new_sigh = sigh;
+	}
+
+	if (new_fs || new_ns || new_mm || new_fd || new_ulist) {
 
 		task_lock(current);
 
@@ -1595,12 +1612,6 @@ asmlinkage long sys_unshare(unsigned lon
 			new_ns = ns;
 		}
 
-		if (new_sigh) {
-			sigh = current->sighand;
-			rcu_assign_pointer(current->sighand, new_sigh);
-			new_sigh = sigh;
-		}
-
 		if (new_mm) {
 			mm = current->mm;
 			active_mm = current->active_mm;

[PATCH] unshare: Error if passed unsupported flags

[Eric W. Biederman]

This patch does a bare bones trivial patch to ensure we always
get -EINVAL on the unsupported cases for sys_unshare.  If this
goes in before 2.6.16 it allows us to forward compatible with
future applications using sys_unshare.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>


---

 kernel/fork.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

46868b4b6ebeb9042dded68a6f6301ffe06820c9
diff --git a/kernel/fork.c b/kernel/fork.c
index 46060cb..411b10d 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1535,6 +1535,12 @@ asmlinkage long sys_unshare(unsigned lon
 	struct sem_undo_list *new_ulist = NULL;
 
 	check_unshare_flags(&unshare_flags);
+       
+	/* Return -EINVAL for all unsupported flags */
+	err = -EINVAL;
+	if (unshare_flags & ~(CLONE_THREAD|CLONE_FS|CLONE_NEWNS|CLONE_SIGHAND|
+				CLONE_VM|CLONE_FILES|CLONE_SYSVSEM))
+		goto bad_unshare_out;
 
 	if ((err = unshare_thread(unshare_flags)))
 		goto bad_unshare_out;
-- 
1.2.4.g2d33

Re: [PATCH] unshare: Error if passed unsupported flags

[Janak Desai]

Thanks Eric. I had just started to do this based on Andrew's request.

-Janak

Eric W. Biederman wrote:

>This patch does a bare bones trivial patch to ensure we always
>get -EINVAL on the unsupported cases for sys_unshare.  If this
>goes in before 2.6.16 it allows us to forward compatible with
>future applications using sys_unshare.
>
>Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
>
>
>---
>
> kernel/fork.c |    6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
>46868b4b6ebeb9042dded68a6f6301ffe06820c9
>diff --git a/kernel/fork.c b/kernel/fork.c
>index 46060cb..411b10d 100644
>--- a/kernel/fork.c
>+++ b/kernel/fork.c
>@@ -1535,6 +1535,12 @@ asmlinkage long sys_unshare(unsigned lon
> 	struct sem_undo_list *new_ulist = NULL;
> 
> 	check_unshare_flags(&unshare_flags);
>+       
>+	/* Return -EINVAL for all unsupported flags */
>+	err = -EINVAL;
>+	if (unshare_flags & ~(CLONE_THREAD|CLONE_FS|CLONE_NEWNS|CLONE_SIGHAND|
>+				CLONE_VM|CLONE_FILES|CLONE_SYSVSEM))
>+		goto bad_unshare_out;
> 
> 	if ((err = unshare_thread(unshare_flags)))
> 		goto bad_unshare_out;
>  
>

Re: [PATCH] unshare: Use rcu_assign_pointer when setting sighand

[Janak Desai]

Oleg Nesterov wrote:

>Andrew Morton wrote:
>  
>
>>Oleg Nesterov <oleg@tv-sign.ru> wrote:
>>    
>>
>>>Isn't it better to just replace this code with
>>>'BUG_ON(new_sigh != NULL)' ?
>>>
>>>It is never executed, but totally broken, afaics.
>>>task_lock() has nothing to do with ->sighand changing.
>>>
>>>      
>>>
>>/*
>> * Unsharing of sighand for tasks created with CLONE_SIGHAND is not
>> * supported yet
>> */
>>static int unshare_sighand(unsigned long unshare_flags, struct sighand_struct **new_sighp)
>>
>>It's all just a place-holder at present.
>>
>>If we don't plan on ever supporting unshare(CLONE_SIGHAND) we should take
>>that code out and make it return EINVAL.  Right now.
>>
>>And because we don't presently support CLONE_SIGHAND we should return
>>EINVAL if it's set.  Right now.
>>
>>And we should change sys_unshare() to reject not-understood flags.  Right
>>now.
>>
>>If we don't do these things we'll silently break 2.6.16-back-compatibility
>>of applications which are coded for future kernels.
>>    
>>
>
>unshare_sighand() is ok, it never populates *new_sighp, it just returns
>errror code: 0 when ->sighand is not shared, EINVAL otherwise.
>
>I argued about 'if (new_sigh)' code in sys_unshare() because it lies about
>locking rules.
>
>Btw, copy_process() forbids CLONE_SIGHAND without CLONE_VM (is there a
>good reason for that?), but one can do unshare(CLONE_VM). This is odd.
>  
>
Yes, copy_process forbids cloning of signal handlers without cloning of vm.
However, it does allow cloning of vm without cloning of signal handlers. For
those processes, that are sharing vm but not signal handlers, unsharing 
of vm
is allowed.

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

Re: [PATCH] unshare: Use rcu_assign_pointer when setting sighand

[Janak Desai]

Andrew Morton wrote:

>Oleg Nesterov <oleg@tv-sign.ru> wrote:
>  
>
>>"Eric W. Biederman" wrote:
>>    
>>
>>>@@ -1573,7 +1573,7 @@ asmlinkage long sys_unshare(unsigned lon
>>>
>>>                if (new_sigh) {
>>>                        sigh = current->sighand;
>>>-                       current->sighand = new_sigh;
>>>+                       rcu_assign_pointer(current->sighand, new_sigh);
>>>                        new_sigh = sigh;
>>>                }
>>>      
>>>
>>Isn't it better to just replace this code with
>>'BUG_ON(new_sigh != NULL)' ?
>>
>>It is never executed, but totally broken, afaics.
>>task_lock() has nothing to do with ->sighand changing.
>>
>>    
>>
>
>/*
> * Unsharing of sighand for tasks created with CLONE_SIGHAND is not
> * supported yet
> */
>static int unshare_sighand(unsigned long unshare_flags, struct sighand_struct **new_sighp)
>
>It's all just a place-holder at present.
>
>If we don't plan on ever supporting unshare(CLONE_SIGHAND) we should take
>that code out and make it return EINVAL.  Right now.
>
>And because we don't presently support CLONE_SIGHAND we should return
>EINVAL if it's set.  Right now.
>  
>
unshare does return EINVAL if signal handler unsharing is attempted by a
process that is currently sharing its signal handler with another 
process. However,
because of the interaction between signal handlers and vm, CLONE_SIGHAND
is set anytime vm unsharing is attempted. That is, if you are trying to 
unshare
vm, you must unshare signal handlers. But if they are not being shared 
in the
first place, there is no need to prevent unsharing of vm.

Therefore, unshare returns EINVAL if unsharing of signal handlers and/or vm
is attempted by a process that is sharing its signal handler.

>And we should change sys_unshare() to reject not-understood flags.  Right
>now.
>  
>
Eric just posted a patch to do this.

>If we don't do these things we'll silently break 2.6.16-back-compatibility
>of applications which are coded for future kernels.
>
>-
>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/
>
>  
>

Re: [PATCH] unshare: Use rcu_assign_pointer when setting sighand

[Oleg Nesterov]

Janak Desai wrote:
> 
> Oleg Nesterov wrote:
> >
> >Btw, copy_process() forbids CLONE_SIGHAND without CLONE_VM (is there a
> >good reason for that?), but one can do unshare(CLONE_VM). This is odd.
> >
> >
> Yes, copy_process forbids cloning of signal handlers without cloning of vm.
> However, it does allow cloning of vm without cloning of signal handlers. For
> those processes, that are sharing vm but not signal handlers, unsharing
> of vm
> is allowed.

Yes, I was wrong, I missed check_unshare_flags(),

	/*
	 * If unsharing vm, must also unshare signal handlers.
	 */
	if (*flags_ptr & CLONE_VM)
		*flags_ptr |= CLONE_SIGHAND;

Looking below,

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

Shouldn't this be:

		*flags_ptr |= (CLONE_THREAD | CLONE_VM)

? Currently it doesn't matter, but still.


However, I stronly beleive unshare(CLONE_VM) is buggy.

sys_unshare:


		if (new_mm) {
			...
			new_mm = mm;
		}

	...

	bad_unshare_cleanup_vm:
		if (new_mm)
			mmput(new_mm);


mmput() ignores mm->core_waiters.

No?

Oleg.

[PATCH] for 2.6.16, disable unshare_vm()

[Oleg Nesterov]

sys_unshare() does mmput(new_mm). This is not enough
if we have mm->core_waiters. This patch is a temporary
fix for soon to be released 2.6.16.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- MM/kernel/fork.c~	2006-03-18 23:15:13.000000000 +0300
+++ MM/kernel/fork.c	2006-03-18 23:15:56.000000000 +0300
@@ -1512,9 +1512,7 @@ static int unshare_vm(unsigned long unsh
 
 	if ((unshare_flags & CLONE_VM) &&
 	    (mm && atomic_read(&mm->mm_users) > 1)) {
-		*new_mmp = dup_mm(current);
-		if (!*new_mmp)
-			return -ENOMEM;
+		return -EINVAL;
 	}
 
 	return 0;

Re: [PATCH] for 2.6.16, disable unshare_vm()

[Linus Torvalds]

On Sat, 18 Mar 2006, Oleg Nesterov wrote:
>
> sys_unshare() does mmput(new_mm). This is not enough
> if we have mm->core_waiters. This patch is a temporary
> fix for soon to be released 2.6.16.

Yes. Quick raising of hands: is there anybody out there that expects to 
use unshare(CLONE_VM) right now? One of the reasons it was integrated was 
that I thought glibc wanted it for distros. Is disabling the CLONE_VM 
unsharing going to impact that?

		Linus

Re: [PATCH] for 2.6.16, disable unshare_vm()

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

Linus Torvalds wrote:
> Yes. Quick raising of hands: is there anybody out there that expects to 
> use unshare(CLONE_VM) right now? One of the reasons it was integrated was 
> that I thought glibc wanted it for distros. Is disabling the CLONE_VM 
> unsharing going to impact that?

I'm not planning to use unshare(CLONE_VM).  It's not needed for any
functionality planned so far.  What we (as in Red Hat) need unshare()
for now is the filesystem side.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 253 bytes --]

Re: [PATCH] for 2.6.16, disable unshare_vm()

[Janak Desai]

Linus Torvalds wrote:

>On Sat, 18 Mar 2006, Oleg Nesterov wrote:
>  
>
>>sys_unshare() does mmput(new_mm). This is not enough
>>if we have mm->core_waiters. This patch is a temporary
>>fix for soon to be released 2.6.16.
>>    
>>
>
>Yes. Quick raising of hands: is there anybody out there that expects to 
>use unshare(CLONE_VM) right now? One of the reasons it was integrated was 
>that I thought glibc wanted it for distros. Is disabling the CLONE_VM 
>unsharing going to impact that?
>
>		Linus
>
>  
>
We are using namespace and filesystem unsharing for our common criteria
certification work and would not be impacted by disabling of vm unsharing.

-Janak

unshare(CLONE_VM) Re: [PATCH] unshare: Use rcu_assign_pointer when setting sighand

[Julian Smith]

On Sat, 18 Mar 2006 20:24:51 +0300
Oleg Nesterov <oleg@tv-sign.ru> wrote:

[...]

> However, I stronly beleive unshare(CLONE_VM) is buggy.
> 
> sys_unshare:
> 
> 
> 		if (new_mm) {
> 			...
> 			new_mm = mm;
> 		}
> 
> 	...
> 
> 	bad_unshare_cleanup_vm:
> 		if (new_mm)
> 			mmput(new_mm);
> 
> 
> mmput() ignores mm->core_waiters.

Apologies for re-opening a ten-year-old thread.

I'm looking into whether it would be possible to extend the unshare
syscall to support the CLONE_VM flag with multi-threaded processes,
because this would allow us at Undo to record multi-threaded user
processes much more efficiently than at present.

We currently have to serialise threads and so suffer an N-times
slowdown when recording a process with N cpu-bound threads. But if we
could get per-thread memory permissions with unshare(CLONE_VM), we'd be
able record a multi-threaded process with almost no per-thread
slowdown.

When the unshare syscall was introduced, it seems that the
mm->core_waiters issue was the only thing that prevented CLONE_VM being
supported. Is that right, or were there other problems too?

Many thanks for any information about this.

- Julian

-- 
http://undo-software.com

Re: unshare(CLONE_VM) Re: [PATCH] unshare: Use rcu_assign_pointer when setting sighand

[Linus Torvalds]

On Mon, Mar 14, 2016 at 6:15 AM, Julian Smith <jsmith@undo-software.com> wrote:
>
> I'm looking into whether it would be possible to extend the unshare
> syscall to support the CLONE_VM flag with multi-threaded processes,
> because this would allow us at Undo to record multi-threaded user
> processes much more efficiently than at present.

At the point where you want to unsahe the VM, you should just use a
full clone() instead.

The thing is, unsharing the VM absolutely _requires_ you to also
unshare signals and some other state too (we require that thread
groups are in the same VM, for example, but also the child tid
information etc etc).

And the whole "copy VM" case is so expensive that at that point
there's no advantage to "unshare", you might as well just do a full
clone() (perhaps still sharing file descriptors and fs state).

So while I think a

    unshare(CLONE_VM | CLONE_SIGHAND | CLONE_THREAD |
CLONE_CHILD_CLEARTID | CLONE_CHILD_SETTID);

might be possible from a technical standpoint, I'm not seeing the huge
advantage to users vs just doing something like

    clone(new_vm_function, NULL, CLONE_VFORK | CLONE_FILES | CLONE_FS
| CLONE_PARENT..);
     _exit();

(fixup details to actually work - the above is meant more as a
"something remotely like this" rather than actually equivalent)

The costs of forking and exiting a thread are almost all about just
the VM copying and tear-down, so a "unshare(CLONE_VM)" is
fundamentally not a cheap operation (and at the other range of the
spectrum: an exit of a thread where there are other sharing threads is
fundamentally quite cheap, because it just ends up decrementing
counters).

So my gut feel is that no, we really don't want unshare(CLONE_VM),
because it *is* a very complicated operation and doesn't actually
perform any better than just cloning.

And the "it is a very complicated operation" really comes not from the
fact that we can't copy the VM - we have that support already, but
because CLONE_VM really does go hand-in-hand with so many special
cases. Oleg pointed out that mm->core_waiters thing last time around,
but that just ends up being a detail: the whole VM sharing just ends
up being very central to a lot of small details..

               Linus