fix race copy_process() vs de_thread()

From: Hiroshi Shimamoto <[email protected]>

There is a race between de_thread() and copy_process().
When a thread is during execve and another thread is during clone, exec-ing thread may be hung up in de_thread() waiting other threads are finished. The root cause is that cleanup_signal() which is called when fork() failed doesn't cause wake up the waiting thread at de_thread(), because there is no check signal->count.

We need the check signal->group_exit_task and signal->notify_count.

Here is a reproducer, it may generate a thread which never die.

#include <sys/types.h>
#include <sys/wait.h>
#include <unistd.h>
#include <stdio.h>
#include <errno.h>
#include <pthread.h>

void *null_thread(void *p)

{
	for (;;)
		sleep(1);

	return NULL;
}

void *exec_thread(void *p)

{
	execl("/bin/true", "/bin/true", NULL);

	return null_thread(p);
}

int main(int argc, char **argv)

{
	for (;;) {
		pid_t pid;
		int ret, status;

		pid = fork();
		if (pid < 0)
			break;

		if (!pid) {
			pthread_t tid;

			pthread_create(&tid, NULL, exec_thread, NULL);
			for (;;)
				pthread_create(&tid, NULL, null_thread, NULL);
		}

		do {
			ret = waitpid(pid, &status, 0);
		} while (ret == -1 && errno == EINTR);
	}

	return 0;
}

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

index 3ffa10f..be6c5b5 100644
@@ -882,6 +882,9 @@ static void cleanup_signal(struct task_struct *tsk)
 {
 	struct signal_struct *sig = tsk->signal;
 
+	if (!sig)
+		return;
+
 	atomic_dec(&sig->live);
 
 	if (atomic_dec_and_test(&sig->count))

@@ -1230,6 +1233,15 @@ static struct task_struct *copy_process(unsigned long clone_flags,
  	 */
 	recalc_sigpending();
 	if (signal_pending(current)) {
+		/* If there is any task waiting for the group exit, notify it */
+		if ((clone_flags & CLONE_THREAD) &&
+		    p->signal->group_exit_task) {
+			atomic_dec(&p->signal->live);
+			atomic_dec(&p->signal->count);
+			if (atomic_read(&p->signal->count) == p->signal->notify_count)
+				wake_up_process(p->signal->group_exit_task);
+			p->signal = NULL;
+		}
 		spin_unlock(&current->sighand->siglock);
 		write_unlock_irq(&tasklist_lock);
 		retval = -ERESTARTNOINTR;

1.6.3.3

Ah, you're right. clone_process() may fail several reasons and failures after copy_signal() will cause this issue.

I don't know.

Update version is below.

Thanks,
Hiroshi

========
From: Hiroshi Shimamoto <[email protected]>

There is a race between de_thread() and copy_process().
When a thread is during execve and another thread is during clone, a exec-ing thread may be hung up in de_thread() waiting other threads are finished. The root cause is that cleanup_signal() which is called when fork() failed doesn't cause wake up the waiting thread at de_thread(), because there is no check signal->count.

We need the check signal->group_exit_task and signal->notify_count.

Here is a reproducer, it may generate a thread which never die.

void *null_thread(void *p)

{
	for (;;)
		sleep(1);

	return NULL;
}

void *exec_thread(void *p)

{
	execl("/bin/true", "/bin/true", NULL);

	return null_thread(p);
}

int main(int argc, char **argv)

{
	for (;;) {
		pid_t pid;
		int ret, status;

		pid = fork();
		if (pid < 0)
			break;

		if (!pid) {
			pthread_t tid;

			pthread_create(&tid, NULL, exec_thread, NULL);
			for (;;)
				pthread_create(&tid, NULL, null_thread, NULL);
		}

		do {
			ret = waitpid(pid, &status, 0);
		} while (ret == -1 && errno == EINTR);
	}

	return 0;
}

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

index 3ffa10f..bc41c41 100644
@@ -886,6 +886,14 @@ static void cleanup_signal(struct task_struct *tsk)
 
 	if (atomic_dec_and_test(&sig->count))
 		__cleanup_signal(sig);
+	else {
+		/* If there is any task waiting for the group exit, notify it */
+		spin_lock(&tsk->sighand->siglock);
+		if (sig->group_exit_task &&
+		    atomic_read(&sig->count) == sig->notify_count)
+			wake_up_process(sig->group_exit_task);
+		spin_unlock(&tsk->sighand->siglock);
+	}
 }
 
 static void copy_flags(unsigned long clone_flags, struct task_struct *p)

1.6.3.3

I'm not sure I follow the problem scenario you are citing.

The thread in copy_process() will return -ERESTARTNOINTR
after calling cleanup_signal(), which does properly decrement sig->count to return it to the state before the copy_process() call. Then this thread will get to signal handling, dequeue its SIGKILL from zap_other_threads(), and die itself. When it's finally reaped, by itself in exit_notify(), or by de_thread() in the case of a replaced group_leader, the normal __exit_signal() will do that group_exit_task logic.

What part of this sequence fails to occur in your tests?

Thanks,
Roland

Ah, I see. This happens when the copy_process() thread was the old group_leader itself. Then it never does release_task() itself.

Your fix seems adequate. Another approach would be to have some place like exit_signals() check for the group_exit_task case.

Oleg, what do you think?

Thanks,
Roland

The point is that de_thread() waits until other thread calls wake_up_process(). In __exit_signal() when sig->count == 2, the thread calls wake_up_process(), and then de_thread() will continue. However if another thread is during copy_process(), the sig->count is incremented at copy_signal(). That makes no wake_up_process().

The scenario flow is like this;

thread A thread B thread C sig->count

   |                 |           |              3
  clone()          exec()        |              |
  copy_process()   de_thread()   |              |
  copy_signal()      |           |              4
   |                zap          |              4
  KILL               |          KILL            4
   |                 |          __exit_signal() 3 no wake up
  clean_signal()     |                          2 no wake up
   |                 |                             during copy_process()
  exit               |

never wake up

thanks,
Hiroshi

Yes. Imho signal->count must die. But I never had time to kill it.

It is not needed. For example, __exit_signal() can just check thread_group_leader() instead of atomic_dec_and_test(sig->count).

As for this bug, I'd like to think a bit more. But how about the patch below? With this patch

- copy_process() increments signal/live only when we know we start the new thread

- if copy_process() fails, we just check CLONE_THREAD. If true - do nothing, the counters were not changed. If false - just release ->signal, counters must be 1.

Oleg.

@@ -815,11 +815,8 @@ static int copy_signal(unsigned long clo
 {
 	struct signal_struct *sig;
 
-	if (clone_flags & CLONE_THREAD) {
-		atomic_inc(&current->signal->count);
-		atomic_inc(&current->signal->live);
+	if (clone_flags & CLONE_THREAD)
 		return 0;
-	}
 
 	sig = kmem_cache_alloc(signal_cachep, GFP_KERNEL);
 	tsk->signal = sig;

@@ -877,16 +874,6 @@ void __cleanup_signal(struct signal_stru
 	kmem_cache_free(signal_cachep, sig);
 }
 
-static void cleanup_signal(struct task_struct *tsk)
-{
-	struct signal_struct *sig = tsk->signal;
-
-	atomic_dec(&sig->live);
-
-	if (atomic_dec_and_test(&sig->count))
-		__cleanup_signal(sig);
-}
-
 static void copy_flags(unsigned long clone_flags, struct task_struct *p)
 {
 	unsigned long new_flags = p->flags;

@@ -1239,6 +1226,8 @@ static struct task_struct *copy_process(
 	}
 
 	if (clone_flags & CLONE_THREAD) {
+		atomic_inc(&current->signal->count);
+		atomic_inc(&current->signal->live);
 		p->group_leader = current->group_leader;
 		list_add_tail_rcu(&p->thread_group, &p->group_leader->thread_group);
 	}

@@ -1282,7 +1271,8 @@ bad_fork_cleanup_mm:
 	if (p->mm)
 		mmput(p->mm);
 bad_fork_cleanup_signal:
-	cleanup_signal(p);
+	if (!(clone_flags & CLONE_THREAD))
+		__cleanup_signal(p);
 bad_fork_cleanup_sighand:
 	__cleanup_sighand(p->sighand);
 bad_fork_cleanup_fs:

Or we can do a bit smaller patch. But in any case, copy_process() must not use sig->count as a refcounter.

And of course, it would be nice to avoid playing ->notify_count games in copy_process() pathes.

Oleg

@@ -816,7 +816,6 @@ static int copy_signal(unsigned long clo
 	struct signal_struct *sig;
 
 	if (clone_flags & CLONE_THREAD) {
-		atomic_inc(&current->signal->count);
 		atomic_inc(&current->signal->live);
 		return 0;
 	}

@@ -881,9 +880,7 @@ static void cleanup_signal(struct task_s
 {
 	struct signal_struct *sig = tsk->signal;
 
-	atomic_dec(&sig->live);
-
-	if (atomic_dec_and_test(&sig->count))
+	if (atomic_dec_and_test(&sig->live))
 		__cleanup_signal(sig);
 }
 

@@ -1239,6 +1236,7 @@ static struct task_struct *copy_process(
 	}
 
 	if (clone_flags & CLONE_THREAD) {
+		atomic_inc(&current->signal->count);
 		p->group_leader = current->group_leader;
 		list_add_tail_rcu(&p->thread_group, &p->group_leader->thread_group);
 	}

I think I like your first patch better. It's a larger patch, but that's because it winds up with smaller code.

Thanks,
Roland

Yes, agreed.

And another reason for the first patch: ->live should not be used as a refcnt, it is not. This should work for copy_process() but looks confusing and relies on CLONE_THREAD subtleness anyway.

I'll re-check this patch and resend. I guess -stable needs this fix too.

Oleg.

Spotted by Hiroshi Shimamoto who also provided the test-case below.

copy_process() pathes use signal->count as a reference counter, but it is not. This test case

	#include <sys/types.h>
	#include <sys/wait.h>
	#include <unistd.h>
	#include <stdio.h>
	#include <errno.h>
	#include <pthread.h>

	void *null_thread(void *p)
	{
		for (;;)
			sleep(1);

		return NULL;
	}

	void *exec_thread(void *p)
	{
		execl("/bin/true", "/bin/true", NULL);

		return null_thread(p);
	}

	int main(int argc, char **argv)
	{
		for (;;) {
			pid_t pid;
			int ret, status;

			pid = fork();
			if (pid < 0)
				break;

			if (!pid) {
				pthread_t tid;

				pthread_create(&tid, NULL, exec_thread, NULL);
				for (;;)
					pthread_create(&tid, NULL, null_thread, NULL);
			}

			do {
				ret = waitpid(pid, &status, 0);
			} while (ret == -1 && errno == EINTR);
		}

		return 0;
	}

quickly creates the unkillable task.

If copy_process(CLONE_THREAD) races with de_thread() copy_signal()->atomic(signal->count) breaks the signal->notify_count logic, and the execing thread can hang forever in kernel space.

Change copy_process() to increment count/live only when we know for sure we can't fail. In this case the forked thread will take care of its reference to signal correctly.

If copy_process() fails, check CLONE_THREAD flag. If it it set - do nothing, the counters were not changed and current belongs to the same thread group. If it is not set, ->signal must be released in any case (and ->count must be == 1), the forked child is the only thread in the thread group.

We need more cleanups here, in particular signal->count should not be used by de_thread/__exit_signal at all. This patch only fixes the bug.

---

 kernel/fork.c |   20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

@@ -816,11 +816,8 @@ static int copy_signal(unsigned long clo
 {
 	struct signal_struct *sig;
 
-	if (clone_flags & CLONE_THREAD) {
-		atomic_inc(&current->signal->count);
-		atomic_inc(&current->signal->live);
+	if (clone_flags & CLONE_THREAD)
 		return 0;
-	}
 
 	sig = kmem_cache_alloc(signal_cachep, GFP_KERNEL);
 	tsk->signal = sig;

@@ -878,16 +875,6 @@ void __cleanup_signal(struct signal_stru
 	kmem_cache_free(signal_cachep, sig);
 }
 
-static void cleanup_signal(struct task_struct *tsk)
-{
-	struct signal_struct *sig = tsk->signal;
-
-	atomic_dec(&sig->live);
-
-	if (atomic_dec_and_test(&sig->count))
-		__cleanup_signal(sig);
-}
-
 static void copy_flags(unsigned long clone_flags, struct task_struct *p)
 {
 	unsigned long new_flags = p->flags;

@@ -1240,6 +1227,8 @@ static struct task_struct *copy_process(
 	}
 
 	if (clone_flags & CLONE_THREAD) {
+		atomic_inc(&current->signal->count);
+		atomic_inc(&current->signal->live);
 		p->group_leader = current->group_leader;
 		list_add_tail_rcu(&p->thread_group, &p->group_leader->thread_group);
 	}

@@ -1282,7 +1271,8 @@ bad_fork_cleanup_mm:
 	if (p->mm)
 		mmput(p->mm);
 bad_fork_cleanup_signal:
-	cleanup_signal(p);
+	if (!(clone_flags & CLONE_THREAD))
+		__cleanup_signal(p->signal);
 bad_fork_cleanup_sighand:
 	__cleanup_sighand(p->sighand);
 bad_fork_cleanup_fs:

Nice fix!

Thanks,
Hiroshi