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

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

[Oleg Nesterov]

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.

However ptrace_attach() calls __ptrace_may_access() and needs task_lock()
to pin task->mm for get_dumpable(). But we can call __ptrace_may_access()
before we take tasklist_lock, ->cred_exec_mutex protects us against
do_execve() path which can change creds and MMF_DUMP* flags.

(ugly, but we can't use ptrace_may_access() because it hides the error
 code, so we have to take task_lock() and use __ptrace_may_access()).

Also, kill "if (!task->mm)" check. It buys nothing, we can attach to the
task right before it does exit_mm(). Instead, add PF_KTHREAD check to
prevent attaching to the kernel thread with a borrowed ->mm.

What we need is to make sure we can't attach after exit_notify(), check
task->exit_state.

And finally, move ptrace_traceme() up near ptrace_attach() to keep them
close to each other.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/ptrace.c |  127 ++++++++++++++++++++------------------------------------
 1 file changed, 47 insertions(+), 80 deletions(-)

--- PTRACE/kernel/ptrace.c~2_ATTACH	2009-05-03 19:30:15.000000000 +0200
+++ PTRACE/kernel/ptrace.c	2009-05-03 19:57:11.000000000 +0200
@@ -177,66 +177,79 @@ bool ptrace_may_access(struct task_struc
 int ptrace_attach(struct task_struct *task)
 {
 	int retval;
-	unsigned long flags;
 
 	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;
+	/*
+	 * 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)
+	if (retval < 0)
 		goto out;
 
-	retval = -EPERM;
-repeat:
-	/*
-	 * Nasty, nasty.
-	 *
-	 * We want to hold both the task-lock and the
-	 * tasklist_lock for writing at the same time.
-	 * But that's against the rules (tasklist_lock
-	 * is taken for reading by interrupts on other
-	 * cpu's that may have task_lock).
-	 */
 	task_lock(task);
-	if (!write_trylock_irqsave(&tasklist_lock, flags)) {
-		task_unlock(task);
-		do {
-			cpu_relax();
-		} while (!write_can_lock(&tasklist_lock));
-		goto repeat;
-	}
-
-	if (!task->mm)
-		goto bad;
-	/* the same process cannot be attached many times */
-	if (task->ptrace & PT_PTRACED)
-		goto bad;
 	retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
+	task_unlock(task);
 	if (retval)
-		goto bad;
+		goto unlock_creds;
 
-	/* Go */
-	task->ptrace |= PT_PTRACED;
+	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);
-bad:
-	write_unlock_irqrestore(&tasklist_lock, flags);
-	task_unlock(task);
+unlock_tasklist:
+	write_unlock_irq(&tasklist_lock);
+unlock_creds:
 	mutex_unlock(&task->cred_exec_mutex);
 out:
 	return retval;
 }
 
+/**
+ * ptrace_traceme  --  helper for PTRACE_TRACEME
+ *
+ * Performs checks and sets PT_PTRACED.
+ * Should be used by all ptrace implementations for PTRACE_TRACEME.
+ */
+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;
+}
+
 /*
  * Called with irqs disabled, returns true if childs should reap themselves.
  */
@@ -574,52 +587,6 @@ int ptrace_request(struct task_struct *c
 }
 
 /**
- * ptrace_traceme  --  helper for PTRACE_TRACEME
- *
- * Performs checks and sets PT_PTRACED.
- * Should be used by all ptrace implementations for PTRACE_TRACEME.
- */
-int ptrace_traceme(void)
-{
-	int ret = -EPERM;
-
-	/*
-	 * Are we already being traced?
-	 */
-repeat:
-	task_lock(current);
-	if (!(current->ptrace & PT_PTRACED)) {
-		/*
-		 * See ptrace_attach() comments about the locking here.
-		 */
-		unsigned long flags;
-		if (!write_trylock_irqsave(&tasklist_lock, flags)) {
-			task_unlock(current);
-			do {
-				cpu_relax();
-			} while (!write_can_lock(&tasklist_lock));
-			goto repeat;
-		}
-
-		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_irqrestore(&tasklist_lock, flags);
-	}
-	task_unlock(current);
-	return ret;
-}
-
-/**
  * ptrace_get_task_struct  --  grab a task struct reference for ptrace
  * @pid:       process id to grab a task_struct reference of
  *

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

[Oleg Nesterov]

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.

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

[Roland McGrath]

This looks good to me overall.  It might be worth slicing it into two or
more patches, just for bisect paranoia.  (e.g. PF_KTHREAD; task_lock in
ptrace_attach; task_lock in ptrace_traceme.)

I think it merits a comment that the PF_KTHREAD check does not need any
interlock because daemonize() will detach ptrace via reparent_to_kthreadd()
after it sets PF_KTHREAD.  (vs the old ->mm check under task_lock.)

It is worth noting that this changes the security_ptrace_traceme() call so
it's no longer under task_lock().  I can't see any way the LSM hooks care,
but it is a change.

You also didn't mention the s/|=/=/ changes.  Those are correct, we've
already agreed, but the commit log should mention that this subtle change
was intentional.


Thanks,
Roland

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

[Oleg Nesterov]

On 05/04, Roland McGrath wrote:
>
> This looks good to me overall.  It might be worth slicing it into two or
> more patches, just for bisect paranoia.  (e.g. PF_KTHREAD; task_lock in
> ptrace_attach; task_lock in ptrace_traceme.)

OK,

> I think it merits a comment that the PF_KTHREAD check does not need any
> interlock because daemonize() will detach ptrace via reparent_to_kthreadd()
> after it sets PF_KTHREAD.  (vs the old ->mm check under task_lock.)

Agreed, but actually the patch doesn't make the difference wrt daemonize().
currently ptrace_attach() can take task_lock() just before daemonize() calls
exit_mm().

> It is worth noting that this changes the security_ptrace_traceme() call so
> it's no longer under task_lock().  I can't see any way the LSM hooks care,
> but it is a change.

Yes, good point.

> You also didn't mention the s/|=/=/ changes.  Those are correct, we've
> already agreed, but the commit log should mention that this subtle change
> was intentional.

Yes! Forgot to mention, thanks.

Oleg.