[RFC PATCH 7/X] ptrace: mv task->parent ptrace_task->pt_tracer

[RFC PATCH 7/X] ptrace: mv task->parent ptrace_task->pt_tracer

[Oleg Nesterov]

Move task_struct->parent into ptrace_task->pt_tracer and change the users
accordingly.

The next patches will add the helper to simplify/cleanup the usage of
->pt_tracer, and unify do_notify_parent/do_notify_parent_cldstop.

 include/linux/sched.h     |    8 +-------
 include/linux/init_task.h |    1 -
 include/linux/ptrace.h    |   18 ++++++++++--------
 include/linux/tracehook.h |    4 ++--
 kernel/ptrace.c           |    8 ++++----
 kernel/exit.c             |   18 +++++++-----------
 kernel/signal.c           |   24 +++++++++++++++---------
 7 files changed, 39 insertions(+), 42 deletions(-)

--- PTRACE/include/linux/sched.h~7_MV_PARENT	2009-05-24 23:10:47.000000000 +0200
+++ PTRACE/include/linux/sched.h	2009-05-25 00:17:28.000000000 +0200
@@ -1186,13 +1186,7 @@ struct task_struct {
 	/* Canary value for the -fstack-protector gcc feature */
 	unsigned long stack_canary;
 
-	/* 
-	 * pointers to (original) parent process, youngest child, younger sibling,
-	 * older sibling, respectively.  (p->father can be replaced with 
-	 * p->real_parent->pid)
-	 */
-	struct task_struct *real_parent; /* real parent process */
-	struct task_struct *parent; /* recipient of SIGCHLD, wait4() reports */
+	struct task_struct *real_parent; /* parent process */
 	/*
 	 * children/sibling forms the list of my natural children
 	 */
--- PTRACE/include/linux/init_task.h~7_MV_PARENT	2009-04-15 12:53:58.000000000 +0200
+++ PTRACE/include/linux/init_task.h	2009-05-25 01:30:35.000000000 +0200
@@ -139,7 +139,6 @@ extern struct cred init_cred;
 	.ptraced	= LIST_HEAD_INIT(tsk.ptraced),			\
 	.ptrace_entry	= LIST_HEAD_INIT(tsk.ptrace_entry),		\
 	.real_parent	= &tsk,						\
-	.parent		= &tsk,						\
 	.children	= LIST_HEAD_INIT(tsk.children),			\
 	.sibling	= LIST_HEAD_INIT(tsk.sibling),			\
 	.group_leader	= &tsk,						\
--- PTRACE/include/linux/ptrace.h~7_MV_PARENT	2009-05-24 23:38:10.000000000 +0200
+++ PTRACE/include/linux/ptrace.h	2009-05-25 00:27:38.000000000 +0200
@@ -76,7 +76,8 @@
 #include <linux/sched.h>		/* For struct task_struct.  */
 
 struct ptrace_task {
-	unsigned long	pt_flags;
+	unsigned long		pt_flags;
+	struct task_struct	*pt_tracer;
 };
 
 extern int alloc_ptrace_task(struct task_struct *child);
@@ -103,11 +104,6 @@ extern int __ptrace_may_access(struct ta
 /* Returns true on success, false on denial. */
 extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);
 
-static inline int ptrace_reparented(struct task_struct *child)
-{
-	return child->real_parent != child->parent;
-}
-
 /**
  * task_ptrace - return %PT_* flags that apply to a task
  * @task:	pointer to &task_struct in question
@@ -120,6 +116,12 @@ static inline int task_ptrace(struct tas
 		task->ptrace_task->pt_flags : 0;
 }
 
+static inline int ptrace_reparented(struct task_struct *child)
+{
+	return unlikely(task_ptrace(child)) &&
+		child->ptrace_task->pt_tracer != child->real_parent;
+}
+
 static inline void ptrace_unlink(struct task_struct *child)
 {
 	if (unlikely(task_ptrace(child)))
@@ -164,10 +166,10 @@ static inline void ptrace_init_task(stru
 {
 	INIT_LIST_HEAD(&child->ptrace_entry);
 	INIT_LIST_HEAD(&child->ptraced);
-	child->parent = child->real_parent;
 
 	if (unlikely(child->ptrace_task) && task_ptrace(current))
-		ptrace_link(child, task_ptrace(current), current->parent);
+		ptrace_link(child, task_ptrace(current),
+				current->ptrace_task->pt_tracer);
 }
 
 /**
--- PTRACE/include/linux/tracehook.h~7_MV_PARENT	2009-05-24 22:15:08.000000000 +0200
+++ PTRACE/include/linux/tracehook.h	2009-05-25 01:51:11.000000000 +0200
@@ -171,8 +171,8 @@ static inline int tracehook_unsafe_exec(
  */
 static inline struct task_struct *tracehook_tracer_task(struct task_struct *tsk)
 {
-	if (task_ptrace(tsk) & PT_PTRACED)
-		return rcu_dereference(tsk->parent);
+	if (tsk->ptrace_task)
+		return rcu_dereference(tsk->ptrace_task->pt_tracer);
 	return NULL;
 }
 
--- PTRACE/kernel/ptrace.c~7_MV_PARENT	2009-05-25 00:03:45.000000000 +0200
+++ PTRACE/kernel/ptrace.c	2009-05-25 00:41:10.000000000 +0200
@@ -47,7 +47,7 @@ void ptrace_link(struct task_struct *chi
 
 	BUG_ON(!list_empty(&child->ptrace_entry));
 	list_add(&child->ptrace_entry, &tracer->ptraced);
-	child->parent = tracer;
+	child->ptrace_task->pt_tracer = tracer;
 }
 
 /*
@@ -85,7 +85,7 @@ void __ptrace_unlink(struct task_struct 
 	BUG_ON(!task_ptrace(child));
 
 	child->ptrace_task->pt_flags = 0;
-	child->parent = child->real_parent;
+	child->ptrace_task->pt_tracer = NULL;
 	list_del_init(&child->ptrace_entry);
 
 	arch_ptrace_untrace(child);
@@ -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->parent == current) {
+	if (task_ptrace(child) && child->ptrace_task->pt_tracer == current) {
 		ret = 0;
 		/*
 		 * child->sighand can't be NULL, release_task()
@@ -259,7 +259,7 @@ int ptrace_traceme(void)
 	write_lock_irq(&tasklist_lock);
 	/* Are we already being traced? */
 	if (!task_ptrace(current)) {
-		ret = security_ptrace_traceme(current->parent);
+		ret = security_ptrace_traceme(current->real_parent);
 		/*
 		 * Check PF_EXITING to ensure ->real_parent has not passed
 		 * exit_ptrace(). Otherwise we don't report the error but
--- PTRACE/kernel/exit.c~7_MV_PARENT	2009-05-20 16:00:52.000000000 +0200
+++ PTRACE/kernel/exit.c	2009-05-25 00:49:27.000000000 +0200
@@ -336,7 +336,7 @@ static void reparent_to_kthreadd(void)
 
 	ptrace_unlink(current);
 	/* Reparent to init */
-	current->real_parent = current->parent = kthreadd_task;
+	current->real_parent = kthreadd_task;
 	list_move_tail(&current->sibling, &current->real_parent->children);
 
 	/* Set the exit signal to SIGCHLD so we signal init on exit */
@@ -780,10 +780,6 @@ static void forget_original_parent(struc
 
 	list_for_each_entry_safe(p, n, &father->children, sibling) {
 		p->real_parent = reaper;
-		if (p->parent == father) {
-			BUG_ON(task_ptrace(p));
-			p->parent = p->real_parent;
-		}
 		reparent_thread(father, p, &dead_children);
 	}
 	write_unlock_irq(&tasklist_lock);
@@ -1210,17 +1206,17 @@ static int wait_task_zombie(struct wait_
 		 * p->signal fields, because they are only touched by
 		 * __exit_signal, which runs with tasklist_lock
 		 * write-locked anyway, and so is excluded here.  We do
-		 * need to protect the access to p->parent->signal fields,
-		 * as other threads in the parent group can be right
-		 * here reaping other children at the same time.
+		 * need to protect the access to p->real_parent->signal
+		 * fields, as other threads in the parent group can be
+		 * right here reaping other children at the same time.
 		 *
 		 * We use thread_group_cputime() to get times for the thread
 		 * group, which consolidates times for all threads in the
 		 * group including the group leader.
 		 */
 		thread_group_cputime(p, &cputime);
-		spin_lock_irq(&p->parent->sighand->siglock);
-		psig = p->parent->signal;
+		spin_lock_irq(&p->real_parent->sighand->siglock);
+		psig = p->real_parent->signal;
 		sig = p->signal;
 		psig->cutime =
 			cputime_add(psig->cutime,
@@ -1251,7 +1247,7 @@ static int wait_task_zombie(struct wait_
 			sig->oublock + sig->coublock;
 		task_io_accounting_add(&psig->ioac, &p->ioac);
 		task_io_accounting_add(&psig->ioac, &sig->ioac);
-		spin_unlock_irq(&p->parent->sighand->siglock);
+		spin_unlock_irq(&p->real_parent->sighand->siglock);
 	}
 
 	/*
--- PTRACE/kernel/signal.c~7_MV_PARENT	2009-04-29 15:45:28.000000000 +0200
+++ PTRACE/kernel/signal.c	2009-05-25 01:22:58.000000000 +0200
@@ -1399,6 +1399,7 @@ int do_notify_parent(struct task_struct 
 {
 	struct siginfo info;
 	unsigned long flags;
+	struct task_struct *parent;
 	struct sighand_struct *psig;
 	int ret = sig;
 
@@ -1410,6 +1411,11 @@ 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 = tsk->real_parent;
+
 	info.si_signo = sig;
 	info.si_errno = 0;
 	/*
@@ -1425,7 +1431,7 @@ int do_notify_parent(struct task_struct 
 	 * correct to rely on this
 	 */
 	rcu_read_lock();
-	info.si_pid = task_pid_nr_ns(tsk, tsk->parent->nsproxy->pid_ns);
+	info.si_pid = task_pid_nr_ns(tsk, parent->nsproxy->pid_ns);
 	info.si_uid = __task_cred(tsk)->uid;
 	rcu_read_unlock();
 
@@ -1444,7 +1450,7 @@ int do_notify_parent(struct task_struct 
 		info.si_status = tsk->exit_code >> 8;
 	}
 
-	psig = tsk->parent->sighand;
+	psig = parent->sighand;
 	spin_lock_irqsave(&psig->siglock, flags);
 	if (!task_ptrace(tsk) && sig == SIGCHLD &&
 	    (psig->action[SIGCHLD-1].sa.sa_handler == SIG_IGN ||
@@ -1469,8 +1475,8 @@ int do_notify_parent(struct task_struct 
 			sig = -1;
 	}
 	if (valid_signal(sig) && sig > 0)
-		__group_send_sig_info(sig, &info, tsk->parent);
-	__wake_up_parent(tsk, tsk->parent);
+		__group_send_sig_info(sig, &info, parent);
+	__wake_up_parent(tsk, parent);
 	spin_unlock_irqrestore(&psig->siglock, flags);
 
 	return ret;
@@ -1484,7 +1490,7 @@ static void do_notify_parent_cldstop(str
 	struct sighand_struct *sighand;
 
 	if (task_ptrace(tsk))
-		parent = tsk->parent;
+		parent = tsk->ptrace_task->pt_tracer;
 	else {
 		tsk = tsk->group_leader;
 		parent = tsk->real_parent;
@@ -1496,7 +1502,7 @@ static void do_notify_parent_cldstop(str
 	 * see comment in do_notify_parent() abot the following 3 lines
 	 */
 	rcu_read_lock();
-	info.si_pid = task_pid_nr_ns(tsk, tsk->parent->nsproxy->pid_ns);
+	info.si_pid = task_pid_nr_ns(tsk, parent->nsproxy->pid_ns);
 	info.si_uid = __task_cred(tsk)->uid;
 	rcu_read_unlock();
 
@@ -1544,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->parent->mm))
+	    unlikely(current->mm == current->ptrace_task->pt_tracer->mm))
 		return 0;
 
 	return 1;
@@ -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);
 	}
 
 	/* If the (new) signal is now blocked, requeue it.  */

Re: [RFC PATCH 7/X] ptrace: mv task->parent ptrace_task->pt_tracer

[Oleg Nesterov]

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?

Oleg.

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

[Oleg Nesterov]

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.  */

Re: [RFC PATCH 7/X] ptrace: mv task->parent ptrace_task->pt_tracer

[Roland McGrath]

> 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?

Yes, though nothing really cares about these values for such cases.
(It's really only there for dealing with debuggers that were written
before PTRACE_SETSIGINFO was invented.)

It's probably best now to clean this up so that this logic is
applied in the tracer causing the resumption rather than in the
tracee.  i.e. do it in ptrace_resume() and ptrace_detach().  It's
only now with your exhaustive clean-up of ptrace that we will get
really confident that we are covering all the possible paths that
can change ->exit_code and then resume it.  (I think those two are
it, but you should check my logic.)


Thanks,
Roland

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

[Roland McGrath]

> 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.

There is nothing really wrong with this.  But I think that this stuff will
get sufficiently reworked again differently later on if it's converted to
use utrace that this incremental cleanup may not really help any.

> 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?

It seems likely that we will rework tracehook_tracer_task() later.
It has three kinds of callers:

1. task_state() for "TracerPid:" line.
   It remains to be seen if we want to make some hookified way that might
   ever have a non-ptrace tracer supply the value here.  This was the main
   original expectation of what tracehook_tracer_task() would do.
2. check_mem_permission()
   I've already suggested to you that I think we want to swallow this
   use as part of the clean-up/replacement of ptrace_may_access().
3. SELinux: selinux_bprm_set_creds(), selinux_setprocattr()
   It makes sure that "PROCESS PTRACE" tracer->tracee avc checks can
   inhibit the transition (exec/setprocattr call).

For each of these, we have yet to hash out whether we will only ever want a
cleaned-up ptrace support here, or if in a future generalized tracing setup
like utrace these should be hooks that some non-ptrace kind of tracer
facility could also supply.  Figuring any piece of all that out is way
beyond the simple data structure cleanup phase.  I don't think we want to
get into any of that quite yet.

> +	parent = ptrace_tracer(tsk);
> +	if (likely(!parent))
>  		parent = tsk->real_parent;

This likely() doesn't buy much anyway, I'd just write the shorter:

	parent = ptrace_tracer(task) ?: tsk->real_parent;

>  static inline int may_ptrace_stop(void)
>  {
> -	if (!likely(task_ptrace(current)))
> +	struct task_struct *tracer = ptrace_tracer(current);
> +
> +	if (!likely(tracer))
>  		return 0;

Is there a particular rationale to checking ptrace_tracer() != NULL vs
task_ptrace() != 0?  Or is it just that they should already be guaranteed
synonymous, and here you have use for the tracer pointer a few lines later?

> +	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;

I think the different clean-up I suggested is better for this.  (Move that
logic to resume-time in the tracer context.)  It's an inconsequential nit,
but it feels a little wrong e.g. that you take task_uid(tracer) before the
stop, but the tracer could call setuid() before it does PTRACE_CONT.  The
PTRACE_CONT (or whatever) is the "signal generation event", so that's the
point at which the si_uid value being determined makes most sense to me
because it parallels what a normal signal generation does.


Thanks,
Roland

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

[Oleg Nesterov]

On 05/26, Roland McGrath wrote:
>
> > 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.
>
> There is nothing really wrong with this.  But I think that this stuff will
> get sufficiently reworked again differently later on if it's converted to
> use utrace that this incremental cleanup may not really help any.

Yes, but currently this change really makes the code look better. Just look
at this

-       if (task_ptrace(child) && child->ptrace_task->pt_tracer == current) {
+       if (ptrace_tracer(child) == current) {

change. But yes, these cosmetic changes will likely be reconsidered
later. The same for s/task->ptrace/task_ptrace(task)/ changes.

> > 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?
>
> It seems likely that we will rework tracehook_tracer_task() later.
> It has three kinds of callers:
>
> 1. task_state() for "TracerPid:" line.
>    It remains to be seen if we want to make some hookified way that might
>    ever have a non-ptrace tracer supply the value here.  This was the main
>    original expectation of what tracehook_tracer_task() would do.
> 2. check_mem_permission()
>    I've already suggested to you that I think we want to swallow this
>    use as part of the clean-up/replacement of ptrace_may_access().
> 3. SELinux: selinux_bprm_set_creds(), selinux_setprocattr()
>    It makes sure that "PROCESS PTRACE" tracer->tracee avc checks can
>    inhibit the transition (exec/setprocattr call).
>
> For each of these, we have yet to hash out whether we will only ever want a
> cleaned-up ptrace support here, or if in a future generalized tracing setup
> like utrace these should be hooks that some non-ptrace kind of tracer
> facility could also supply.  Figuring any piece of all that out is way
> beyond the simple data structure cleanup phase.  I don't think we want to
> get into any of that quite yet.

So, I assume it is better to not use tracehook_tracer_task() and add
another helper like this patch does.

> > +	parent = ptrace_tracer(tsk);
> > +	if (likely(!parent))
> >  		parent = tsk->real_parent;
>
> This likely() doesn't buy much anyway, I'd just write the shorter:
>
> 	parent = ptrace_tracer(task) ?: tsk->real_parent;

OK,

> >  static inline int may_ptrace_stop(void)
> >  {
> > -	if (!likely(task_ptrace(current)))
> > +	struct task_struct *tracer = ptrace_tracer(current);
> > +
> > +	if (!likely(tracer))
> >  		return 0;
>
> Is there a particular rationale to checking ptrace_tracer() != NULL vs
> task_ptrace() != 0?

No, except the code looks better, imho.

> Or is it just that they should already be guaranteed
> synonymous, and here you have use for the tracer pointer a few lines later?

Yes.

Oleg.

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

[Roland McGrath]

> So, I assume it is better to not use tracehook_tracer_task() and add
> another helper like this patch does.

I guess I don't think it matters one way or the other until we figure out
the rest of it.

Thanks,
Roland

Re: [RFC PATCH 7/X] ptrace: mv task->parent ptrace_task->pt_tracer

[Oleg Nesterov]

On 05/26, Roland McGrath wrote:
>
> > 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?
>
> Yes, though nothing really cares about these values for such cases.
> (It's really only there for dealing with debuggers that were written
> before PTRACE_SETSIGINFO was invented.)
>
> It's probably best now to clean this up so that this logic is
> applied in the tracer causing the resumption rather than in the
> tracee.  i.e. do it in ptrace_resume() and ptrace_detach().

Hmm. Didn't think about this, and I agree this looks nicer...
So, we need something like

	void ptrace_set_exit_code(struct task_struct *child, int exit_code)
	{
		unsigned long flags;

		if (!exit_code)
			child->exit_code = exit_code;

		if (child->exit_code == exit_code)
			return;

		if (lock_task_sighand(child, &flags)) {
			siginfo_t *info = child->last_siginfo;

			if (info && info->info->si_signo != exit_code) {
				info->si_signo = exit_code;
				info->si_errno = 0;
				info->si_code = SI_USER;
				info->si_pid = task_pid_nr_ns(current, child->nsproxy->pid_ns);
				info->si_uid = task_uid(current);

			}
			child->exit_code = exit_code;
			unlock_task_sighand(child, &flags);
		}
	}

And ptrace_resume/ptrace_detach should use ptrace_set_exit_code()
instead of child->exit_code = data.

The disadvantage is, ptrace_notify() does not need this, we add the
little pessimization...

And. This change adds another dependency with arches which implement
their own resume.

So. Do you think this cleanup should be done before/with this series
or we can do it later?

Oleg.

ptrace && task->exit_code

[Oleg Nesterov]

I didn't realize this until yesterday, but perhaps it makes sense
to decouple ptrace && task_struct->exit_code?

If not - do not read further.

This is not completely trivial, needs another short series.

And. I spent a lot of time, but I can't see how to solve the problems
with TASK_STOPPED tasks if we do this change.

For the moment, forget that ->exit_code is used by do_exit/etc. Suppose
we just move task->exit_code into ptrace_task->xxx.

Now. The never traced task (->ptrace_task == NULL) stops and sleeps in
TASK_STOPPED.

The tracer attaches, and then ptrace_check_attach() changes its ->state
to TASK_TRACED. But what should we do to ensure do_wait() will succeed
later?

Currently wait_task_stopped(ptrace => 1) needs ->exit_code != 0. Perhaps
we can change ptrace_check_attach() to set ptrace_task->xxx if it is zero.

But can't we just change wait_task_stopped() to return success when
ptrace == T regardless of ->exit_code == 0 ? I guess, the answer is
"we can break things".

What do you think?

Oleg.

Re: [RFC PATCH 7/X] ptrace: mv task->parent ptrace_task->pt_tracer

[Roland McGrath]

> 		if (!exit_code)
> 			child->exit_code = exit_code;

The condition is wrong.  It's unconditional except in the "already killed"
case that ptrace_detach() is checking for.  It never depends on the value.
(You probably meant the inverse of this test.  But that's wrong too.
PTRACE_CONT,0 must clear the old signal and not deliver any signal.)

> 		if (child->exit_code == exit_code)
> 			return;

This makes no sense unless it's before setting it.

> 		if (lock_task_sighand(child, &flags)) {
> 			siginfo_t *info = child->last_siginfo;
[...]
> And ptrace_resume/ptrace_detach should use ptrace_set_exit_code()
> instead of child->exit_code = data.

Right.

> The disadvantage is, ptrace_notify() does not need this, we add the
> little pessimization...

It can check for !child->last_siginfo before lock_task_sighand().

> And. This change adds another dependency with arches which implement
> their own resume.

The current draft series is meant to assume arch issues are already dealt
with before this merges.  If we need to sequence this part of it later than
most of it, we can revisit that later before really preparing to merge it.

> So. Do you think this cleanup should be done before/with this series
> or we can do it later?

Whatever you think fits best.  Right now I just want to get the rough draft
of the series all the way to the end of the most substantive work.  Do that
however seems most efficacious now.  We can juggle the order again later to
ease the eventual merging.


Thanks,
Roland

Re: ptrace && task->exit_code

[Roland McGrath]

> I didn't realize this until yesterday, but perhaps it makes sense
> to decouple ptrace && task_struct->exit_code?

I've long thought this was an attractive idea.  But it seems to have lots
of complications at least as long as ptrace-wait shares so much code with
normal wait.  I'd figured this might be one of the last things we clean up
after ptrace is disentangled from core data structures in most every other
way.

> This is not completely trivial, needs another short series.

I suspect it is more hassle than benefit to do this now.
I don't think it is the right priority.

> And. I spent a lot of time, but I can't see how to solve the problems
> with TASK_STOPPED tasks if we do this change.

I bet the complications of this all will be substantially different after
we change the ptrace locking.  So let's not worry about it yet.


Thanks,
Roland

Re: [RFC PATCH 7/X] ptrace: mv task->parent ptrace_task->pt_tracer

[Oleg Nesterov]

On 05/27, Roland McGrath wrote:
>
> > 		if (!exit_code)
> > 			child->exit_code = exit_code;
>
> The condition is wrong.  It's unconditional except in the "already killed"
> case that ptrace_detach() is checking for.  It never depends on the value.
> (You probably meant the inverse of this test.  But that's wrong too.
> PTRACE_CONT,0 must clear the old signal and not deliver any signal.)

This is optimization. If exit_code == 0 we do not need to fixup
->last_siginfo. Note that we have another "child->exit_code = exit_code"
below in the slow path.

> > 		if (child->exit_code == exit_code)
> > 			return;
>
> This makes no sense unless it's before setting it.

This also covers the "exit_code == 0" case. If exit_code = 0 we have
already set child->exit_code, we can return.

> > The disadvantage is, ptrace_notify() does not need this, we add the
> > little pessimization...
>
> It can check for !child->last_siginfo before lock_task_sighand().

ptrace_stop() always sets ->last_siginfo != NULL.

> > And. This change adds another dependency with arches which implement
> > their own resume.
>
> The current draft series is meant to assume arch issues are already dealt
> with before this merges.  If we need to sequence this part of it later than
> most of it, we can revisit that later before really preparing to merge it.
>
> > So. Do you think this cleanup should be done before/with this series
> > or we can do it later?
>
> Whatever you think fits best.  Right now I just want to get the rough draft
> of the series all the way to the end of the most substantive work.  Do that
> however seems most efficacious now.  We can juggle the order again later to
> ease the eventual merging.

OK, lets do this change later ;)

Oleg.

Re: [RFC PATCH 7/X] ptrace: mv task->parent ptrace_task->pt_tracer

[Roland McGrath]

> This is optimization. If exit_code == 0 we do not need to fixup
> ->last_siginfo. Note that we have another "child->exit_code = exit_code"
> below in the slow path.

Ok, I see.

> > It can check for !child->last_siginfo before lock_task_sighand().
> 
> ptrace_stop() always sets ->last_siginfo != NULL.

Oh, right.  It's only job control stops where it's NULL.

> OK, lets do this change later ;)

Agreed.


Thanks,
Roland

Re: [RFC PATCH 7/X] ptrace: mv task->parent ptrace_task->pt_tracer

[Oleg Nesterov]

On 05/27, Roland McGrath wrote:
>
> > OK, lets do this change later ;)
>
> Agreed.

Damn. OTOH, it is so ugly to cache pid/uid for the very unlikely case.

Roland, will you agree with the patch below?

If yes, I'd like to send it separately.

Oleg.

-------------------------------------------------------------------
[PATCH -mm] ptrace: ptrace_signal: fix the usage of ->parent

This patch complicates the code to fix the pure theoretical problems.
But since we are going to change this code, it is better to fix them
anyway.

- If we are not traced any longer after ptrace_stop(), si_pid/si_uid
  are not necessary right.

- It is not safe to dereference current->parent without tasklist or
  RCU lock. The tracer can detach and exit. ->siglock can't prevent
  this, and (in theory) local_irq_disable() doesn't imply RCU lock.

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

--- PTRACE/kernel/signal.c~PTRACE_SIGNAL	2009-05-28 04:12:02.000000000 +0200
+++ PTRACE/kernel/signal.c	2009-05-28 04:14:38.000000000 +0200
@@ -1770,11 +1770,22 @@ static int ptrace_signal(int signr, sigi
 	   specific in the siginfo structure then it should
 	   have updated *info via PTRACE_SETSIGINFO.  */
 	if (signr != info->si_signo) {
+		struct task_struct *tracer;
+
 		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);
+
+		rcu_read_lock();
+		tracer = current->parent;
+		if (task_ptrace(current)) {
+			info->si_pid = task_pid_vnr(tracer);
+			info->si_uid = task_uid(tracer);
+		} else {
+			info->si_pid = 0;
+			info->si_uid = 0;
+		}
+		rcu_read_unlock();
 	}
 
 	/* If the (new) signal is now blocked, requeue it.  */

Re: [RFC PATCH 7/X] ptrace: mv task->parent ptrace_task->pt_tracer

[Roland McGrath]

If you're going to zero out fields in a corner case, I think you should
zero si_code too so it's consistent with what collect_signal() produces
when a signal is posted with OOM for the sigq allocation.  Modulo that nit,
this is fine by me.  (It is likely nobody will ever see this path anyway,
since we'll probably merge the clean-up that replaces it before any release
that uses this series at all.)


Thanks,
Roland

Re: [RFC PATCH 7/X] ptrace: mv task->parent ptrace_task->pt_tracer

[Oleg Nesterov]

On 05/27, Roland McGrath wrote:
>
> If you're going to zero out fields in a corner case, I think you should
> zero si_code too

Well, SI_USER == 0, so si_code is zero. And SI_USER looks more
consistent to me, because

> so it's consistent with what collect_signal() produces
> when a signal is posted with OOM for the sigq allocation.

perhaps it is better to change collect_signal() to set SI_USER ?
Note that __send_signal() user SI_USER, not zero in is_si_special()
case.

> (It is likely nobody will ever see this path anyway,
> since we'll probably merge the clean-up that replaces it before any release
> that uses this series at all.)

Yes!

But without some fix, I was really puzzled how I can make any
progress now.

You never know which change will take most time...

Oleg.

Re: [RFC PATCH 7/X] ptrace: mv task->parent ptrace_task->pt_tracer

[Roland McGrath]

> Well, SI_USER == 0, so si_code is zero. And SI_USER looks more
> consistent to me, because

Ah so.

> > so it's consistent with what collect_signal() produces
> > when a signal is posted with OOM for the sigq allocation.
> 
> perhaps it is better to change collect_signal() to set SI_USER ?
> Note that __send_signal() user SI_USER, not zero in is_si_special()
> case.

Sure, though as you point out it's purely cosmetic.


Thanks,
Roland

Re: ptrace && task->exit_code

[Oleg Nesterov]

On 05/27, Roland McGrath wrote:
>
> > I didn't realize this until yesterday, but perhaps it makes sense
> > to decouple ptrace && task_struct->exit_code?
>
> I've long thought this was an attractive idea.  But it seems to have lots
> of complications at least as long as ptrace-wait shares so much code with
> normal wait.  I'd figured this might be one of the last things we clean up
> after ptrace is disentangled from core data structures in most every other
> way.
>
> > This is not completely trivial, needs another short series.
>
> I suspect it is more hassle than benefit to do this now.
> I don't think it is the right priority.
>
> > And. I spent a lot of time, but I can't see how to solve the problems
> > with TASK_STOPPED tasks if we do this change.
>
> I bet the complications of this all will be substantially different after
> we change the ptrace locking.  So let's not worry about it yet.

I just can't stop thinking of it ;)

Perhaps I missed something, but except the problem above this does not
look too hard. How about something like this:

	--- a/kernel/ptrace.c
	+++ b/kernel/ptrace.c
	@@ -228,7 +228,11 @@ int ptrace_attach(struct task_struct *task)
	 
		__ptrace_link(task, current);
	 
	-	send_sig_info(SIGSTOP, SEND_SIG_FORCED, task);
	+	spin_lock(task->signal->siglock);
	+	if (task_is_stopped(task) && !task->exit_code)
	+		task->exit_code = SIGSTOP;
	+	specific_send_sig_info(SIGSTOP, SEND_SIG_FORCED, task);
	+	spin_unlock(task->signal->siglock);
	 bad:
		write_unlock_irqrestore(&tasklist_lock, flags);
		task_unlock(task);

?

If we attach, and the task is already stopped, this really means
it was traced and untraced. We can set ->exit_code = SIGSTOP to
ensure do_wait() will succeed.

This also relates to attach-wait-on-stopped test-case, I cc'ed
Jan and Denys.

Note also that after

	do_wait: fix waiting for the group stop with the dead leader
	commit: 90bc8d8b1a38f1ab131a2399a202e1889db95de8

we can't confuse task->real_parent waiting for jctl stop.

What do you think?

Oleg.

Re: ptrace && task->exit_code

[Roland McGrath]

> If we attach, and the task is already stopped, this really means
> it was traced and untraced. We can set ->exit_code = SIGSTOP to
> ensure do_wait() will succeed.

If you go that route you really need a do_notify_parent_cldstop() call too.
Obviously the tracer itself won't be blocked in wait while it's in the
middle of this ptrace call.  But it might be relying on a designated thread
calling wait, or it might call wait only when provoked by SIGCHLD, etc.

If you do that, you might as well just do:

	spin_lock(task->signal->siglock);
	specific_send_sig_info(SIGSTOP, SEND_SIG_FORCED, task);
	signal_wake_up(task, 1);
	spin_unlock(task->signal->siglock);

That way PTRACE_ATTACH would have a uniform effect on task whether it
was stopped or not.  It always gives the tracer a fresh stop, never
leaves another SIGSTOP queued afterwards, and it's always a proper
ptrace stop where the debugger gets the full range of options like
injecting a new signal and using PTRACE_SETSIGINFO.  (OTOH, then the
debugger can't necessarily tell if the task had been stopped or not
before the attach.)

Jan will probably affirm that userland debuggers wish it had always been
this way.  But real userland debuggers already cope with existing
kernels and will have to continue to cope with old kernels for a long
time to come.  So I don't see that it buys anything to change it now.

> This also relates to attach-wait-on-stopped test-case, I cc'ed
> Jan and Denys.
> 
> Note also that after
> 
> 	do_wait: fix waiting for the group stop with the dead leader
> 	commit: 90bc8d8b1a38f1ab131a2399a202e1889db95de8
> 
> we can't confuse task->real_parent waiting for jctl stop.

Hmm.  I had not thought about how the 90bc8d8 change to touch
group_exit_code instead for real parents affected this ptrace area.

That means that PTRACE_ATTACH while already stopped but not waited-for
no longer "steals" the real parent's tracking of the child's stoppedness
when it had not yet done a wait after its SIGCHLD/wakeup.  After detach,
that wait could happen just like it would have before the debugger came
along.  However, nothing will wake up the parent's wait if it's already
blocked in one at detach time.


Thanks,
Roland