Re: [PATCH 2/2] ptrace: do not use task_lock() for attach

[Oleg Nesterov <oleg@redhat.com>]

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.