Messages in this thread | | | Date | Mon, 10 Apr 2006 21:43:46 +0400 | From | Oleg Nesterov <> | Subject | Re: [PATCH] fix de_thread() vs do_coredump() deadlock |
| |
On 04/09, Roland McGrath wrote: > > [PATCH] Fix race between exec and fatal signals
I'll try to study this patch carefully tomorrow, but now I have the feeling it is not right (probably my misunderstanding after the quick reading).
> --- a/fs/exec.c > +++ b/fs/exec.c > @@ -606,15 +606,16 @@ static int de_thread(struct task_struct > > ... [snip] ... > > - zap_other_threads(current); > + zap_other_threads(current, SIGNAL_GROUP_EXEC); > > ... [snip] ... > > -void zap_other_threads(struct task_struct *p) > +void zap_other_threads(struct task_struct *p, int flag) > { > struct task_struct *t; > > - p->signal->flags = SIGNAL_GROUP_EXIT; > + if (unlikely(p->signal->flags & SIGNAL_GROUP_EXEC)) { > + /* > + * We are cancelling an exec that is in progress, to let > + * the thread group die instead. We need to wake the > + * exec'ing thread up from uninterruptible wait. > + */ > + BUG_ON(flag != SIGNAL_GROUP_EXIT); > + t = p->signal->group_exit_task; > + p->signal->group_exit_task = NULL; > + p->signal->notify_count = 0; > + wake_up_process(t); > + } > + > + p->signal->flags = flag; > p->signal->group_stop_count = 0;
So, de_thread() sets SIGNAL_GROUP_EXEC and sends SIGKILL to other thereads.
Sub-thread receives the signal, and calls get_signal_to_deliver->do_group_exit. do_group_exit() calls zap_other_threads(SIGNAL_GROUP_EXIT) because there is no SIGNAL_GROUP_EXIT set. zap_other_threads() notices SIGNAL_GROUP_EXEC, wakes up execer, and changes ->signal->flags to SIGNAL_GROUP_EXIT.
de_thread() re-locks sighand, sees !SIGNAL_GROUP_EXEC and goes to 'dying:'.
No?
Another problem. de_thread() sets '->group_exit_task = current' _only_ if 'atomic_read(&sig->count) > count', so wake_up_process(->group_exit_task) in zap_other_threads() is unsafe.
Oleg.
- 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/
| |