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
SubjectRe: [PATCH] fix kill_proc_info() vs copy_process() race
Oleg Nesterov wrote:
>
> Sorry, I was wrong. Without CLONE_THREAD current->sighand.siglock can't help,
> we need p->sighand.siglock, I beleive.

Also, it is stupid to do write_lock(&tasklist_lock) without clearing irqs.

Ok, may be something like (untested, for review only) patch below ?

attach_pid() does wmb(), group_send_sig_info()->rcu_dereference() calls
rmb(), so we can just reverse PIDTYPE_PID/PIDTYPE_TGID attaching?

Note that now we check sigismember(->pending, SIGKILL) holding both
tasklist and ->sighand.siglock, this means we can kill SIGNAL_GROUP_EXIT
check under 'if (clone_flags & CLONE_THREAD)':

__group_complete_signal() and zap_other_threads() need at least
->sighand.siglock and they send SIGKILL without unlocking.

We can miss SIGNAL_GROUP_EXIT from do_coredump(), but it is possible
anyway. The new thread will be killed later, from zap_threads().

What do you think?

Oleg.

--- RC-1/kernel/fork.c~ 2006-02-07 01:41:14.000000000 +0300
+++ RC-1/kernel/fork.c 2006-02-07 02:13:10.000000000 +0300
@@ -1066,11 +1066,13 @@ static task_t *copy_process(unsigned lon
!cpu_online(task_cpu(p))))
set_task_cpu(p, smp_processor_id());

+ spin_lock(&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)) {
+ spin_unlock(&current->sighand->siglock);
write_unlock_irq(&tasklist_lock);
retval = -EINTR;
goto bad_fork_cleanup_namespace;
@@ -1084,18 +1086,6 @@ static task_t *copy_process(unsigned lon
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);
- retval = -EAGAIN;
- goto bad_fork_cleanup_namespace;
- }
p->group_leader = current->group_leader;

if (current->signal->group_stop_count > 0) {
@@ -1122,8 +1112,6 @@ static task_t *copy_process(unsigned lon
*/
p->it_prof_expires = jiffies_to_cputime(1);
}
-
- spin_unlock(&current->sighand->siglock);
}

/*
@@ -1135,8 +1123,8 @@ static task_t *copy_process(unsigned lon
if (unlikely(p->ptrace & PT_PTRACED))
__ptrace_link(p, current->parent);

- attach_pid(p, PIDTYPE_PID, p->pid);
attach_pid(p, PIDTYPE_TGID, p->tgid);
+ attach_pid(p, PIDTYPE_PID, p->pid);
if (thread_group_leader(p)) {
p->signal->tty = current->signal->tty;
p->signal->pgrp = process_group(current);
@@ -1149,6 +1137,7 @@ static task_t *copy_process(unsigned lon

nr_threads++;
total_forks++;
+ spin_unlock(&current->sighand->siglock);
write_unlock_irq(&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 20:10    [W:0.061 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site