Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1612711 > unrolled thread

Re: [RFC][PATCH] exec: Don't wait for ptraced threads to be reaped.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCH] exec: Don't wait for ptraced threads to be reaped. ebiederm@xmission.com (Eric W. Biederman) - 2017-03-30 10:20 +0200
    [RFC][PATCH 0/2] exec: Fixing ptrace'd mulit-threaded hang ebiederm@xmission.com (Eric W. Biederman) - 2017-04-01 07:20 +0200
      [RFC][PATCH 1/2] sighand: Count each thread group once in sighand_struct ebiederm@xmission.com (Eric W. Biederman) - 2017-04-01 07:20 +0200
      [RFC][PATCH 2/2] exec: If possible don't wait for ptraced threads to be reaped ebiederm@xmission.com (Eric W. Biederman) - 2017-04-01 07:30 +0200
        Re: [RFC][PATCH 2/2] exec: If possible don't wait for ptraced  threads to be reaped Oleg Nesterov <oleg@redhat.com> - 2017-04-02 17:40 +0200
          Re: [RFC][PATCH 2/2] exec: If possible don't wait for ptraced threads to be reaped ebiederm@xmission.com (Eric W. Biederman) - 2017-04-02 21:00 +0200
            Re: [RFC][PATCH 2/2] exec: If possible don't wait for ptraced  threads to be reaped Oleg Nesterov <oleg@redhat.com> - 2017-04-03 20:20 +0200
              Re: [RFC][PATCH 2/2] exec: If possible don't wait for ptraced threads to be reaped ebiederm@xmission.com (Eric W. Biederman) - 2017-04-03 23:20 +0200
                Re: [RFC][PATCH 2/2] exec: If possible don't wait for ptraced         threads to be reaped Oleg Nesterov <oleg@redhat.com> - 2017-04-05 18:50 +0200
      Re: [RFC][PATCH 0/2] exec: Fixing ptrace'd mulit-threaded hang Oleg Nesterov <oleg@redhat.com> - 2017-04-02 17:40 +0200
      [RFC][PATCH v2 4/5] exec: If possible don't wait for ptraced threads to be reaped ebiederm@xmission.com (Eric W. Biederman) - 2017-04-03 01:00 +0200
        Re: [RFC][PATCH v2 4/5] exec: If possible don't wait for ptraced         threads to be reaped Oleg Nesterov <oleg@redhat.com> - 2017-04-05 18:20 +0200
      [RFC][PATCH v2 1/5] ptrace: Don't wait in PTRACE_O_TRACEEXIT for exec or coredump ebiederm@xmission.com (Eric W. Biederman) - 2017-04-03 01:00 +0200
        Re: [RFC][PATCH v2 1/5] ptrace: Don't wait in PTRACE_O_TRACEEXIT         for exec or coredump Oleg Nesterov <oleg@redhat.com> - 2017-04-05 18:30 +0200
      [RFC][PATCH v2 0/5] exec: Fixing ptrace'd mulit-threaded hang ebiederm@xmission.com (Eric W. Biederman) - 2017-04-03 01:00 +0200
        [RFC][PATCH v2 3/5] clone: Disallown CLONE_THREAD with a shared sighand_struct ebiederm@xmission.com (Eric W. Biederman) - 2017-04-03 01:00 +0200
          Re: [RFC][PATCH v2 3/5] clone: Disallown CLONE_THREAD with a         shared sighand_struct Oleg Nesterov <oleg@redhat.com> - 2017-04-05 18:30 +0200
            Re: [RFC][PATCH v2 3/5] clone: Disallown CLONE_THREAD with a shared sighand_struct ebiederm@xmission.com (Eric W. Biederman) - 2017-04-05 19:50 +0200
              Re: [RFC][PATCH v2 3/5] clone: Disallown CLONE_THREAD with a         shared sighand_struct Oleg Nesterov <oleg@redhat.com> - 2017-04-05 20:20 +0200
        [RFC][PATCH v2 2/5] sighand: Count each thread group once in sighand_struct ebiederm@xmission.com (Eric W. Biederman) - 2017-04-03 01:00 +0200
        [RFC][PATCH v2 5/5] signal: Don't allow accessing signal_struct by old threads after exec ebiederm@xmission.com (Eric W. Biederman) - 2017-04-03 01:10 +0200
          Re: [RFC][PATCH v2 5/5] signal: Don't allow accessing         signal_struct by old threads after exec Oleg Nesterov <oleg@redhat.com> - 2017-04-05 18:20 +0200
            Re: [RFC][PATCH v2 5/5] signal: Don't allow accessing signal_struct by old threads after exec ebiederm@xmission.com (Eric W. Biederman) - 2017-04-05 20:30 +0200
              Re: [RFC][PATCH v2 5/5] signal: Don't allow accessing signal_struct  by old threads after exec Oleg Nesterov <oleg@redhat.com> - 2017-04-06 18:00 +0200
    Re: [RFC][PATCH] exec: Don't wait for ptraced threads to be reaped. Oleg Nesterov <oleg@redhat.com> - 2017-04-02 18:20 +0200
      Re: [RFC][PATCH] exec: Don't wait for ptraced threads to be reaped. ebiederm@xmission.com (Eric W. Biederman) - 2017-04-02 23:20 +0200
        Re: [RFC][PATCH] exec: Don't wait for ptraced threads to be reaped. Oleg Nesterov <oleg@redhat.com> - 2017-04-03 20:40 +0200
          scope of cred_guard_mutex. ebiederm@xmission.com (Eric W. Biederman) - 2017-04-04 01:00 +0200
            Re: scope of cred_guard_mutex. Oleg Nesterov <oleg@redhat.com> - 2017-04-05 18:10 +0200
              Re: scope of cred_guard_mutex. Kees Cook <keescook@chromium.org> - 2017-04-05 18:20 +0200
              Re: scope of cred_guard_mutex. ebiederm@xmission.com (Eric W. Biederman) - 2017-04-05 20:10 +0200
                Re: scope of cred_guard_mutex. Oleg Nesterov <oleg@redhat.com> - 2017-04-05 20:20 +0200
            Re: scope of cred_guard_mutex. Oleg Nesterov <oleg@redhat.com> - 2017-04-06 18:00 +0200
              Re: scope of cred_guard_mutex. Kees Cook <keescook@chromium.org> - 2017-04-08 00:10 +0200
          Re: [RFC][PATCH] exec: Don't wait for ptraced threads to be reaped. ebiederm@xmission.com (Eric W. Biederman) - 2017-04-04 01:00 +0200

Page 1 of 2  [1] 2  Next page →

#1612711 — Re: [RFC][PATCH] exec: Don't wait for ptraced threads to be reaped.

Oleg Nesterov <oleg@redhat.com> writes:

> On 03/03, Eric W. Biederman wrote:
>> @@ -1065,11 +1065,8 @@ static int de_thread(struct task_struct *tsk)
>>  	}
>>
>>  	sig->group_exit_task = tsk;
>> -	sig->notify_count = zap_other_threads(tsk);
>> -	if (!thread_group_leader(tsk))
>> -		sig->notify_count--;
>> -
>> -	while (sig->notify_count) {
>> +	zap_other_threads(tsk);
>> +	while (atomic_read(&sig->live) > 1) {
>>  		__set_current_state(TASK_KILLABLE);
>>  		spin_unlock_irq(lock);
>>  		schedule();
>
> Very nice. So de_thread() returns as soon as all other threads decrement
> signal->live in do_exit(). Before they do, say, exit_mm(). This is already
> wrong, for example this breaks OOM. Plus a lot more problems afaics,  but
> lets ignore this.

Which means that we need to keep sig->notify_count.

> Note that de_thread() also unshares ->sighand before return. So in the
> case of mt exec it will likely see oldsighand->count != 1 and alloc the
> new sighand_struct and this breaks the locking.
>
> Because the execing thread will use newsighand->siglock to protect its
> signal_struct while the zombie threads will use oldsighand->siglock to
> protect the same signal struct. Yes, tasklist_lock + the fact irq_disable
> implies rcu_lock mostly save us but not entirely, say, a foreign process
> doing __send_signal() can take the right or the wrong lock depending on
> /dev/random.

Which leads to the question how can we get back tot he 2.4 behavior
of freeing sighand_struct in do_exit?

At which point as soon as we free sighand_struct if we are the last
to dying thread notify de_thread and everything works.

There are only two uses of sighand->siglock that I can see in
release_task:  __ptrace_unlink and __exit_signal.

For what __ptrace_unlink is doing we should just be able to skip
acquiring of siglock if PF_EXITING is set.

__exit_signal is a little more interesting but half of what it is
doing looks like it was pulled out of do_exit and just needs to
be put back.

Which probably adds up to 4 or 5 small carefully written patches to sort
out that part of the exit path, but I think it leads to a very simple
and straight forward result.  With the only side effect that we
can exec a process and still have a debugger reaping zombie threads.

Oleg does that sound reasonable to you?

Eric

[toc] | [next] | [standalone]

#1614396 — [RFC][PATCH 0/2] exec: Fixing ptrace'd mulit-threaded hang

I spent a little more time with this and only waiting until the killed
thread are zombies (and not reaped as we do today) really looks like
the right fix.

Oleg the following two patches work on top of your PTRACE_EVENT_EXIT
change and probably need a little more cleanup until they are ready
for serious posting.

That said I want to I want to post the code so I have a change at
some feedback before I prepare the final round of patches.

These patches only handle the case when sighand_struct is not
shared between different multi-threaded processes.  The general
case is solvable but that is a quite a bit more code.

Eric W. Biederman (2):
      sighand: Count each thread group once in sighand_struct
      exec: If possible don't wait for ptraced threads to be reaped

 fs/exec.c                    | 15 ++++++++++-----
 include/linux/sched/signal.h |  2 +-
 kernel/exit.c                | 15 ++++++++++-----
 kernel/fork.c                |  6 ++++--
 kernel/signal.c              |  8 ++++++--
 5 files changed, 31 insertions(+), 15 deletions(-)

Eric

[toc] | [prev] | [next] | [standalone]

#1614397 — [RFC][PATCH 1/2] sighand: Count each thread group once in sighand_struct

In practice either a thread group is either using a sighand_struct or
it isn't.  Therefore simplify things a bit and only increment the
count in sighand_struct when a new thread group is created that uses
the existing sighand_struct, and only decrement the count in
sighand_struct when a thread group exits.

As well as standing on it's own merits this has the potential to simply
de_thread.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/exit.c | 2 +-
 kernel/fork.c | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index e126ebf2400c..8c5b3e106298 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -163,9 +163,9 @@ static void __exit_signal(struct task_struct *tsk)
 	tsk->sighand = NULL;
 	spin_unlock(&sighand->siglock);
 
-	__cleanup_sighand(sighand);
 	clear_tsk_thread_flag(tsk, TIF_SIGPENDING);
 	if (group_dead) {
+		__cleanup_sighand(sighand);
 		flush_sigqueue(&sig->shared_pending);
 		tty_kref_put(tty);
 	}
diff --git a/kernel/fork.c b/kernel/fork.c
index 6c463c80e93d..fe6f1bf32bb9 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1295,7 +1295,8 @@ static int copy_sighand(unsigned long clone_flags, struct task_struct *tsk)
 	struct sighand_struct *sig;
 
 	if (clone_flags & CLONE_SIGHAND) {
-		atomic_inc(&current->sighand->count);
+		if (!(clone_flags & CLONE_THREAD))
+			atomic_inc(&current->sighand->count);
 		return 0;
 	}
 	sig = kmem_cache_alloc(sighand_cachep, GFP_KERNEL);
@@ -1896,7 +1897,8 @@ static __latent_entropy struct task_struct *copy_process(
 	if (!(clone_flags & CLONE_THREAD))
 		free_signal_struct(p->signal);
 bad_fork_cleanup_sighand:
-	__cleanup_sighand(p->sighand);
+	if (!(clone_flags & CLONE_THREAD))
+		__cleanup_sighand(p->sighand);
 bad_fork_cleanup_fs:
 	exit_fs(p); /* blocking */
 bad_fork_cleanup_files:
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1614398 — [RFC][PATCH 2/2] exec: If possible don't wait for ptraced threads to be reaped

Take advantage of the situation when sighand->count == 1 to only wait
for threads to reach EXIT_ZOMBIE instead of EXIT_DEAD in de_thread.
Only old old linux threading libraries use CLONE_SIGHAND without
CLONE_THREAD.  So this situation should be present most of the time.

This allows ptracing through a multi-threaded exec without the danger
of stalling the exec.  As historically exec waits for the other
threads to be reaped in de_thread before completing.  This is
necessary as it is not safe to unshare the sighand_struct until all of
the other threads in this thread group are reaped, because the lock to
serialize threads in a thread group siglock lives in sighand_struct.

When oldsighand->count == 1 we know that there are no other
users and unsharing the sighand struct in exec is pointless.
This makes it safe to only wait for threads to become zombies
as the siglock won't change during exec and release_task
will use the samve siglock for the old threads as for
the new threads.

Cc: stable@vger.kernel.org
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/exec.c                    | 15 ++++++++++-----
 include/linux/sched/signal.h |  2 +-
 kernel/exit.c                | 13 +++++++++----
 kernel/signal.c              |  8 ++++++--
 4 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 65145a3df065..0fd29342bbe4 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1052,6 +1052,7 @@ static int de_thread(struct task_struct *tsk)
 	struct signal_struct *sig = tsk->signal;
 	struct sighand_struct *oldsighand = tsk->sighand;
 	spinlock_t *lock = &oldsighand->siglock;
+	bool may_hang;
 
 	if (thread_group_empty(tsk))
 		goto no_thread_group;
@@ -1069,9 +1070,10 @@ static int de_thread(struct task_struct *tsk)
 		return -EAGAIN;
 	}
 
+	may_hang = atomic_read(&oldsighand->count) != 1;
 	sig->group_exit_task = tsk;
-	sig->notify_count = zap_other_threads(tsk);
-	if (!thread_group_leader(tsk))
+	sig->notify_count = zap_other_threads(tsk, may_hang ? 1 : -1);
+	if (may_hang && !thread_group_leader(tsk))
 		sig->notify_count--;
 
 	while (sig->notify_count) {
@@ -1092,9 +1094,10 @@ static int de_thread(struct task_struct *tsk)
 	if (!thread_group_leader(tsk)) {
 		struct task_struct *leader = tsk->group_leader;
 
-		for (;;) {
-			cgroup_threadgroup_change_begin(tsk);
-			write_lock_irq(&tasklist_lock);
+		cgroup_threadgroup_change_begin(tsk);
+		write_lock_irq(&tasklist_lock);
+
+		for (;may_hang;) {
 			/*
 			 * Do this under tasklist_lock to ensure that
 			 * exit_notify() can't miss ->group_exit_task
@@ -1108,6 +1111,8 @@ static int de_thread(struct task_struct *tsk)
 			schedule();
 			if (unlikely(__fatal_signal_pending(tsk)))
 				goto killed;
+			cgroup_threadgroup_change_begin(tsk);
+			write_lock_irq(&tasklist_lock);
 		}
 
 		/*
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 2cf446704cd4..187a9e980d3a 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -298,7 +298,7 @@ extern __must_check bool do_notify_parent(struct task_struct *, int);
 extern void __wake_up_parent(struct task_struct *p, struct task_struct *parent);
 extern void force_sig(int, struct task_struct *);
 extern int send_sig(int, struct task_struct *, int);
-extern int zap_other_threads(struct task_struct *p);
+extern int zap_other_threads(struct task_struct *p, int do_count);
 extern struct sigqueue *sigqueue_alloc(void);
 extern void sigqueue_free(struct sigqueue *);
 extern int send_sigqueue(struct sigqueue *,  struct task_struct *, int group);
diff --git a/kernel/exit.c b/kernel/exit.c
index 8c5b3e106298..972df5ebf79f 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -712,6 +712,8 @@ static void forget_original_parent(struct task_struct *father,
  */
 static void exit_notify(struct task_struct *tsk, int group_dead)
 {
+	struct sighand_struct *sighand = tsk->sighand;
+	struct signal_struct *signal = tsk->signal;
 	bool autoreap;
 	struct task_struct *p, *n;
 	LIST_HEAD(dead);
@@ -739,9 +741,12 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	if (tsk->exit_state == EXIT_DEAD)
 		list_add(&tsk->ptrace_entry, &dead);
 
-	/* mt-exec, de_thread() is waiting for group leader */
-	if (unlikely(tsk->signal->notify_count < 0))
-		wake_up_process(tsk->signal->group_exit_task);
+	spin_lock(&sighand->siglock);
+	/* mt-exec, de_thread is waiting for threads to exit */
+	if (signal->notify_count < 0 && !++signal->notify_count)
+		wake_up_process(signal->group_exit_task);
+
+	spin_unlock(&sighand->siglock);
 	write_unlock_irq(&tasklist_lock);
 
 	list_for_each_entry_safe(p, n, &dead, ptrace_entry) {
@@ -975,7 +980,7 @@ do_group_exit(int exit_code)
 		else {
 			sig->group_exit_code = exit_code;
 			sig->flags = SIGNAL_GROUP_EXIT;
-			zap_other_threads(current);
+			zap_other_threads(current, 0);
 		}
 		spin_unlock_irq(&sighand->siglock);
 	}
diff --git a/kernel/signal.c b/kernel/signal.c
index 986ef55641ea..e3a5bc239345 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1196,7 +1196,7 @@ force_sig_info(int sig, struct siginfo *info, struct task_struct *t)
 /*
  * Nuke all other threads in the group.
  */
-int zap_other_threads(struct task_struct *p)
+int zap_other_threads(struct task_struct *p, int do_count)
 {
 	struct task_struct *t = p;
 	int count = 0;
@@ -1205,13 +1205,17 @@ int zap_other_threads(struct task_struct *p)
 
 	while_each_thread(p, t) {
 		task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
-		count++;
+		if (do_count > 0)
+			count++;
 
 		/* Don't bother with already dead threads */
 		if (t->exit_state)
 			continue;
 		sigaddset(&t->pending.signal, SIGKILL);
 		signal_wake_up(t, 1);
+
+		if (do_count < 0)
+			count--;
 	}
 
 	return count;
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1614758 — Re: [RFC][PATCH 2/2] exec: If possible don't wait for ptraced threads to be reaped

On 04/01, Eric W. Biederman wrote:
>
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1052,6 +1052,7 @@ static int de_thread(struct task_struct *tsk)
>  	struct signal_struct *sig = tsk->signal;
>  	struct sighand_struct *oldsighand = tsk->sighand;
>  	spinlock_t *lock = &oldsighand->siglock;
> +	bool may_hang;
>
>  	if (thread_group_empty(tsk))
>  		goto no_thread_group;
> @@ -1069,9 +1070,10 @@ static int de_thread(struct task_struct *tsk)
>  		return -EAGAIN;
>  	}
>
> +	may_hang = atomic_read(&oldsighand->count) != 1;
>  	sig->group_exit_task = tsk;
> -	sig->notify_count = zap_other_threads(tsk);
> -	if (!thread_group_leader(tsk))
> +	sig->notify_count = zap_other_threads(tsk, may_hang ? 1 : -1);

Eric, this is amazing. So with this patch exec does different things depening
on whether sighand is shared with another CLONE_SIGHAND task or not. To me
this doesn't look sane in any case.

And of course you do realize that it doesn't solve the problem entirely? If I
modify my test-case a little bit

	int xxx(void *arg)
	{
		for (;;)
			pause();
	}

	void *thread(void *arg)
	{
		ptrace(PTRACE_TRACEME, 0,0,0);
		return NULL;
	}

	int main(void)
	{
		int pid = fork();

		if (!pid) {
			pthread_t pt;
			char stack[16 * 1024];

			clone(xxx, stack + 16*1024, CLONE_SIGHAND|CLONE_VM, NULL);

			pthread_create(&pt, NULL, thread, NULL);
			pthread_join(pt, NULL);
			execlp("echo", "echo", "passed", NULL);
		}

		sleep(1);
		// or anything else which needs ->cred_guard_mutex,
		// say open(/proc/$pid/mem)
		ptrace(PTRACE_ATTACH, pid, 0,0);
		kill(pid, SIGCONT);

		return 0;
	}

it should deadlock the same way?

So what is the point to make the, well imo insane, patch if it doesn't solve
the problem?

And btw zap_other_threads(may_hang == 0) is racy. Either you need tasklist or
exit_notify() should set tsk->exit_state under siglock, otherwise zap() can
return the wrong count.

Finally. This patch creates the nice security hole. Let me modify my test-case
again:

	void *thread(void *arg)
	{
		ptrace(PTRACE_TRACEME, 0,0,0);
		return NULL;
	}

	int main(void)
	{
		int pid = fork();

		if (!pid) {
			pthread_t pt;
			pthread_create(&pt, NULL, thread, NULL);
			pthread_join(pt, NULL);
			execlp(path-to-setuid-binary, args);
		}

		sleep(1);

		// Now we can send the signals to setiuid app
		kill(pid+1, ANYSIGNAL);

		return 0;
	}

I see another email from your with another proposal. I disagree, will reply soon.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1614784 — Re: [RFC][PATCH 2/2] exec: If possible don't wait for ptraced threads to be reaped

Oleg Nesterov <oleg@redhat.com> writes:

> On 04/01, Eric W. Biederman wrote:
>>
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1052,6 +1052,7 @@ static int de_thread(struct task_struct *tsk)
>>  	struct signal_struct *sig = tsk->signal;
>>  	struct sighand_struct *oldsighand = tsk->sighand;
>>  	spinlock_t *lock = &oldsighand->siglock;
>> +	bool may_hang;
>>
>>  	if (thread_group_empty(tsk))
>>  		goto no_thread_group;
>> @@ -1069,9 +1070,10 @@ static int de_thread(struct task_struct *tsk)
>>  		return -EAGAIN;
>>  	}
>>
>> +	may_hang = atomic_read(&oldsighand->count) != 1;
>>  	sig->group_exit_task = tsk;
>> -	sig->notify_count = zap_other_threads(tsk);
>> -	if (!thread_group_leader(tsk))
>> +	sig->notify_count = zap_other_threads(tsk, may_hang ? 1 : -1);
>
> Eric, this is amazing. So with this patch exec does different things depening
> on whether sighand is shared with another CLONE_SIGHAND task or not. To me
> this doesn't look sane in any case.

It is a 99% solution that makes it possible to talk about and review
letting the exec continue after the subthreads are killed but not
reaped.

Sigh I should have made may_hang say:

may_hang = (atomic_read(&oldsignand->count) != 1) && (sig->nr_threads > 1)

Which covers all know ways userspace actually uses these clone flags.

> And btw zap_other_threads(may_hang == 0) is racy. Either you need tasklist or
> exit_notify() should set tsk->exit_state under siglock, otherwise zap() can
> return the wrong count.

zap_other_thread(tsk, 0) only gets called in the case where we don't
care about the return value.  It does not get called from fs/exec.c

> Finally. This patch creates the nice security hole. Let me modify my test-case
> again:
>
> 	void *thread(void *arg)
> 	{
> 		ptrace(PTRACE_TRACEME, 0,0,0);
> 		return NULL;
> 	}
>
> 	int main(void)
> 	{
> 		int pid = fork();
>
> 		if (!pid) {
> 			pthread_t pt;
> 			pthread_create(&pt, NULL, thread, NULL);
> 			pthread_join(pt, NULL);
> 			execlp(path-to-setuid-binary, args);
> 		}
>
> 		sleep(1);
>
> 		// Now we can send the signals to setiuid app
> 		kill(pid+1, ANYSIGNAL);
>
> 		return 0;
> 	}

That is a substantive objection, and something that definitely needs
to get fixed.   Can you think of anything else?

Eric

[toc] | [prev] | [next] | [standalone]

#1615460 — Re: [RFC][PATCH 2/2] exec: If possible don't wait for ptraced threads to be reaped

On 04/02, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > And btw zap_other_threads(may_hang == 0) is racy. Either you need tasklist or
> > exit_notify() should set tsk->exit_state under siglock, otherwise zap() can
> > return the wrong count.
>
> zap_other_thread(tsk, 0) only gets called in the case where we don't
> care about the return value.  It does not get called from fs/exec.c

I meant that may_hang == 0 implies zap_other_threads(do_count => -1) which should
return the number of threads which didn't pass exit_notify(). The returned value
can be wrong unless you change exit_notify() to set exit_state under siglock.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1615559 — Re: [RFC][PATCH 2/2] exec: If possible don't wait for ptraced threads to be reaped

Oleg Nesterov <oleg@redhat.com> writes:

> On 04/02, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>>
>> > And btw zap_other_threads(may_hang == 0) is racy. Either you need tasklist or
>> > exit_notify() should set tsk->exit_state under siglock, otherwise zap() can
>> > return the wrong count.
>>
>> zap_other_thread(tsk, 0) only gets called in the case where we don't
>> care about the return value.  It does not get called from fs/exec.c
>
> I meant that may_hang == 0 implies zap_other_threads(do_count => -1) which should
> return the number of threads which didn't pass exit_notify(). The returned value
> can be wrong unless you change exit_notify() to set exit_state under
> siglock.

Interesting an existing bug.  I won't deny that one.

Subtle to catch but easy enough to fix.

Eric

[toc] | [prev] | [next] | [standalone]

#1617132 — Re: [RFC][PATCH 2/2] exec: If possible don't wait for ptraced threads to be reaped

On 04/03, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > I meant that may_hang == 0 implies zap_other_threads(do_count => -1) which should
> > return the number of threads which didn't pass exit_notify(). The returned value
> > can be wrong unless you change exit_notify() to set exit_state under
> > siglock.

but I forgot to add that, of course, this problem is very minor because
we can only miss a thread which is already at the end of exit_notify()
so nothing bad can happen.

But imo should be fixed anyway, simply because this looks wrong/racy.
Your recent 4/5 has the same problem.

> Interesting an existing bug.

Hmm... what do you mean? The current code looks fine.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1614757 — Re: [RFC][PATCH 0/2] exec: Fixing ptrace'd mulit-threaded hang

On 04/01, Eric W. Biederman wrote:
>
> These patches only handle the case when sighand_struct is not
> shared between different multi-threaded processes.

Ah, I didn't read 0/2 when looked at 2/2, so at least you documented this.

> The general
> case is solvable but that is a quite a bit more code.

I don't think so, please see my reply to 2/2.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1614832 — [RFC][PATCH v2 4/5] exec: If possible don't wait for ptraced threads to be reaped

Take advantage of the situation when sighand->count == 1 to only wait
for threads to reach EXIT_ZOMBIE instead of EXIT_DEAD in de_thread.
Only old old linux threading libraries use CLONE_SIGHAND without
CLONE_THREAD.  So this situation should be present most of the time.

This allows ptracing through a multi-threaded exec without the danger
of stalling the exec.  As historically exec waits for the other
threads to be reaped in de_thread before completing.  This is
necessary as it is not safe to unshare the sighand_struct until all of
the other threads in this thread group are reaped, because the lock to
serialize threads in a thread group siglock lives in sighand_struct.

When oldsighand->count == 1 we know that there are no other
users and unsharing the sighand struct in exec is pointless.
This makes it safe to only wait for threads to become zombies
as the siglock won't change during exec and release_task
will use the samve siglock for the old threads as for
the new threads.

Cc: stable@vger.kernel.org
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/exec.c       | 22 ++--------------------
 kernel/exit.c   | 18 ++++++++----------
 kernel/signal.c |  2 +-
 3 files changed, 11 insertions(+), 31 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 65145a3df065..303a114b00ce 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1071,9 +1071,6 @@ static int de_thread(struct task_struct *tsk)
 
 	sig->group_exit_task = tsk;
 	sig->notify_count = zap_other_threads(tsk);
-	if (!thread_group_leader(tsk))
-		sig->notify_count--;
-
 	while (sig->notify_count) {
 		__set_current_state(TASK_KILLABLE);
 		spin_unlock_irq(lock);
@@ -1092,23 +1089,8 @@ static int de_thread(struct task_struct *tsk)
 	if (!thread_group_leader(tsk)) {
 		struct task_struct *leader = tsk->group_leader;
 
-		for (;;) {
-			cgroup_threadgroup_change_begin(tsk);
-			write_lock_irq(&tasklist_lock);
-			/*
-			 * Do this under tasklist_lock to ensure that
-			 * exit_notify() can't miss ->group_exit_task
-			 */
-			sig->notify_count = -1;
-			if (likely(leader->exit_state))
-				break;
-			__set_current_state(TASK_KILLABLE);
-			write_unlock_irq(&tasklist_lock);
-			cgroup_threadgroup_change_end(tsk);
-			schedule();
-			if (unlikely(__fatal_signal_pending(tsk)))
-				goto killed;
-		}
+		cgroup_threadgroup_change_begin(tsk);
+		write_lock_irq(&tasklist_lock);
 
 		/*
 		 * The only record we have of the real-time age of a
diff --git a/kernel/exit.c b/kernel/exit.c
index 8c5b3e106298..955c96e3fc12 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -118,13 +118,6 @@ static void __exit_signal(struct task_struct *tsk)
 		tty = sig->tty;
 		sig->tty = NULL;
 	} else {
-		/*
-		 * If there is any task waiting for the group exit
-		 * then notify it:
-		 */
-		if (sig->notify_count > 0 && !--sig->notify_count)
-			wake_up_process(sig->group_exit_task);
-
 		if (tsk == sig->curr_target)
 			sig->curr_target = next_thread(tsk);
 	}
@@ -712,6 +705,8 @@ static void forget_original_parent(struct task_struct *father,
  */
 static void exit_notify(struct task_struct *tsk, int group_dead)
 {
+	struct sighand_struct *sighand = tsk->sighand;
+	struct signal_struct *signal = tsk->signal;
 	bool autoreap;
 	struct task_struct *p, *n;
 	LIST_HEAD(dead);
@@ -739,9 +734,12 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	if (tsk->exit_state == EXIT_DEAD)
 		list_add(&tsk->ptrace_entry, &dead);
 
-	/* mt-exec, de_thread() is waiting for group leader */
-	if (unlikely(tsk->signal->notify_count < 0))
-		wake_up_process(tsk->signal->group_exit_task);
+	spin_lock(&sighand->siglock);
+	/* mt-exec, de_thread is waiting for threads to exit */
+	if (signal->notify_count > 0 && !--signal->notify_count)
+		wake_up_process(signal->group_exit_task);
+
+	spin_unlock(&sighand->siglock);
 	write_unlock_irq(&tasklist_lock);
 
 	list_for_each_entry_safe(p, n, &dead, ptrace_entry) {
diff --git a/kernel/signal.c b/kernel/signal.c
index 11fa736eb2ae..fd75ba33ee3d 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1205,13 +1205,13 @@ int zap_other_threads(struct task_struct *p)
 
 	while_each_thread(p, t) {
 		task_clear_jobctl_pending(t, JOBCTL_PENDING_MASK);
-		count++;
 
 		/* Don't bother with already dead threads */
 		if (t->exit_state)
 			continue;
 		sigaddset(&t->pending.signal, SIGKILL);
 		signal_wake_up(t, 1);
+		count++;
 	}
 
 	return count;
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1617109 — Re: [RFC][PATCH v2 4/5] exec: If possible don't wait for ptraced threads to be reaped

On 04/02, Eric W. Biederman wrote:
>
> Take advantage of the situation when sighand->count == 1 to only wait
> for threads to reach EXIT_ZOMBIE instead of EXIT_DEAD in de_thread.

Let me comment this patch first, it looks mostly fine to me.

And note that this is what my patch does too: exec() waits until
all threads pass exit_notify() and drops cred_guard_mutex.

However, with my patch patch exec() then waits until all threads
disappear. This is uglifies the code but this is simple and safe.

With your patches exec doesn't do another wait and succeeds after
the 1st wait. I think this is wrong and the next patch is not enough.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1614833 — [RFC][PATCH v2 1/5] ptrace: Don't wait in PTRACE_O_TRACEEXIT for exec or coredump

Take advantage of the fact that no ptrace stop will wait for
a debugger if another process sends SIGKILL to the waiting task.

In the case of exec and coredump which have many interesting deadlock
opportunities and no one tests the what happens if you are ptraced
during exec or coredump act like another SIGKILL was immediately
sent to the process and don't stop.

Keep sending the signal to the tracer so that this appears like
the worst case where someone else sent the process a SIGKILL before
the tracer could react.  So all non-buggy tracers must support
this case.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 kernel/signal.c | 103 +++++++++++++++++++++++---------------------------------
 1 file changed, 42 insertions(+), 61 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index 7e59ebc2c25e..11fa736eb2ae 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1740,30 +1740,6 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
 	spin_unlock_irqrestore(&sighand->siglock, flags);
 }
 
-static inline int may_ptrace_stop(void)
-{
-	if (!likely(current->ptrace))
-		return 0;
-	/*
-	 * Are we in the middle of do_coredump?
-	 * If so and our tracer is also part of the coredump stopping
-	 * is a deadlock situation, and pointless because our tracer
-	 * is dead so don't allow us to stop.
-	 * If SIGKILL was already sent before the caller unlocked
-	 * ->siglock we must see ->core_state != NULL. Otherwise it
-	 * is safe to enter schedule().
-	 *
-	 * This is almost outdated, a task with the pending SIGKILL can't
-	 * block in TASK_TRACED. But PTRACE_EVENT_EXIT can be reported
-	 * after SIGKILL was already dequeued.
-	 */
-	if (unlikely(current->mm->core_state) &&
-	    unlikely(current->mm == current->parent->mm))
-		return 0;
-
-	return 1;
-}
-
 /*
  * Return non-zero if there is a SIGKILL that should be waking us up.
  * Called with the siglock held.
@@ -1789,6 +1765,8 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 	__releases(&current->sighand->siglock)
 	__acquires(&current->sighand->siglock)
 {
+	bool ptrace_parent;
+	bool ptrace_wait;
 	bool gstop_done = false;
 
 	if (arch_ptrace_stop_needed(exit_code, info)) {
@@ -1842,53 +1820,56 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 
 	spin_unlock_irq(&current->sighand->siglock);
 	read_lock(&tasklist_lock);
-	if (may_ptrace_stop()) {
-		/*
-		 * Notify parents of the stop.
-		 *
-		 * While ptraced, there are two parents - the ptracer and
-		 * the real_parent of the group_leader.  The ptracer should
-		 * know about every stop while the real parent is only
-		 * interested in the completion of group stop.  The states
-		 * for the two don't interact with each other.  Notify
-		 * separately unless they're gonna be duplicates.
-		 */
+	ptrace_parent = likely(current->ptrace);
+	/*
+	 * Notify parents of the stop.
+	 *
+	 * While ptraced, there are two parents - the ptracer and
+	 * the real_parent of the group_leader.  The ptracer should
+	 * know about every stop while the real parent is only
+	 * interested in the completion of group stop.  The states
+	 * for the two don't interact with each other.  Notify
+	 * separately unless they're gonna be duplicates.
+	 */
+	if (ptrace_parent)
 		do_notify_parent_cldstop(current, true, why);
-		if (gstop_done && ptrace_reparented(current))
-			do_notify_parent_cldstop(current, false, why);
+	if (gstop_done && (ptrace_reparented(current) || !ptrace_parent))
+		do_notify_parent_cldstop(current, false, why);
 
-		/*
-		 * Don't want to allow preemption here, because
-		 * sys_ptrace() needs this task to be inactive.
-		 *
-		 * XXX: implement read_unlock_no_resched().
-		 */
-		preempt_disable();
-		read_unlock(&tasklist_lock);
-		preempt_enable_no_resched();
-		freezable_schedule();
-	} else {
-		/*
-		 * By the time we got the lock, our tracer went away.
-		 * Don't drop the lock yet, another tracer may come.
-		 *
-		 * If @gstop_done, the ptracer went away between group stop
-		 * completion and here.  During detach, it would have set
-		 * JOBCTL_STOP_PENDING on us and we'll re-enter
-		 * TASK_STOPPED in do_signal_stop() on return, so notifying
-		 * the real parent of the group stop completion is enough.
-		 */
-		if (gstop_done)
-			do_notify_parent_cldstop(current, false, why);
+	/*
+	 * Always wait for the traceer if the process is traced unless
+	 * the process is in the middle of an exec or core dump.
+	 *
+	 * In exec we risk a deadlock with de_thread waiting for the
+	 * thread to be killed, the tracer, and acquring cred_guard_mutex.
+	 *
+	 * In coredump we risk a deadlock if the tracer is also part
+	 * of the coredump stopping.
+	 */
+	ptrace_wait = ptrace_parent &&
+		!current->signal->group_exit_task &&
+		!current->mm->core_state;
 
+	if (!ptrace_wait) {
 		/* tasklist protects us from ptrace_freeze_traced() */
 		__set_current_state(TASK_RUNNING);
 		if (clear_code)
 			current->exit_code = 0;
-		read_unlock(&tasklist_lock);
 	}
 
 	/*
+	 * Don't want to allow preemption here, because
+	 * sys_ptrace() needs this task to be inactive.
+	 *
+	 * XXX: implement read_unlock_no_resched().
+	 */
+	preempt_disable();
+	read_unlock(&tasklist_lock);
+	preempt_enable_no_resched();
+	if (ptrace_wait)
+		freezable_schedule();
+
+	/*
 	 * We are back.  Now reacquire the siglock before touching
 	 * last_siginfo, so that we are sure to have synchronized with
 	 * any signal-sending on another CPU that wants to examine it.
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1617124 — Re: [RFC][PATCH v2 1/5] ptrace: Don't wait in PTRACE_O_TRACEEXIT for exec or coredump

On 04/02, Eric W. Biederman wrote:
>
> In the case of exec and coredump which have many interesting deadlock
> opportunities

So this patch is very close to my 2/2 one-liner, except

	- you removed the current->mm == current->parent->mm check

	  I didn't do this on purpose, because even the->core_state
	  is not really needed if we check ->group_exit_task, this
	  need more changes anyway, but I won't argue.

	- With your patch we send the notification to debugger even
	  if we are not going to stop.

	  This is not wrong, but why? This is pointless, nobody rely
	  on SIGCHLD, if nothing else it doesn't queue.

	  Again, I won't argue, but this complicates both the patch
	  and the code for no reason. Unless I missed something.

> Keep sending the signal to the tracer so that this appears like
> the worst case where someone else sent the process a SIGKILL before
> the tracer could react.  So all non-buggy tracers must support
> this case.

Well, I can't understand the changelog. Sure, debugger must support
this case, but obviously this can break things anyway.

For example. The coredumping thread must stop in PTRACE_EVENT_EXIT.
There is a tool (I don't remember its name) which does
ptrace_attach(PTRACE_SEIZE, PTRACE_O_TRACEEXIT) after the coredump
was already started, closes the pipe, and reads the registers when
this thread actually exits.

This patch or my 2/2 should not break it, ->group_exit_task will be
cleared after do_coredump(), but unfortunately something else can be
broken.

So I think the changelog should mention that yes, this is the user
visible change which _can_ break something anyway.

In short. I will be really happy if this patch comes from you, not me ;)

Oleg.

[toc] | [prev] | [next] | [standalone]

#1614834 — [RFC][PATCH v2 0/5] exec: Fixing ptrace'd mulit-threaded hang

Oleg your comment about kill being able to send signal was an important
dimension I had missed thank you.

This patchset just denies the case of SIGHAND between different
multi-threaded processes as I don't think anyone cares.  I can
fix that if anyone cares but I am not certain we actally do.

I have reworked the ptrace notification code so that we always
send notifications if we can but don't wait if it is a coredump
or an exec.  Which simpilifies the code nicely.

A few more tweaks are needed before a final version but I think
things are compelling.

 fs/exec.c                    |  23 ++-------
 include/linux/sched/signal.h |   1 +
 kernel/exit.c                |  20 ++++----
 kernel/fork.c                |  14 +++++-
 kernel/ptrace.c              |   4 ++
 kernel/signal.c              | 112 +++++++++++++++++++------------------------
 6 files changed, 78 insertions(+), 96 deletions(-)

Eric W. Biederman (5):
      ptrace: Don't wait in PTRACE_O_TRACEEXIT for exec or coredump
      sighand: Count each thread group once in sighand_struct
      clone: Disallown CLONE_THREAD with a shared sighand_struct
      exec: If possible don't wait for ptraced threads to be reaped
      signal: Don't allow accessing signal_struct by old threads after exec

Eric

[toc] | [prev] | [next] | [standalone]

#1614835 — [RFC][PATCH v2 3/5] clone: Disallown CLONE_THREAD with a shared sighand_struct

Old threading libraries used CLONE_SIGHAND without clone thread.
Modern threadding libraries always use CLONE_SIGHAND | CLONE_THREAD.

Therefore let's simplify our lives and stop supporting a case no one cares about.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/fork.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/fork.c b/kernel/fork.c
index fe6f1bf32bb9..0632ac1180be 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1515,6 +1515,13 @@ static __latent_entropy struct task_struct *copy_process(
 	if ((clone_flags & CLONE_THREAD) && !(clone_flags & CLONE_SIGHAND))
 		return ERR_PTR(-EINVAL);
 
+	/* Disallow CLONE_THREAD with a shared SIGHAND structure.  No
+	 * one cares and supporting it leads to unnecessarily complex
+	 * code.
+	 */
+	if ((clone_flags & CLONE_THREAD) && (atomic_read(&current->sighand->count) > 1))
+		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
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

#1617121 — Re: [RFC][PATCH v2 3/5] clone: Disallown CLONE_THREAD with a shared sighand_struct

On 04/02, Eric W. Biederman wrote:
>
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1515,6 +1515,13 @@ static __latent_entropy struct task_struct *copy_process(
>  	if ((clone_flags & CLONE_THREAD) && !(clone_flags & CLONE_SIGHAND))
>  		return ERR_PTR(-EINVAL);
>
> +	/* Disallow CLONE_THREAD with a shared SIGHAND structure.  No
> +	 * one cares

Well, can't resists... I won't argue, but we can't know if no one cares
or not. I agree that most probably this won't break something, but who
knows... I am always scared when we add the incompatible changes.

> and supporting it leads to unnecessarily complex
> +	 * code.
> +	 */
> +	if ((clone_flags & CLONE_THREAD) && (atomic_read(&current->sighand->count) > 1))
> +		return ERR_PTR(-EINVAL);

Perhaps the comment should explain why we do this and say that
sighand-unsharing in de_thread() depends on this.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1617242 — Re: [RFC][PATCH v2 3/5] clone: Disallown CLONE_THREAD with a shared sighand_struct

Oleg Nesterov <oleg@redhat.com> writes:

> On 04/02, Eric W. Biederman wrote:
>>
>> --- a/kernel/fork.c
>> +++ b/kernel/fork.c
>> @@ -1515,6 +1515,13 @@ static __latent_entropy struct task_struct *copy_process(
>>  	if ((clone_flags & CLONE_THREAD) && !(clone_flags & CLONE_SIGHAND))
>>  		return ERR_PTR(-EINVAL);
>>
>> +	/* Disallow CLONE_THREAD with a shared SIGHAND structure.  No
>> +	 * one cares
>
> Well, can't resists... I won't argue, but we can't know if no one cares
> or not. I agree that most probably this won't break something, but who
> knows... I am always scared when we add the incompatible changes.

I agree that changing userspace semantics is something to be very
careful with.  But at least for purposes of discussion I think this is a
good patch.

I can avoid this change but it requires moving sighand->siglock
into signal_struct and introducing a new spinlock into sighand_struct
to just guard the signal handlers.

However I think the change to move siglock would be a distraction from
the larger issues of this patchset.

Once we address the core issues I will be happy to revisit this.

>> and supporting it leads to unnecessarily complex
>> +	 * code.
>> +	 */
>> +	if ((clone_flags & CLONE_THREAD) && (atomic_read(&current->sighand->count) > 1))
>> +		return ERR_PTR(-EINVAL);
>
> Perhaps the comment should explain why we do this and say that
> sighand-unsharing in de_thread() depends on this.

That would be a better comment.

Eric

[toc] | [prev] | [next] | [standalone]

#1617258 — Re: [RFC][PATCH v2 3/5] clone: Disallown CLONE_THREAD with a shared sighand_struct

On 04/05, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> I agree that changing userspace semantics is something to be very
> careful with.  But at least for purposes of discussion I think this is a
> good patch.

I agree that we need it with your approach,

but imo it would be much better to not depend on the subtle changes
like this. My 2/2 or your 1/5 are already bad enough.

> I can avoid this change but it requires moving sighand->siglock
> into signal_struct and introducing a new spinlock into sighand_struct
> to just guard the signal handlers.

Oh, this looks much, much worse to me.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1614836 — [RFC][PATCH v2 2/5] sighand: Count each thread group once in sighand_struct

In practice either a thread group is either using a sighand_struct or
it isn't.  Therefore simplify things a bit and only increment the
count in sighand_struct when a new thread group is created that uses
the existing sighand_struct, and only decrement the count in
sighand_struct when a thread group exits.

As well as standing on it's own merits this has the potential to simply
de_thread.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/exit.c | 2 +-
 kernel/fork.c | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index e126ebf2400c..8c5b3e106298 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -163,9 +163,9 @@ static void __exit_signal(struct task_struct *tsk)
 	tsk->sighand = NULL;
 	spin_unlock(&sighand->siglock);
 
-	__cleanup_sighand(sighand);
 	clear_tsk_thread_flag(tsk, TIF_SIGPENDING);
 	if (group_dead) {
+		__cleanup_sighand(sighand);
 		flush_sigqueue(&sig->shared_pending);
 		tty_kref_put(tty);
 	}
diff --git a/kernel/fork.c b/kernel/fork.c
index 6c463c80e93d..fe6f1bf32bb9 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1295,7 +1295,8 @@ static int copy_sighand(unsigned long clone_flags, struct task_struct *tsk)
 	struct sighand_struct *sig;
 
 	if (clone_flags & CLONE_SIGHAND) {
-		atomic_inc(&current->sighand->count);
+		if (!(clone_flags & CLONE_THREAD))
+			atomic_inc(&current->sighand->count);
 		return 0;
 	}
 	sig = kmem_cache_alloc(sighand_cachep, GFP_KERNEL);
@@ -1896,7 +1897,8 @@ static __latent_entropy struct task_struct *copy_process(
 	if (!(clone_flags & CLONE_THREAD))
 		free_signal_struct(p->signal);
 bad_fork_cleanup_sighand:
-	__cleanup_sighand(p->sighand);
+	if (!(clone_flags & CLONE_THREAD))
+		__cleanup_sighand(p->sighand);
 bad_fork_cleanup_fs:
 	exit_fs(p); /* blocking */
 bad_fork_cleanup_files:
-- 
2.10.1

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web