lkml.org 
[lkml]   [2006]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
Subject[PATCH] fix kill_proc_info() vs copy_process() race
kill_proc_info() takes taskllist_lock only for sig_kernel_stop()
case nowadays. I beleive it races with copy_process().

The first race is simple, copy_process:

/*
* Check for pending SIGKILL! The new thread should not be allowed
* to slip out of an OOM kill. (or normal SIGKILL.)
*/
if (sigismember(&current->pending.signal, SIGKILL))
return EINTR;

This relies on tasklist_lock and is racy now.


The second race is more tricky, copy_process:

attach_pid(p, PIDTYPE_PID, p->pid);
attach_pid(p, PIDTYPE_TGID, p->tgid);

This means that we can find a task in kill_proc_info()->find_task_by_pid()
which is not registered as part of thread group yet. Various bad things can
happen, note that handle_stop_signal(SIGCONT) and __group_complete_signal()
iterate over threads list. But p->pids[PIDTYPE_TGID] is a copy of current's
'struct pid' from dup_task_struct(), and if we don't have CLONE_THREAD here
we will use completely unreleated (parent's) thread list.

I think we can solve these problems by enlarging a ->siglock's scope in
copy_process(), but I don't know how to test this patch.

NOTE: release_task()->__unhash_process() path is safe, we already have
->sighand == NULL while detaching PIDTYPE_PID/PIDTYPE_TGID nonatomically.

Unless I missed something, I personally think this is a 2.6.16 material.

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

--- RC-1/kernel/fork.c~ 2006-02-06 22:04:40.000000000 +0300
+++ RC-1/kernel/fork.c 2006-02-06 22:11:51.000000000 +0300
@@ -1050,7 +1050,7 @@ static task_t *copy_process(unsigned lon
sched_fork(p, clone_flags);

/* Need tasklist lock for parent etc handling! */
- write_lock_irq(&tasklist_lock);
+ write_lock(&tasklist_lock);

/*
* The task hasn't been attached yet, so its cpus_allowed mask will
@@ -1066,33 +1066,34 @@ static task_t *copy_process(unsigned lon
!cpu_online(task_cpu(p))))
set_task_cpu(p, smp_processor_id());

+ /* CLONE_PARENT re-uses the old parent */
+ if (clone_flags & (CLONE_PARENT|CLONE_THREAD))
+ p->real_parent = current->real_parent;
+ else
+ p->real_parent = current;
+ p->parent = p->real_parent;
+
+ spin_lock_irq(&current->sighand->siglock);
/*
* Check for pending SIGKILL! The new thread should not be allowed
* to slip out of an OOM kill. (or normal SIGKILL.)
*/
if (sigismember(&current->pending.signal, SIGKILL)) {
- write_unlock_irq(&tasklist_lock);
+ spin_unlock_irq(&current->sighand->siglock);
+ write_unlock(&tasklist_lock);
retval = -EINTR;
goto bad_fork_cleanup_namespace;
}

- /* CLONE_PARENT re-uses the old parent */
- if (clone_flags & (CLONE_PARENT|CLONE_THREAD))
- p->real_parent = current->real_parent;
- else
- p->real_parent = current;
- p->parent = p->real_parent;
-
if (clone_flags & CLONE_THREAD) {
- spin_lock(&current->sighand->siglock);
/*
* Important: if an exit-all has been started then
* do not create this new thread - the whole thread
* group is supposed to exit anyway.
*/
if (current->signal->flags & SIGNAL_GROUP_EXIT) {
- spin_unlock(&current->sighand->siglock);
- write_unlock_irq(&tasklist_lock);
+ spin_unlock_irq(&current->sighand->siglock);
+ write_unlock(&tasklist_lock);
retval = -EAGAIN;
goto bad_fork_cleanup_namespace;
}
@@ -1122,8 +1123,6 @@ static task_t *copy_process(unsigned lon
*/
p->it_prof_expires = jiffies_to_cputime(1);
}
-
- spin_unlock(&current->sighand->siglock);
}

/*
@@ -1151,7 +1150,9 @@ static task_t *copy_process(unsigned lon

nr_threads++;
total_forks++;
- write_unlock_irq(&tasklist_lock);
+ spin_unlock_irq(&current->sighand->siglock);
+ write_unlock(&tasklist_lock);
+
proc_fork_connector(p);
return p;
-
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/
\
 
 \ /
  Last update: 2006-02-06 16:33    [W:0.098 / U:0.488 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site