Oleg Nesterov [off-list ref] writes:
On 03/03, Eric W. Biederman wrote:
quoted
@@ -1065,11 +1065,8 @@ static int de_thread(struct task_struct *tsk)
}
sig->group_exit_task = tsk;
- sig->notify_count = zap_other_threads(tsk);
- if (!thread_group_leader(tsk))
- sig->notify_count--;
-
- while (sig->notify_count) {
+ zap_other_threads(tsk);
+ while (atomic_read(&sig->live) > 1) {
__set_current_state(TASK_KILLABLE);
spin_unlock_irq(lock);
schedule();
Very nice. So de_thread() returns as soon as all other threads decrement
signal->live in do_exit(). Before they do, say, exit_mm(). This is already
wrong, for example this breaks OOM. Plus a lot more problems afaics, but
lets ignore this.
Which means that we need to keep sig->notify_count.
Note that de_thread() also unshares ->sighand before return. So in the
case of mt exec it will likely see oldsighand->count != 1 and alloc the
new sighand_struct and this breaks the locking.
Because the execing thread will use newsighand->siglock to protect its
signal_struct while the zombie threads will use oldsighand->siglock to
protect the same signal struct. Yes, tasklist_lock + the fact irq_disable
implies rcu_lock mostly save us but not entirely, say, a foreign process
doing __send_signal() can take the right or the wrong lock depending on
/dev/random.
Which leads to the question how can we get back tot he 2.4 behavior
of freeing sighand_struct in do_exit?
At which point as soon as we free sighand_struct if we are the last
to dying thread notify de_thread and everything works.
There are only two uses of sighand->siglock that I can see in
release_task: __ptrace_unlink and __exit_signal.
For what __ptrace_unlink is doing we should just be able to skip
acquiring of siglock if PF_EXITING is set.
__exit_signal is a little more interesting but half of what it is
doing looks like it was pulled out of do_exit and just needs to
be put back.
Which probably adds up to 4 or 5 small carefully written patches to sort
out that part of the exit path, but I think it leads to a very simple
and straight forward result. With the only side effect that we
can exec a process and still have a debugger reaping zombie threads.
Oleg does that sound reasonable to you?
Eric
On 03/30, Eric W. Biederman wrote:
Oleg Nesterov [off-list ref] writes:
quoted
Very nice. So de_thread() returns as soon as all other threads decrement
signal->live in do_exit(). Before they do, say, exit_mm(). This is already
wrong, for example this breaks OOM. Plus a lot more problems afaics, but
lets ignore this.
Which means that we need to keep sig->notify_count.
Yes, although we need to make it less ugly.
quoted
Note that de_thread() also unshares ->sighand before return. So in the
case of mt exec it will likely see oldsighand->count != 1 and alloc the
new sighand_struct and this breaks the locking.
Because the execing thread will use newsighand->siglock to protect its
signal_struct while the zombie threads will use oldsighand->siglock to
protect the same signal struct. Yes, tasklist_lock + the fact irq_disable
implies rcu_lock mostly save us but not entirely, say, a foreign process
doing __send_signal() can take the right or the wrong lock depending on
/dev/random.
Which leads to the question how can we get back tot he 2.4 behavior
of freeing sighand_struct in do_exit?
At which point as soon as we free sighand_struct if we are the last
to dying thread notify de_thread and everything works.
I was thinking about the similar option, see below, but decided that we
should not do this at least right now.
For what __ptrace_unlink is doing we should just be able to skip
acquiring of siglock if PF_EXITING is set.
We can even remove it from release_task() path, this is simple.
__exit_signal is a little more interesting but half of what it is
doing looks like it was pulled out of do_exit and just needs to
be put back.
That is. I think we should actually unhash the exiting sub-thread even
if it is traced. IOW, remove it from thread/pid/parent/etc lists and
nullify its ->sighand. IMO, whatever we do thread_group_empty(current)
should be true after exec.
So the exiting sub-trace should look almost a EXIT_DEAD task except it
still should report to debugger.
But this is dangerous. Say, wait4(upid <= 0) becomes unsafe because
task_pid_type(PIDTYPE_PGID) won't work.
Which probably adds up to 4 or 5 small carefully written patches to sort
out that part of the exit path,
Perhaps I am wrong, but I think you underestimate the problems, and it is
not clear to me if we really want this.
=========================================================================
Anyway, Eric, even if we can and want to do this, why we can't do this on
top of my fix?
I simply fail to understand why you dislike it that much. Yes it is not
pretty, I said this many times, but it is safe in that it doesn't really
change the current behaviour.
I am much more worried about 2/2 you didn't argue with, this patch _can_
break something and this is obviously not good even if PTRACE_EVENT_EXIT
was always broken.
Oleg.