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

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 <[email protected]>

---

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

f0cdb649b7140927777f4355631648b396ee235b

kernel/fork.c +1 -1

index d2706e9..2f24553 100644
@@ -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;
 		}
 

Signature · 7 lines

1.2.4.g2d33

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

kernel/fork.c asmlinkage long sys_unshare(unsigned lon · line 1578

Quoted 9 lines · Eric W. Biederman

kernel/fork.c +1 -1

@@ -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.

Quoted 7 lines · Oleg Nesterov

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.

Quoted 22 lines · Andrew Morton

Quoted 8 lines · Oleg Nesterov

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.

Quoted 10 lines

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 5 lines · Oleg Nesterov

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.

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?

Quoted 5 lines · 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.

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

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.

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.

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 <[email protected]>

---

kernel/fork.c | 6 ++++++

1 files changed, 6 insertions(+), 0 deletions(-)

46868b4b6ebeb9042dded68a6f6301ffe06820c9

kernel/fork.c +6 -0

index 46060cb..411b10d 100644
@@ -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;

Quoted 16 lines · Oleg Nesterov

kernel/fork.c asmlinkage long sys_unshare(unsigned lon · line 1579

Quoted 10 lines · Eric W. Biederman

kernel/fork.c +1 -1

@@ -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)' ?

Quoted 5 lines

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.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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.

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.

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

Maybe Janak or Linus can comment?

Quoted 5 lines · Andrew Morton

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,

Quoted 7 lines · Linus Torvalds

Quoted 5 lines · Andrew Morton

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

Maybe Janak or Linus can comment?

Quoted 3 lines

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.

Quoted 10 lines · [email protected] (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.

Quoted 8 lines

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