[PATCH 2/2] introduce ptrace_reparented() helper

[PATCH 2/2] introduce ptrace_reparented() helper

[Oleg Nesterov]

Add another trivial helper for the sake of grep. It also auto-documents the
fact that ->parent != real_parent implies ->ptrace.

No functional changes.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 25/include/linux/ptrace.h~5_PR	2008-02-17 23:40:09.000000000 +0300
+++ 25/include/linux/ptrace.h	2008-03-03 06:22:15.000000000 +0300
@@ -98,6 +98,10 @@ extern void ptrace_untrace(struct task_s
 extern int ptrace_may_attach(struct task_struct *task);
 extern int __ptrace_may_attach(struct task_struct *task);
 
+static inline int ptrace_reparented(struct task_struct *child)
+{
+	return child->real_parent != child->parent;
+}
 static inline void ptrace_link(struct task_struct *child,
 			       struct task_struct *new_parent)
 {
--- 25/kernel/exit.c~5_PR	2008-03-03 05:43:17.000000000 +0300
+++ 25/kernel/exit.c	2008-03-03 06:24:56.000000000 +0300
@@ -629,7 +629,7 @@ reparent_thread(struct task_struct *p, s
 	if (unlikely(traced)) {
 		/* Preserve ptrace links if someone else is tracing this child.  */
 		list_del_init(&p->ptrace_list);
-		if (p->parent != p->real_parent)
+		if (ptrace_reparented(p))
 			list_add(&p->ptrace_list, &p->real_parent->ptrace_children);
 	} else {
 		/* If this child is being traced, then we're the one tracing it
@@ -796,8 +796,8 @@ static void exit_notify(struct task_stru
 	 * only has special meaning to our real parent.
 	 */
 	if (!task_detached(tsk) && thread_group_empty(tsk)) {
-		int signal = (tsk->parent == tsk->real_parent)
-				? tsk->exit_signal : SIGCHLD;
+		int signal = ptrace_reparented(tsk) ?
+				SIGCHLD : tsk->exit_signal;
 		do_notify_parent(tsk, signal);
 	} else if (tsk->ptrace) {
 		do_notify_parent(tsk, SIGCHLD);
@@ -1198,8 +1198,7 @@ static int wait_task_zombie(struct task_
 		return 0;
 	}
 
-	/* traced means p->ptrace, but not vice versa */
-	traced = (p->real_parent != p->parent);
+	traced = ptrace_reparented(p);
 
 	if (likely(!traced)) {
 		struct signal_struct *psig;

Re: [PATCH 2/2] introduce ptrace_reparented() helper

[Oleg Nesterov]

Somehow the patch I sent misses the change in ptrace.c, it can use the
new helper too.


[PATCH 2/2] introduce ptrace_reparented() helper

Add another trivial helper for the sake of grep. It also auto-documents the
fact that ->parent != real_parent implies ->ptrace.

No functional changes.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 25/include/linux/ptrace.h~5_PR	2008-02-17 23:40:09.000000000 +0300
+++ 25/include/linux/ptrace.h	2008-03-03 06:22:15.000000000 +0300
@@ -98,6 +98,10 @@ extern void ptrace_untrace(struct task_s
 extern int ptrace_may_attach(struct task_struct *task);
 extern int __ptrace_may_attach(struct task_struct *task);
 
+static inline int ptrace_reparented(struct task_struct *child)
+{
+	return child->real_parent != child->parent;
+}
 static inline void ptrace_link(struct task_struct *child,
 			       struct task_struct *new_parent)
 {
--- 25/kernel/exit.c~5_PR	2008-03-03 05:43:17.000000000 +0300
+++ 25/kernel/exit.c	2008-03-03 06:24:56.000000000 +0300
@@ -629,7 +629,7 @@ reparent_thread(struct task_struct *p, s
 	if (unlikely(traced)) {
 		/* Preserve ptrace links if someone else is tracing this child.  */
 		list_del_init(&p->ptrace_list);
-		if (p->parent != p->real_parent)
+		if (ptrace_reparented(p))
 			list_add(&p->ptrace_list, &p->real_parent->ptrace_children);
 	} else {
 		/* If this child is being traced, then we're the one tracing it
@@ -796,8 +796,8 @@ static void exit_notify(struct task_stru
 	 * only has special meaning to our real parent.
 	 */
 	if (!task_detached(tsk) && thread_group_empty(tsk)) {
-		int signal = (tsk->parent == tsk->real_parent)
-				? tsk->exit_signal : SIGCHLD;
+		int signal = ptrace_reparented(tsk) ?
+				SIGCHLD : tsk->exit_signal;
 		do_notify_parent(tsk, signal);
 	} else if (tsk->ptrace) {
 		do_notify_parent(tsk, SIGCHLD);
@@ -1198,8 +1198,7 @@ static int wait_task_zombie(struct task_
 		return 0;
 	}
 
-	/* traced means p->ptrace, but not vice versa */
-	traced = (p->real_parent != p->parent);
+	traced = ptrace_reparented(p);
 
 	if (likely(!traced)) {
 		struct signal_struct *psig;
--- 25/kernel/ptrace.c~5_PR	2008-02-17 23:40:09.000000000 +0300
+++ 25/kernel/ptrace.c	2008-03-03 06:23:43.000000000 +0300
@@ -73,7 +73,7 @@ void __ptrace_unlink(struct task_struct 
 	BUG_ON(!child->ptrace);
 
 	child->ptrace = 0;
-	if (!list_empty(&child->ptrace_list)) {
+	if (ptrace_reparented(child)) {
 		list_del_init(&child->ptrace_list);
 		remove_parent(child);
 		child->parent = child->real_parent;

Re: [PATCH 2/2] introduce ptrace_reparented() helper

[Roland McGrath]

> Somehow the patch I sent misses the change in ptrace.c, it can use the
> new helper too.

Actually, my pedantic streak prefers that to be in a separate patch.

I think the ptrace_reparented cleanup is fine, it is purely cosmetic and
improves readability.  

> -	if (!list_empty(&child->ptrace_list)) {
> +	if (ptrace_reparented(child)) {

This is changing the test from list_empty(&child->ptrace_list)
to child->parent == child->real_parent.  It should indeed be
impossible for those tests not to match.  But, paranoia is its
own reward.  I don't object to the change, but it should be
separate so bisect distinguishes it should it ever turn out to
matter in some way we are now overlooking.  I'd even be a
little inclined towards:

	if (child->real_parent == child->parent) {
		BUG_ON(!list_empty(&child->ptrace_list));
		return 0;
	} else {
		BUG_ON(list_empty(&child->ptrace_list));
		return 1;
	}

except of course you couldn't use that in the reparent_thread case.


Thanks,
Roland

Re: [PATCH 2/2] introduce ptrace_reparented() helper

[Oleg Nesterov]

On 03/04, Roland McGrath wrote:
>
> > Somehow the patch I sent misses the change in ptrace.c, it can use the
> > new helper too.
> 
> Actually, my pedantic streak prefers that to be in a separate patch.
> ...
> I don't object to the change, but it should be
> separate so bisect distinguishes it should it ever turn out to
> matter in some way we are now overlooking. 

OK, agreed, will do.

> I'd even be a
> little inclined towards:
> 
> 	if (child->real_parent == child->parent) {
> 		BUG_ON(!list_empty(&child->ptrace_list));
> 		return 0;
> 	} else {
> 		BUG_ON(list_empty(&child->ptrace_list));
> 		return 1;
> 	}
> 
> except of course you couldn't use that in the reparent_thread case.
                                                ^^^^^^^^^^^^^^^
I can't believe. You are reading my mind!

I am planning to do some changes in forget_original_parent (fix 2 very
old minor bugs and _perhaps_ add some improvement). I hit the minor but
nasty problem: this open coded __ptrace_unlink() in reparent_thread().
_This_ is the actual reason for this patch.

So. Would you object if I do

	--- kernel/ptrace.c	2008-03-03 17:01:06.000000000 +0300
	+++ kernel/ptrace.c	2008-03-05 20:22:44.801142777 +0300
	@@ -67,11 +67,12 @@ void ptrace_untrace(struct task_struct *
	  * remove it from the ptrace list.
	  *
	  * Must be called with the tasklist lock write-held.
	+ *
	+ * Either the caller is ptracer, or the caller is ->real_parent
	+ * and the child is not traced.
	  */
	 void __ptrace_unlink(struct task_struct *child)
	 {
	-	BUG_ON(!child->ptrace);
	-
		child->ptrace = 0;
		if (ptrace_reparented(child)) {
			list_del_init(&child->ptrace_list);

?

Oleg.

Re: [PATCH 2/2] introduce ptrace_reparented() helper

[Roland McGrath]

> I am planning to do some changes in forget_original_parent (fix 2 very
> old minor bugs and _perhaps_ add some improvement). I hit the minor but
> nasty problem: this open coded __ptrace_unlink() in reparent_thread().
> _This_ is the actual reason for this patch.
> 
> So. Would you object if I do
> 
> 	--- kernel/ptrace.c	2008-03-03 17:01:06.000000000 +0300
> 	+++ kernel/ptrace.c	2008-03-05 20:22:44.801142777 +0300
> 	@@ -67,11 +67,12 @@ void ptrace_untrace(struct task_struct *
> 	  * remove it from the ptrace list.
> 	  *
> 	  * Must be called with the tasklist lock write-held.
> 	+ *
> 	+ * Either the caller is ptracer, or the caller is ->real_parent
> 	+ * and the child is not traced.
> 	  */
> 	 void __ptrace_unlink(struct task_struct *child)
> 	 {
> 	-	BUG_ON(!child->ptrace);
> 	-
> 		child->ptrace = 0;
> 		if (ptrace_reparented(child)) {
> 			list_del_init(&child->ptrace_list);
> 
> ?

That seems ok.  The duplication of magic in reparent_thread is indeed bad.
I don't think this BUG_ON is buying us much if we haven't hit it in a long
time.


Thanks,
Roland