[patch 15/71] clone(): fix race between copy_process() and de_thread()

From: Greg KH
Date: Fri Sep 04 2009 - 20:33:38 EST

2.6.30-stable review patch. If anyone has any objections, please let us know.

From: Oleg Nesterov <oleg@xxxxxxxxxx>

commit 4ab6c08336535f8c8e42cf45d7adeda882eff06e upstream.

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

copy_process() uses 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 (;;)

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)

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

Reported-by: Hiroshi Shimamoto <h-shimamoto@xxxxxxxxxxxxx>
Tested-by: Hiroshi Shimamoto <h-shimamoto@xxxxxxxxxxxxx>
Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
Acked-by: Roland McGrath <roland@xxxxxxxxxx>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxx>

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

--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -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;
@@ -1237,6 +1224,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);
@@ -1281,7 +1270,8 @@ bad_fork_cleanup_mm:
if (p->mm)
- cleanup_signal(p);
+ if (!(clone_flags & CLONE_THREAD))
+ __cleanup_signal(p->signal);

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