On Thu, 10 Jun 2021 09:31:42 +0200 Bernd Edlinger bernd.edlinger@hotmail.de wrote:
This introduces signal->unsafe_execve_in_progress, which is used to fix the case when at least one of the sibling threads is traced, and therefore the trace process may dead-lock in ptrace_attach, but de_thread will need to wait for the tracer to continue execution.
The solution is to detect this situation and allow ptrace_attach to continue, while de_thread() is still waiting for traced zombies to be eventually released. When the current thread changed the ptrace status from non-traced to traced, we can simply abort the whole execve and restart it by returning -ERESTARTSYS. This needs to be done before changing the thread leader, because the PTRACE_EVENT_EXEC needs to know the old thread pid.
Although it is technically after the point of no return, we just have to reset bprm->point_of_no_return here, since at this time only the other threads have received a fatal signal, not the current thread.
From the user's point of view the whole execve was
simply delayed until after the ptrace_attach.
Other threads die quickly since the cred_guard_mutex is released, but a deadly signal is already pending. In case the mutex_lock_killable misses the signal, ->unsafe_execve_in_progress makes sure they release the mutex immediately and return with -ERESTARTNOINTR.
This means there is no API change, unlike the previous version of this patch which was discussed here:
https://lore.kernel.org/lkml/b6537ae6-31b1-5c50-f32b-8b8332ace882@hotmail.de...
See tools/testing/selftests/ptrace/vmaccess.c for a test case that gets fixed by this change.
Note that since the test case was originally designed to test the ptrace_attach returning an error in this situation, the test expectation needed to be adjusted, to allow the API to succeed at the first attempt.
Here's the diff from v8. It's conventional to tell reviewers what changed when sending out a new version.
What changed in this version?
--- a/fs/exec.c~exec-fix-dead-lock-in-de_thread-with-ptrace_attach-v9 +++ a/fs/exec.c @@ -1056,29 +1056,31 @@ static int de_thread(struct task_struct return -EAGAIN; }
- while_each_thread(tsk, t) { - if (unlikely(t->ptrace) && t != tsk->group_leader) - sig->unsafe_execve_in_progress = true; - } - sig->group_exit_task = tsk; sig->notify_count = zap_other_threads(tsk); if (!thread_group_leader(tsk)) sig->notify_count--; - spin_unlock_irq(lock);
- if (unlikely(sig->unsafe_execve_in_progress)) + while_each_thread(tsk, t) { + if (unlikely(t->ptrace) && t != tsk->group_leader) + sig->unsafe_execve_in_progress = true; + } + + if (unlikely(sig->unsafe_execve_in_progress)) { + spin_unlock_irq(lock); mutex_unlock(&sig->cred_guard_mutex); + spin_lock_irq(lock); + }
- for (;;) { - set_current_state(TASK_KILLABLE); - if (!sig->notify_count) - break; + while (sig->notify_count) { + __set_current_state(TASK_KILLABLE); + spin_unlock_irq(lock); schedule(); if (__fatal_signal_pending(tsk)) goto killed; + spin_lock_irq(lock); } - __set_current_state(TASK_RUNNING); + spin_unlock_irq(lock);
if (unlikely(sig->unsafe_execve_in_progress)) { if (mutex_lock_killable(&sig->cred_guard_mutex)) _