[RFC PATCH 8/X] ptrace: introduce ptrace_tracer() helper

[Oleg Nesterov <oleg@redhat.com>]

On 05/25, Oleg Nesterov wrote:
>
> On 05/25, Oleg Nesterov wrote:
> >
> > Move task_struct->parent into ptrace_task->pt_tracer and change the users
> > accordingly.
> >
> > ...
> >
> > @@ -1773,8 +1779,8 @@ static int ptrace_signal(int signr, sigi
> >  		info->si_signo = signr;
> >  		info->si_errno = 0;
> >  		info->si_code = SI_USER;
> > -		info->si_pid = task_pid_vnr(current->parent);
> > -		info->si_uid = task_uid(current->parent);
> > +		info->si_pid = task_pid_vnr(current->ptrace_task->pt_tracer);
> > +		info->si_uid = task_uid(current->ptrace_task->pt_tracer);
>
> This change is wrong, will re-do. The task can be already untraced
> after ptrace_stop().
>
> But is the current code correct? If we are not traced any longer
> si_pid/si_uid are not necessary right either, we should calculate them
> before ptrace_stop(), no?

... and in theory we need rcu_read_lock() here.

I'd like to send the next patch for review, because I have questions.
It fixes the bug above, but of course the previous patch should be
fixed instead. I'll re-send the whole series (with renames), but first
I'd like to know what Roland thinks.

---------------------------------------------------------------------
[RFC PATCH 8/X] ptrace: introduce ptrace_tracer() helper

Introduce ptrace_tracer() (or suggest a better name) to simplify/cleanup
the code which needs the tracer and checks task_ptrace(). From now nobody
else uses ->pt_tracer except ptrace_link/ptrace_unlink.

Question. Note that ptrace_tracer() is equal to tracehook_tracer_task().
But I do not understand the future plans for tracehook_tracer_task().
Should we just use tracehook_tracer_task() ? If yes, how
ptrace_reparented() can use this helper?


 include/linux/ptrace.h |   17 ++++++++++++-----
 kernel/ptrace.c        |    2 +-
 kernel/signal.c        |   28 +++++++++++++++++-----------
 3 files changed, 30 insertions(+), 17 deletions(-)

--- PTRACE/include/linux/ptrace.h~8_PT_TRACER	2009-05-25 22:52:51.000000000 +0200
+++ PTRACE/include/linux/ptrace.h	2009-05-25 23:21:17.000000000 +0200
@@ -116,10 +116,17 @@ static inline int task_ptrace(struct tas
 		task->ptrace_task->pt_flags : 0;
 }
 
-static inline int ptrace_reparented(struct task_struct *child)
+static inline struct task_struct *ptrace_tracer(struct task_struct *task)
 {
-	return unlikely(task_ptrace(child)) &&
-		child->ptrace_task->pt_tracer != child->real_parent;
+	if (task->ptrace_task)
+		return task->ptrace_task->pt_tracer;
+	return NULL;
+}
+
+static inline bool ptrace_reparented(struct task_struct *child)
+{
+	struct task_struct *tracer = ptrace_tracer(child);
+	return unlikely(tracer) && tracer != child->real_parent;
 }
 
 static inline void ptrace_unlink(struct task_struct *child)
@@ -167,9 +174,9 @@ static inline void ptrace_init_task(stru
 	INIT_LIST_HEAD(&child->ptrace_entry);
 	INIT_LIST_HEAD(&child->ptraced);
 
-	if (unlikely(child->ptrace_task) && task_ptrace(current))
+	if (unlikely(child->ptrace_task) && ptrace_tracer(current))
 		ptrace_link(child, task_ptrace(current),
-				current->ptrace_task->pt_tracer);
+				ptrace_tracer(current));
 }
 
 /**
--- PTRACE/kernel/ptrace.c~8_PT_TRACER	2009-05-25 22:52:51.000000000 +0200
+++ PTRACE/kernel/ptrace.c	2009-05-25 23:24:57.000000000 +0200
@@ -108,7 +108,7 @@ int ptrace_check_attach(struct task_stru
 	 * be changed by us so it's not changing right after this.
 	 */
 	read_lock(&tasklist_lock);
-	if (task_ptrace(child) && child->ptrace_task->pt_tracer == current) {
+	if (ptrace_tracer(child) == current) {
 		ret = 0;
 		/*
 		 * child->sighand can't be NULL, release_task()
--- PTRACE/kernel/signal.c~8_PT_TRACER	2009-05-25 22:52:51.000000000 +0200
+++ PTRACE/kernel/signal.c	2009-05-26 00:14:00.000000000 +0200
@@ -1411,9 +1411,8 @@ int do_notify_parent(struct task_struct 
 	BUG_ON(!task_ptrace(tsk) &&
 	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
 
-	if (task_ptrace(tsk))
-		parent = tsk->ptrace_task->pt_tracer;
-	else
+	parent = ptrace_tracer(tsk);
+	if (likely(!parent))
 		parent = tsk->real_parent;
 
 	info.si_signo = sig;
@@ -1489,9 +1488,8 @@ static void do_notify_parent_cldstop(str
 	struct task_struct *parent;
 	struct sighand_struct *sighand;
 
-	if (task_ptrace(tsk))
-		parent = tsk->ptrace_task->pt_tracer;
-	else {
+	parent = ptrace_tracer(tsk);
+	if (likely(!parent)) {
 		tsk = tsk->group_leader;
 		parent = tsk->real_parent;
 	}
@@ -1538,7 +1536,9 @@ static void do_notify_parent_cldstop(str
 
 static inline int may_ptrace_stop(void)
 {
-	if (!likely(task_ptrace(current)))
+	struct task_struct *tracer = ptrace_tracer(current);
+
+	if (!likely(tracer))
 		return 0;
 	/*
 	 * Are we in the middle of do_coredump?
@@ -1550,7 +1550,7 @@ static inline int may_ptrace_stop(void)
 	 * is safe to enter schedule().
 	 */
 	if (unlikely(current->mm->core_state) &&
-	    unlikely(current->mm == current->ptrace_task->pt_tracer->mm))
+	    unlikely(current->mm == tracer->mm))
 		return 0;
 
 	return 1;
@@ -1756,8 +1756,14 @@ static int do_signal_stop(int signr)
 static int ptrace_signal(int signr, siginfo_t *info,
 			 struct pt_regs *regs, void *cookie)
 {
-	if (!task_ptrace(current))
+	struct task_struct *tracer = ptrace_tracer(current);
+	pid_t pid;
+	uid_t uid;
+
+	if (!tracer)
 		return signr;
+	pid = task_pid_vnr(tracer);
+	uid = task_uid(tracer);
 
 	ptrace_signal_deliver(regs, cookie);
 
@@ -1779,8 +1785,8 @@ static int ptrace_signal(int signr, sigi
 		info->si_signo = signr;
 		info->si_errno = 0;
 		info->si_code = SI_USER;
-		info->si_pid = task_pid_vnr(current->ptrace_task->pt_tracer);
-		info->si_uid = task_uid(current->ptrace_task->pt_tracer);
+		info->si_pid = pid;
+		info->si_uid = uid;
 	}
 
 	/* If the (new) signal is now blocked, requeue it.  */