[PATCH 2/3] ptrace: cleanup check/set of PT_PTRACED during attach

[PATCH 2/3] ptrace: cleanup check/set of PT_PTRACED during attach

[Oleg Nesterov]

ptrace_attach() and ptrace_traceme() are the last functions which look as
if the untraced task can have task->ptrace != 0, this must not be possible.
Change the code to just check ->ptrace != 0 and s/|=/=/ to set PT_PTRACED.

Also, a couple of trivial whitespace cleanups in ptrace_attach().

And 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 |  102 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 51 insertions(+), 51 deletions(-)

--- PTRACE/kernel/ptrace.c~2_PTRACE	2009-05-05 23:17:53.000000000 +0200
+++ PTRACE/kernel/ptrace.c	2009-05-05 23:49:15.000000000 +0200
@@ -186,12 +186,12 @@ int ptrace_attach(struct task_struct *ta
 		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;
 repeat:
 	/*
@@ -219,10 +219,10 @@ repeat:
 	retval = -EPERM;
 	if (unlikely(task->exit_state))
 		goto bad;
-	if (task->ptrace & PT_PTRACED)
+	if (task->ptrace)
 		goto bad;
 
-	task->ptrace |= PT_PTRACED;
+	task->ptrace = PT_PTRACED;
 	if (capable(CAP_SYS_PTRACE))
 		task->ptrace |= PT_PTRACE_CAP;
 
@@ -238,6 +238,52 @@ 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;
+
+	/*
+	 * Are we already being traced?
+	 */
+repeat:
+	task_lock(current);
+	if (!current->ptrace) {
+		/*
+		 * 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;
+}
+
 /*
  * Called with irqs disabled, returns true if childs should reap themselves.
  */
@@ -575,52 +621,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/3] ptrace: cleanup check/set of PT_PTRACED during attach

[Roland McGrath]

Acked-by: Roland McGrath <roland@redhat.com>

Re: [PATCH 2/3] ptrace: cleanup check/set of PT_PTRACED during attach

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> ptrace_attach() and ptrace_traceme() are the last functions which 
> look as if the untraced task can have task->ptrace != 0, this must 
> not be possible. Change the code to just check ->ptrace != 0 and 
> s/|=/=/ to set PT_PTRACED.
> 
> Also, a couple of trivial whitespace cleanups in ptrace_attach().
> 
> And move ptrace_traceme() up near ptrace_attach() to keep them 
> close to each other.

btw., while at it, please also fix the typos in 
include/linux/ptrace.h's PT_* flags section:

/*
 * Ptrace flags
 *
 * The owner ship rules for task->ptrace which holds the ptrace
 * flags is simple.  When a task is running it owns it's task->ptrace
 * flags.  When the a task is stopped the ptracer owns task->ptrace.
 */

s/owner ship/ownership
s/it's/its

Thanks,

	Ingo

Re: [PATCH 2/3] ptrace: cleanup check/set of PT_PTRACED during attach

[Oleg Nesterov]

On 05/06, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > ptrace_attach() and ptrace_traceme() are the last functions which 
> > look as if the untraced task can have task->ptrace != 0, this must 
> > not be possible. Change the code to just check ->ptrace != 0 and 
> > s/|=/=/ to set PT_PTRACED.
> >
> > Also, a couple of trivial whitespace cleanups in ptrace_attach().
> >
> > And move ptrace_traceme() up near ptrace_attach() to keep them 
> > close to each other.
>
> btw., while at it, please also fix the typos in 
> include/linux/ptrace.h's PT_* flags section:
>
> /*
>  * Ptrace flags
>  *
>  * The owner ship rules for task->ptrace which holds the ptrace
>  * flags is simple.  When a task is running it owns it's task->ptrace
>  * flags.  When the a task is stopped the ptracer owns task->ptrace.
>  */
>
> s/owner ship/ownership
> s/it's/its

Yes, thanks.

We should change this comment anyway, because it is not right.

The only case when a task owns (iow, can change it safely) its ->ptrace
is: it is running _and_ traced. I think this is what the comment tried
to say.

But this doesn't really matter, because afaics the correct comment
should say: the task should never touch its ->ptrace, ptracer always
owns it.

There is only one exception afaics, de_thread() or do_wait() can call
release_task()->ptrace_unlink() and clear ->ptrace on behalve of
another (not ptracer) task.

Roland, what do you think?

Oleg.

Re: [PATCH 2/3] ptrace: cleanup check/set of PT_PTRACED during attach

[Roland McGrath]

> The only case when a task owns (iow, can change it safely) its ->ptrace
> is: it is running _and_ traced. I think this is what the comment tried
> to say.

I think so.  I suspect it only ever referred to now-obsolete uses like
PT_DTRACE fiddling.

> But this doesn't really matter, because afaics the correct comment
> should say: the task should never touch its ->ptrace, ptracer always
> owns it.

To be clear, it should say something about how two potential ptracers
exclude each other touching it.

> There is only one exception afaics, de_thread() or do_wait() can call
> release_task()->ptrace_unlink() and clear ->ptrace on behalve of
> another (not ptracer) task.

And ptrace_traceme().  Both of these I'd call "on behalf of the tracer",
and that is close enough to "tracer owns it" if their rules are explained.
(That in contrast to the old comment's suggestion that the tracee could
touch its own unlocked.)


Thanks,
Roland