Fix for ptrace breakage

This patch fixes the following,

- race condition of ptrace flag
- sent odd signal to the tracer
- broken before behavior

Quoted 5 lines

This patch fixes the following,

- race condition of ptrace flag
- sent odd signal to the tracer
- broken before behavior

(looks good to me). I'm wondering about the following:

(quoted diff) +2 -4

-	while (!list_empty(&current->children))
-		zap_thread(list_entry(current->children.next,struct task_struct,sibling), current, 0);
-	while (!list_empty(&current->ptrace_children))
-		zap_thread(list_entry(current->ptrace_children.next,struct task_struct,ptrace_list), current, 1);
+	while ((p = eldest_child(current)) != NULL)
+		zap_thread(p, current);
 	BUG_ON(!list_empty(&current->children));

is it guaranteed that at this point current->ptrace_children is empty?

	list_for_each(_p, &father->ptrace_children) {
		p = list_entry(_p,struct task_struct,ptrace_list);
		list_del_init(&p->ptrace_list);
		reparent_thread(p, reaper, child_reaper);
		if (p->parent != p->real_parent)
			list_add(&p->ptrace_list, &p->real_parent->ptrace_children);
	}

current->ptrace_children should be empty after this reparent.

Quoted 8 lines · OGAWA Hirofumi

	list_for_each(_p, &father->ptrace_children) {
		p = list_entry(_p,struct task_struct,ptrace_list);
		list_del_init(&p->ptrace_list);
		reparent_thread(p, reaper, child_reaper);
		if (p->parent != p->real_parent)
			list_add(&p->ptrace_list, &p->real_parent->ptrace_children);
	}

Quoted 1 line

current->ptrace_children should be empty after this reparent.

oh, okay. It's also cleaner this way.

Grr, sorry. This patch is bad version.

list_for_each(_p, &father->ptrace_children) {

of course, this should

list_for_each_safe(_p, _n, &father->ptrace_children) {

Quoted 7 lines · OGAWA Hirofumi

Grr, sorry. This patch is bad version.

list_for_each(_p, &father->ptrace_children) {

of course, this should

list_for_each_safe(_p, _n, &father->ptrace_children) {

This is a patch which fixed the above. Please apply.

kernel/exit.c renamed from kernel/exit.c~ +1 -1

@@ -443,7 +442,7 @@ void exit_mm(struct task_struct *tsk)
 static inline void forget_original_parent(struct task_struct * father)
 {
 	struct task_struct *p, *reaper = father;
-	struct list_head *_p;
+	struct list_head *_p, *_n;
 
 	reaper = father->group_leader;
 	if (reaper == father)

@@ -462,52 +461,37 @@ static inline void forget_original_paren
 		if (father == p->real_parent)
 			reparent_thread(p, reaper, child_reaper);

kernel/exit.c renamed from kernel/exit.c~ +1 -23

-static inline void zap_thread(task_t *p, task_t *father, int traced)
+static inline void zap_thread(task_t *p, task_t *father)
 {
-	/* If someone else is tracing this thread, preserve the ptrace links.  */
-	if (unlikely(traced)) {
-		task_t *trace_task = p->parent;
-		int ptrace_flag = p->ptrace;
-		BUG_ON (ptrace_flag == 0);
-
-		__ptrace_unlink(p);
-		p->ptrace = ptrace_flag;
-		__ptrace_link(p, trace_task);
-	} else {
-		/*
-		 * Otherwise, if we were tracing this thread, untrace it.
-		 * If we were only tracing the thread (i.e. not its real
-		 * parent), stop here.
-		 */
-		ptrace_unlink (p);
-		if (p->parent != father) {
-			BUG_ON(p->parent != p->real_parent);
-			return;
-		}
-		list_del_init(&p->sibling);
-		p->parent = p->real_parent;

kernel/exit.c renamed from kernel/exit.c~ +5 -4

 	/*
 	 * process group orphan check
 	 * Case ii: Our child is in a different pgrp
 	 * than we are, and it was the only connection
 	 * outside, so the child pgrp is now orphaned.
 	 */
-	if ((p->pgrp != current->pgrp) &&
-	    (p->session == current->session)) {
+	if ((p->pgrp != father->pgrp) &&
+	    (p->session == father->session)) {
 		int pgrp = p->pgrp;
 
-		if (__will_become_orphaned_pgrp(pgrp, 0) && __has_stopped_jobs(pgrp)) {
+		if (__will_become_orphaned_pgrp(pgrp, 0) &&
+		    __has_stopped_jobs(pgrp)) {
 			__kill_pg_info(SIGHUP, (void *)1, pgrp);
 			__kill_pg_info(SIGCONT, (void *)1, pgrp);
 		}

@@ -520,7 +504,7 @@ static inline void zap_thread(task_t *p,
  */
 static void exit_notify(void)
 {
-	struct task_struct *t;
+	struct task_struct *t, *p;
 
 	write_lock_irq(&tasklist_lock);
 

@@ -580,10 +564,8 @@ static void exit_notify(void)
 	if (current->exit_signal != -1)
 		do_notify_parent(current, current->exit_signal);

Quoted 8 lines · OGAWA Hirofumi

Quoted 6 lines · OGAWA Hirofumi

Grr, sorry. This patch is bad version.

list_for_each(_p, &father->ptrace_children) {

of course, this should

Quoted 3 lines

Quoted 1 line

list_for_each_safe(_p, _n, &father->ptrace_children) {

This is a patch which fixed the above. Please apply.

Some comments. First of all, you said you fixed a race on current->ptrace and some other bugs - would you mind saying where they were? It's definitely cleaner after your patch but I'd like to understand where you found bugs, since I think you're introducing more.

If this is unnecessary, perhaps a BUG_ON()? This function is called from daemonize at which point the ptrace flag should be clear, and that's it.

Quoted 19 lines

(quoted diff) +4 -2

@@ -443,7 +442,7 @@ void exit_mm(struct task_struct *tsk)
 static inline void forget_original_parent(struct task_struct * father)
 {
 	struct task_struct *p, *reaper = father;
-	struct list_head *_p;
+	struct list_head *_p, *_n;
 
 	reaper = father->group_leader;
 	if (reaper == father)

@@ -462,52 +461,37 @@ static inline void forget_original_paren
 		if (father == p->real_parent)
 			reparent_thread(p, reaper, child_reaper);
 	}
-	list_for_each(_p, &father->ptrace_children) {
+	list_for_each_safe(_p, _n, &father->ptrace_children) {
 		p = list_entry(_p,struct task_struct,ptrace_list);
+		list_del_init(&p->ptrace_list);
 		reparent_thread(p, reaper, child_reaper);
+		if (p->parent != p->real_parent)

Review static inline void forget_original_paren · line 471

Quoted 3 lines

(quoted diff) +1 -0

+			list_add(&p->ptrace_list, &p->real_parent->ptrace_children);
 	}
 }

So you reparent children on the ptrace_list right here. But they still need to go through zap_thread! You're right, the do_notify_parent in zap_thread isn't necessary; it'll be taken care of in sys_wait4. The orphaned pgrp check is still relevant though.

If you're going to remove the if, you need to maintain its effect! See:

Review static inline void forget_original_paren · line 473

Quoted 4 lines

(quoted diff) +0 -4

-		if (p->parent != father) {
-			BUG_ON(p->parent != p->real_parent);
-			return;
-		}

This is the case where we were tracing something. The ptrace_unlink returned it to its original parent. It doesn't need the remove_parent/add_parent (though they are harmless); it does need to avoid the orphaned pgrp check. It may need the do_notify_parent check, which was a bug in the previous code.

Quoted 9 lines · Daniel Jacobowitz

Quoted 8 lines · OGAWA Hirofumi

Quoted 6 lines · OGAWA Hirofumi

Grr, sorry. This patch is bad version.

list_for_each(_p, &father->ptrace_children) {

of course, this should

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

list_for_each_safe(_p, _n, &father->ptrace_children) {

This is a patch which fixed the above. Please apply.

Some comments. First of all, you said you fixed a race on current->ptrace and some other bugs - would you mind saying where they were? It's definitely cleaner after your patch but I'd like to understand where you found bugs, since I think you're introducing more.

It's the following

		task_t *trace_task = p->parent;
		int ptrace_flag = p->ptrace;
		BUG_ON (ptrace_flag == 0);

		__ptrace_unlink(p);
		p->ptrace = ptrace_flag;
		__ptrace_link(p, trace_task);

Quoted 3 lines

If this is unnecessary, perhaps a BUG_ON()? This function is called from daemonize at which point the ptrace flag should be clear, and that's it.

You aren't looking the source.

void reparent_to_init(void)

{
	write_lock_irq(&tasklist_lock);

	ptrace_unlink(current);

Quoted 19 lines

Quoted 19 lines

(quoted diff) +4 -2

@@ -443,7 +442,7 @@ void exit_mm(struct task_struct *tsk)
 static inline void forget_original_parent(struct task_struct * father)
 {
 	struct task_struct *p, *reaper = father;
-	struct list_head *_p;
+	struct list_head *_p, *_n;
 
 	reaper = father->group_leader;
 	if (reaper == father)

@@ -462,52 +461,37 @@ static inline void forget_original_paren
 		if (father == p->real_parent)
 			reparent_thread(p, reaper, child_reaper);
 	}
-	list_for_each(_p, &father->ptrace_children) {
+	list_for_each_safe(_p, _n, &father->ptrace_children) {
 		p = list_entry(_p,struct task_struct,ptrace_list);
+		list_del_init(&p->ptrace_list);
 		reparent_thread(p, reaper, child_reaper);
+		if (p->parent != p->real_parent)

Quoted 8 lines

Review static inline void forget_original_paren · line 471

Quoted 3 lines

(quoted diff) +1 -0

+			list_add(&p->ptrace_list, &p->real_parent->ptrace_children);
 	}
 }

So you reparent children on the ptrace_list right here. But they still need to go through zap_thread! You're right, the do_notify_parent in zap_thread isn't necessary; it'll be taken care of in sys_wait4. The orphaned pgrp check is still relevant though.

??? You forget tasklist_lock?

Quoted 12 lines

If you're going to remove the if, you need to maintain its effect! See:

Review static inline void forget_original_paren · line 473

Quoted 4 lines

(quoted diff) +0 -4

-		if (p->parent != father) {
-			BUG_ON(p->parent != p->real_parent);
-			return;
-		}

This is the case where we were tracing something. The ptrace_unlink returned it to its original parent. It doesn't need the remove_parent/add_parent (though they are harmless); it does need to avoid the orphaned pgrp check. It may need the do_notify_parent check, which was a bug in the previous code.

What is the basis which you think it is bug?

Quoted 11 lines

Quoted 4 lines

Some comments. First of all, you said you fixed a race on current->ptrace and some other bugs - would you mind saying where they were? It's definitely cleaner after your patch but I'd like to understand where you found bugs, since I think you're introducing more.

It's the following

		task_t *trace_task = p->parent;
		int ptrace_flag = p->ptrace;
		BUG_ON (ptrace_flag == 0);

Quoted 3 lines

		__ptrace_unlink(p);
		p->ptrace = ptrace_flag;
		__ptrace_link(p, trace_task);

We have the tasklist lock. How can there be a race here? The parent can't detach while we're holding the tasklist lock. If there is a race with PTRACE_SETOPTIONS, then PTRACE_SETOPTIONS should take the lock.

Quoted 6 lines

Quoted 4 lines

So you reparent children on the ptrace_list right here. But they still need to go through zap_thread! You're right, the do_notify_parent in zap_thread isn't necessary; it'll be taken care of in sys_wait4. The orphaned pgrp check is still relevant though.

??? You forget tasklist_lock?

Huh?

The problem I am describing is when a child - which will become an orphaned pgrp when ``father'' dies - is being ptraced at the moment of ``father''s death. With your patch it will be moved to reaper->ptrace_children (or child_reaper->ptrace_children) but never orphaned properly. It'll miss a signal.

Quoted 13 lines

Quoted 12 lines

If you're going to remove the if, you need to maintain its effect! See:

Review

Quoted 4 lines

(quoted diff) +0 -4

-		if (p->parent != father) {
-			BUG_ON(p->parent != p->real_parent);
-			return;
-		}

This is the case where we were tracing something. The ptrace_unlink returned it to its original parent. It doesn't need the remove_parent/add_parent (though they are harmless); it does need to avoid the orphaned pgrp check. It may need the do_notify_parent check, which was a bug in the previous code.

Quoted 1 line

What is the basis which you think it is bug?

The death of a tracing process should not have any effect on the traced process except to untrace it. It should not go through the orphaning checks. The orphaning checks assume that the exiting process is the real parent, and will orphan the pgrp if it is not in the same session... as its tracer! That's a bug.

Quoted 15 lines · Daniel Jacobowitz

Quoted 14 lines

Quoted 4 lines

Some comments. First of all, you said you fixed a race on current->ptrace and some other bugs - would you mind saying where they were? It's definitely cleaner after your patch but I'd like to understand where you found bugs, since I think you're introducing more.

It's the following

		task_t *trace_task = p->parent;
		int ptrace_flag = p->ptrace;
		BUG_ON (ptrace_flag == 0);

		__ptrace_unlink(p);
		p->ptrace = ptrace_flag;
		__ptrace_link(p, trace_task);

Quoted 3 lines

We have the tasklist lock. How can there be a race here? The parent can't detach while we're holding the tasklist lock. If there is a race with PTRACE_SETOPTIONS, then PTRACE_SETOPTIONS should take the lock.

No. If the real parent don't change ->ptrace, it doesn't need lock.

Quoted 28 lines

Quoted 6 lines

Quoted 4 lines

So you reparent children on the ptrace_list right here. But they still need to go through zap_thread! You're right, the do_notify_parent in zap_thread isn't necessary; it'll be taken care of in sys_wait4. The orphaned pgrp check is still relevant though.

??? You forget tasklist_lock?

Huh?

The problem I am describing is when a child - which will become an orphaned pgrp when ``father'' dies - is being ptraced at the moment of ``father''s death. With your patch it will be moved to reaper->ptrace_children (or child_reaper->ptrace_children) but never orphaned properly. It'll miss a signal.

Quoted 13 lines

Quoted 12 lines

If you're going to remove the if, you need to maintain its effect! See:

Review

Quoted 4 lines

(quoted diff) +0 -4

-		if (p->parent != father) {
-			BUG_ON(p->parent != p->real_parent);
-			return;
-		}

This is the case where we were tracing something. The ptrace_unlink returned it to its original parent. It doesn't need the remove_parent/add_parent (though they are harmless); it does need to avoid the orphaned pgrp check. It may need the do_notify_parent check, which was a bug in the previous code.

Quoted 7 lines

Quoted 1 line

What is the basis which you think it is bug?

The death of a tracing process should not have any effect on the traced process except to untrace it. It should not go through the orphaning checks. The orphaning checks assume that the exiting process is the real parent, and will orphan the pgrp if it is not in the same session... as its tracer! That's a bug.

Ah, ok. I think, it's longtime (odd) behavior. And you think, it's a bug. Right?

And, both of your and old code has odd behavior. yes?

Quoted 6 lines

Quoted 3 lines

We have the tasklist lock. How can there be a race here? The parent can't detach while we're holding the tasklist lock. If there is a race with PTRACE_SETOPTIONS, then PTRACE_SETOPTIONS should take the lock.

No. If the real parent don't change ->ptrace, it doesn't need lock.

I don't understand what you mean by that. Do you mean, "if it does change ->ptrace, it doesn't need a lock"?

Quoted 4 lines

Ah, ok. I think, it's longtime (odd) behavior. And you think, it's a bug. Right?

And, both of your and old code has odd behavior. yes?

Before your patch, do_notify_parent didn't get called; I think that perhaps it should be. I'll think about that. After your patch the process group will be unexpectedly orphaned, which is not now the case.

Let me sit on this for a couple of hours. I'll send you an alternative patch to look at.