On 05/03, Oleg Nesterov wrote:
>
> Remove the "Nasty, nasty" lock dance in ptrace_attach()/ptrace_traceme().
> From now task_lock() has nothing to do with ptrace at all.
>
> With the recent changes nobody uses task_lock() to serialize with ptrace,
> but in fact it was never needed and it was never used consistently.
arch/um still uses task_lock() to clear PT_DTRACE after exec, but this
should be fixed anyway.
UML shouldn't use PT_DTRACE at all, and nobody except ptrace should
change ptrace flags. arch/um/kernel/exec.c:execve1() is just buggy.
For example, it can race with exit_ptrace()->__ptrace_unlink() and
leak PT_ flags on untraced task.
Jeff, what do you think about the patch I sent you a week ago?
> kernel/ptrace.c | 127
> ++++++++++++++++++++------------------------------------
> 1 file changed, 47 insertions(+), 80 deletions(-)
To simplify the review I am attaching the code with this patch applied,
int ptrace_attach(struct task_struct *task)
{
int retval;
audit_ptrace(task);
retval = -EPERM;
if (unlikely(task->flags & PF_KTHREAD))
goto out;
if (same_thread_group(task, current))
goto out;
/*
* Protect exec's credential calculations against our
interference;
* SUID, SGID and LSM creds get determined differently under
ptrace.
*/
retval = mutex_lock_interruptible(&task->cred_exec_mutex);
if (retval < 0)
goto out;
task_lock(task);
retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
task_unlock(task);
if (retval)
goto unlock_creds;
write_lock_irq(&tasklist_lock);
retval = -EPERM;
if (unlikely(task->exit_state))
goto unlock_tasklist;
if (task->ptrace)
goto unlock_tasklist;
task->ptrace = PT_PTRACED;
if (capable(CAP_SYS_PTRACE))
task->ptrace |= PT_PTRACE_CAP;
__ptrace_link(task, current);
send_sig_info(SIGSTOP, SEND_SIG_FORCED, task);
unlock_tasklist:
write_unlock_irq(&tasklist_lock);
unlock_creds:
mutex_unlock(&task->cred_exec_mutex);
out:
return retval;
}
int ptrace_traceme(void)
{
int ret = -EPERM;
write_lock_irq(&tasklist_lock);
/* Are we already being traced? */
if (!current->ptrace) {
ret = security_ptrace_traceme(current->parent);
/*
* Check PF_EXITING to ensure ->real_parent has not
passed
* exit_ptrace(). Otherwise we don't report the error
but
* pretend ->real_parent untraces us right after return.
*/
if (!ret && !(current->real_parent->flags &
PF_EXITING)) {
current->ptrace = PT_PTRACED;
__ptrace_link(current, current->real_parent);
}
}
write_unlock_irq(&tasklist_lock);
return ret;
}
Oleg.
