Re: [patch] ptrace-fix-2.5.33-A1

Re: [patch] ptrace-fix-2.5.33-A1

[Ingo Molnar]

On Thu, 5 Sep 2002, Daniel Jacobowitz wrote:

> There's definitely still something wrong... let me just run through my
> understanding of these lists, to make sure we're on the same page.
> 
> tsk->children: tsk's children, which are either untraced or traced by
> 	tsk.  They have p->parent == p->real_parent == tsk.
> 	Chained in p->sibling.

no - the way i wrote it originally was that only untraced children should
be on the tsk->children list. Traced tasks will be on the debugger's
->children list, plus will be on the real parent's ->ptrace_children list.

> tsk->ptrace_children: tsk's children, which are traced by some other
> 	process.  They have p->real_parent == tsk and p->parent != tsk.
> 	Chained in p->ptrace_list.

yes. This means that the sum of the two lists gives the real, total set of
children.

this splitup of the lists makes it possible for the debugger to do a wait4
that will get events from the debugged task, and for the debugged task to
also be available to the real parent.

is this really what we want?

(note that the meaning of the lists is not necesserily cleanly expressed
via the code, all deviations are most likely bugs.)

	Ingo

Re: [patch] ptrace-fix-2.5.33-A1

[Daniel Jacobowitz]

On Fri, Sep 06, 2002 at 12:09:51AM +0200, Ingo Molnar wrote:
> 
> On Thu, 5 Sep 2002, Daniel Jacobowitz wrote:
> 
> > There's definitely still something wrong... let me just run through my
> > understanding of these lists, to make sure we're on the same page.
> > 
> > tsk->children: tsk's children, which are either untraced or traced by
> > 	tsk.  They have p->parent == p->real_parent == tsk.
> > 	Chained in p->sibling.
> 
> no - the way i wrote it originally was that only untraced children should
> be on the tsk->children list. Traced tasks will be on the debugger's
> ->children list, plus will be on the real parent's ->ptrace_children list.

Right - let me rephrase.  Tasks which are either:
  - untraced, normal
  - traced, but traced _by their parent_
are on the sibling/children list.

Tasks which are traced by some not-my-parent process go on the
ptrace_children list.  So I think we agree.

> > tsk->ptrace_children: tsk's children, which are traced by some other
> > 	process.  They have p->real_parent == tsk and p->parent != tsk.
> > 	Chained in p->ptrace_list.
> 
> yes. This means that the sum of the two lists gives the real, total set of
> children.
> 
> this splitup of the lists makes it possible for the debugger to do a wait4
> that will get events from the debugged task, and for the debugged task to
> also be available to the real parent.

Great.  I'm not exactly sure on how this works right now: sys_wait4
only iterates over ->children, with the exception of the special code
in TASK_ZOMBIE.  I'm not quite sure when events from a traced process
get to the normal parent of that process, or when they're supposed to.

> is this really what we want?
> 
> (note that the meaning of the lists is not necesserily cleanly expressed
> via the code, all deviations are most likely bugs.)

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch] ptrace-fix-2.5.33-A1

[Ingo Molnar]

On Thu, 5 Sep 2002, Daniel Jacobowitz wrote:

> Right - let me rephrase.  Tasks which are either:
>   - untraced, normal
>   - traced, but traced _by their parent_
> are on the sibling/children list.

hm, why the distinction along whether the debugger == real parent? What
wrong can happen if we always move traced tasks to the ptrace list? The
task will be both in the ptrace list and in the parent's child list, and 
everything should work as expected. This looks a more symmetric and 
simpler thing to me.

> > this splitup of the lists makes it possible for the debugger to do a wait4
> > that will get events from the debugged task, and for the debugged task to
> > also be available to the real parent.
> 
> Great.  I'm not exactly sure on how this works right now: sys_wait4 only
> iterates over ->children, with the exception of the special code in
> TASK_ZOMBIE.  I'm not quite sure when events from a traced process get
> to the normal parent of that process, or when they're supposed to.

i'm not sure about this either. What happens if an (untraced) parent has
traced and untraced children, and does a wait4. Would it confuse the
debugger if the parent could get one of the traced tasks as a result in
wait4? And how does the debugger solve this problem?

	Ingo

Re: [patch] ptrace-fix-2.5.33-A1

[Daniel Jacobowitz]

On Fri, Sep 06, 2002 at 12:25:32AM +0200, Ingo Molnar wrote:
> 
> On Thu, 5 Sep 2002, Daniel Jacobowitz wrote:
> 
> > Right - let me rephrase.  Tasks which are either:
> >   - untraced, normal
> >   - traced, but traced _by their parent_
> > are on the sibling/children list.
> 
> hm, why the distinction along whether the debugger == real parent? What
> wrong can happen if we always move traced tasks to the ptrace list? The
> task will be both in the ptrace list and in the parent's child list, and 
> everything should work as expected. This looks a more symmetric and 
> simpler thing to me.

Hmm, I like that idea.  But I was talking about current implementation. 
If we want to do this then we'd need to fix up every ptrace
implementation in every architecture to call the appropriate function;
it's a separate problem.

> > > this splitup of the lists makes it possible for the debugger to do a wait4
> > > that will get events from the debugged task, and for the debugged task to
> > > also be available to the real parent.
> > 
> > Great.  I'm not exactly sure on how this works right now: sys_wait4 only
> > iterates over ->children, with the exception of the special code in
> > TASK_ZOMBIE.  I'm not quite sure when events from a traced process get
> > to the normal parent of that process, or when they're supposed to.
> 
> i'm not sure about this either. What happens if an (untraced) parent has
> traced and untraced children, and does a wait4. Would it confuse the
> debugger if the parent could get one of the traced tasks as a result in
> wait4? And how does the debugger solve this problem?

Well, it seems to me that when a traced task has an event, it should be
reported first to the debugger - for signals this happens in do_signal
- and then possibly to the normal parent.  But I'm not sure if this
actually happens right now or not.  Worth investigating some more.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch] ptrace-fix-2.5.33-A1

[Ingo Molnar]

On Thu, 5 Sep 2002, Daniel Jacobowitz wrote:

> If we want to do this then we'd need to fix up every ptrace
> implementation in every architecture to call the appropriate function;
> it's a separate problem.

which code relies on having debugged children only in the ->children list
and not in the ->ptrace_children list?

> > i'm not sure about this either. What happens if an (untraced) parent has
> > traced and untraced children, and does a wait4. Would it confuse the
> > debugger if the parent could get one of the traced tasks as a result in
> > wait4? And how does the debugger solve this problem?
> 
> Well, it seems to me that when a traced task has an event, it should be
> reported first to the debugger - for signals this happens in do_signal -
> and then possibly to the normal parent.  But I'm not sure if this
> actually happens right now or not.  Worth investigating some more.

it just cannot happen. There are only two kinds of events passed via
wait4: tracing related and exit related. An exiting task is not traced
anymore. And two tasks cannot trace the same task - so it can never happen
that wait4 wants to look at ->ptrace_children for events.

	Ingo

Re: [patch] ptrace-fix-2.5.33-A1

[Daniel Jacobowitz]

On Fri, Sep 06, 2002 at 12:39:02AM +0200, Ingo Molnar wrote:
> 
> On Thu, 5 Sep 2002, Daniel Jacobowitz wrote:
> 
> > If we want to do this then we'd need to fix up every ptrace
> > implementation in every architecture to call the appropriate function;
> > it's a separate problem.
> 
> which code relies on having debugged children only in the ->children list
> and not in the ->ptrace_children list?

Every implementation of PTRACE_TRACEME leaves them in the ->children
list.  They are never added to ptrace_children.  Whether this is
_right_ is another question.

> > > i'm not sure about this either. What happens if an (untraced) parent has
> > > traced and untraced children, and does a wait4. Would it confuse the
> > > debugger if the parent could get one of the traced tasks as a result in
> > > wait4? And how does the debugger solve this problem?
> > 
> > Well, it seems to me that when a traced task has an event, it should be
> > reported first to the debugger - for signals this happens in do_signal -
> > and then possibly to the normal parent.  But I'm not sure if this
> > actually happens right now or not.  Worth investigating some more.
> 
> it just cannot happen. There are only two kinds of events passed via
> wait4: tracing related and exit related. An exiting task is not traced
> anymore. And two tasks cannot trace the same task - so it can never happen
> that wait4 wants to look at ->ptrace_children for events.

Oh.  You are, of course, right.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch] ptrace-fix-2.5.33-A1

[Ingo Molnar]

i've attached a combined patch of your two patches, against BK-curr. Looks
good to me, and since it passed your more complex ptrace tests ...

	Ingo

--- linux/kernel/exit.c.orig	Fri Sep  6 00:55:02 2002
+++ linux/kernel/exit.c	Fri Sep  6 00:57:58 2002
@@ -66,6 +66,11 @@
 	atomic_dec(&p->user->processes);
 	security_ops->task_free_security(p);
 	free_uid(p->user);
+	if (unlikely(p->ptrace)) {
+		write_lock_irq(&tasklist_lock);
+		__ptrace_unlink(p);
+		write_unlock_irq(&tasklist_lock);
+	}
 	BUG_ON(!list_empty(&p->ptrace_list) || !list_empty(&p->ptrace_children));
 	unhash_process(p);
 
@@ -427,7 +432,7 @@
 	 * There are only two places where our children can be:
 	 *
 	 * - in our child list
-	 * - in the global ptrace list
+	 * - in our ptraced child list
 	 *
 	 * Search them and reparent children.
 	 */
@@ -442,14 +447,22 @@
 	read_unlock(&tasklist_lock);
 }
 
-static inline void zap_thread(task_t *p, task_t *father)
+static inline void zap_thread(task_t *p, task_t *father, int traced)
 {
-	ptrace_unlink(p);
-	list_del_init(&p->sibling);
-	p->ptrace = 0;
+	/* If we were tracing the thread, release it; otherwise preserve the
+	   ptrace links.  */
+	if (unlikely(traced)) {
+		task_t *trace_task = p->parent;
+		__ptrace_unlink(p);
+		p->ptrace = 1;
+		__ptrace_link(p, trace_task);
+	} else {
+		p->ptrace = 0;
+		list_del_init(&p->sibling);
+		p->parent = p->real_parent;
+		list_add_tail(&p->sibling, &p->parent->children);
+	}
 
-	p->parent = p->real_parent;
-	list_add_tail(&p->sibling, &p->parent->children);
 	if (p->state == TASK_ZOMBIE && p->exit_signal != -1)
 		do_notify_parent(p, p->exit_signal);
 	/*
@@ -540,11 +553,11 @@
 
 zap_again:
 	list_for_each_safe(_p, _n, &current->children)
-		zap_thread(list_entry(_p,struct task_struct,sibling), current);
+		zap_thread(list_entry(_p,struct task_struct,sibling), current, 0);
 	list_for_each_safe(_p, _n, &current->ptrace_children)
-		zap_thread(list_entry(_p,struct task_struct,ptrace_list), current);
+		zap_thread(list_entry(_p,struct task_struct,ptrace_list), current, 1);
 	/*
-	 * reparent_thread might drop the tasklist lock, thus we could
+	 * zap_thread might drop the tasklist lock, thus we could
 	 * have new children queued back from the ptrace list into the
 	 * child list:
 	 */
@@ -715,7 +728,7 @@
 				retval = p->pid;
 				if (p->real_parent != p->parent) {
 					write_lock_irq(&tasklist_lock);
-					ptrace_unlink(p);
+					__ptrace_unlink(p);
 					do_notify_parent(p, SIGCHLD);
 					write_unlock_irq(&tasklist_lock);
 				} else
--- linux/kernel/ptrace.c.orig	Fri Sep  6 00:58:13 2002
+++ linux/kernel/ptrace.c	Fri Sep  6 00:58:24 2002
@@ -29,7 +29,7 @@
 	if (!list_empty(&child->ptrace_list))
 		BUG();
 	if (child->parent == new_parent)
-		BUG();
+		return;
 	list_add(&child->ptrace_list, &child->parent->ptrace_children);
 	REMOVE_LINKS(child);
 	child->parent = new_parent;

Re: [patch] ptrace-fix-2.5.33-A1

[OGAWA Hirofumi]

Ingo Molnar <mingo@elte.hu> writes:

> i've attached a combined patch of your two patches, against BK-curr. Looks
> good to me, and since it passed your more complex ptrace tests ...
> 
> 	Ingo
> 
> --- linux/kernel/exit.c.orig	Fri Sep  6 00:55:02 2002
> +++ linux/kernel/exit.c	Fri Sep  6 00:57:58 2002
> @@ -66,6 +66,11 @@
>  	atomic_dec(&p->user->processes);
>  	security_ops->task_free_security(p);
>  	free_uid(p->user);
> +	if (unlikely(p->ptrace)) {
> +		write_lock_irq(&tasklist_lock);
> +		__ptrace_unlink(p);
> +		write_unlock_irq(&tasklist_lock);
> +	}
>  	BUG_ON(!list_empty(&p->ptrace_list) || !list_empty(&p->ptrace_children));

Looks like it's need the only CLONE_DETACH process. Why it's here?

	 * Search them and reparent children.
	 */
	list_for_each(_p, &father->children) {
		p = list_entry(_p,struct task_struct,sibling);
		reparent_thread(p, reaper, child_reaper);
	}

Looks like that tracer deprive a process from real parent.

	list_for_each(_p, &father->ptrace_children) {
		p = list_entry(_p,struct task_struct,ptrace_list);
		reparent_thread(p, reaper, child_reaper);
	}

Thread group makes the child which links both ->children and
->ptrace_children.

>  {
> -	ptrace_unlink(p);
> -	list_del_init(&p->sibling);
> -	p->ptrace = 0;
> +	/* If we were tracing the thread, release it; otherwise preserve the
> +	   ptrace links.  */
> +	if (unlikely(traced)) {
> +		task_t *trace_task = p->parent;
> +		__ptrace_unlink(p);
> +		p->ptrace = 1;

Unexpected change of ptrace flag.

> +		__ptrace_link(p, trace_task);
> +	} else {
> +		p->ptrace = 0;
> +		list_del_init(&p->sibling);
> +		p->parent = p->real_parent;
> +		list_add_tail(&p->sibling, &p->parent->children);

Looks like that tracing child still link ->ptrace_list.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [patch] ptrace-fix-2.5.33-A1

[Daniel Jacobowitz]

Thanks for the comments.

On Sat, Sep 07, 2002 at 12:27:39AM +0900, OGAWA Hirofumi wrote:
> Ingo Molnar <mingo@elte.hu> writes:
> 
> > i've attached a combined patch of your two patches, against BK-curr. Looks
> > good to me, and since it passed your more complex ptrace tests ...
> > 
> > 	Ingo
> > 
> > --- linux/kernel/exit.c.orig	Fri Sep  6 00:55:02 2002
> > +++ linux/kernel/exit.c	Fri Sep  6 00:57:58 2002
> > @@ -66,6 +66,11 @@
> >  	atomic_dec(&p->user->processes);
> >  	security_ops->task_free_security(p);
> >  	free_uid(p->user);
> > +	if (unlikely(p->ptrace)) {
> > +		write_lock_irq(&tasklist_lock);
> > +		__ptrace_unlink(p);
> > +		write_unlock_irq(&tasklist_lock);
> > +	}
> >  	BUG_ON(!list_empty(&p->ptrace_list) || !list_empty(&p->ptrace_children));
> 
> Looks like it's need the only CLONE_DETACH process. Why it's here?

Because it's also needed for non-CLONE_DETACH processes.  I added it
earlier down by the release_task, remember?  I deleted it in this patch
originally, and the change got lost somewhere; my intent is to check
for this only in release_task and nowhere else.  When I have a clear
point to resync with Linus again then I'll make sure this is right.

> 	 * Search them and reparent children.
> 	 */
> 	list_for_each(_p, &father->children) {
> 		p = list_entry(_p,struct task_struct,sibling);
> 		reparent_thread(p, reaper, child_reaper);
> 	}
> 
> Looks like that tracer deprive a process from real parent.

Oh - when the tracer exits the original parent may be corrupted, you
mean?  I guess you're right.  But I've made so many changes to this bit
of code that I'd like to wait until it settles before we fix this -
it's not a new problem.

> 	list_for_each(_p, &father->ptrace_children) {
> 		p = list_entry(_p,struct task_struct,ptrace_list);
> 		reparent_thread(p, reaper, child_reaper);
> 	}
> 
> Thread group makes the child which links both ->children and
> ->ptrace_children.

I don't understand what you mean.

> >  {
> > -	ptrace_unlink(p);
> > -	list_del_init(&p->sibling);
> > -	p->ptrace = 0;
> > +	/* If we were tracing the thread, release it; otherwise preserve the
> > +	   ptrace links.  */
> > +	if (unlikely(traced)) {
> > +		task_t *trace_task = p->parent;
> > +		__ptrace_unlink(p);
> > +		p->ptrace = 1;
> 
> Unexpected change of ptrace flag.

I should've caught that, I actually use the ptrace flags here.  But the
code that uses them is suffering some other BUG() right now.

> > +		__ptrace_link(p, trace_task);
> > +	} else {
> > +		p->ptrace = 0;
> > +		list_del_init(&p->sibling);
> > +		p->parent = p->real_parent;
> > +		list_add_tail(&p->sibling, &p->parent->children);
> 
> Looks like that tracing child still link ->ptrace_list.

Right on both counts; how's this look (on top of the last patch):

===== exit.c 1.49 vs edited =====
--- 1.49/kernel/exit.c	Fri Sep  6 11:26:02 2002
+++ edited/exit.c	Fri Sep  6 11:37:50 2002
@@ -449,15 +449,19 @@
 
 static inline void zap_thread(task_t *p, task_t *father, int traced)
 {
-	/* If we were tracing the thread, release it; otherwise preserve the
-	   ptrace links.  */
+	/* 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 = 1;
+		p->ptrace = ptrace_flag;
 		__ptrace_link(p, trace_task);
 	} else {
-		p->ptrace = 0;
+		/* Otherwise, if we were tracing this thread, untrace it.  */
+		ptrace_unlink (p);
+
 		list_del_init(&p->sibling);
 		p->parent = p->real_parent;
 		list_add_tail(&p->sibling, &p->parent->children);

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch] ptrace-fix-2.5.33-A1

[Ingo Molnar]

On Fri, 6 Sep 2002, Daniel Jacobowitz wrote:

> Because it's also needed for non-CLONE_DETACH processes.  I added it
> earlier down by the release_task, remember?  I deleted it in this patch
> originally, and the change got lost somewhere; my intent is to check for
> this only in release_task and nowhere else.  When I have a clear point
> to resync with Linus again then I'll make sure this is right.

Linus' tree is at http://linux.bkbits.net/linux-2.5/, and i've done the
attached patch to collect & clean up all the things that happened since
yesterday. It's basically what was your tree this morning - i'd suggest
for you to base subsequent patches on BK-curr + this patch. (To reduce
confusion i wont send any new patches for this area from now on.)

	Ingo

diff -rNu linux.orig/kernel/exit.c linux/kernel/exit.c
--- linux.orig/kernel/exit.c	Fri Sep  6 11:37:53 2002
+++ linux/kernel/exit.c	Fri Sep  6 11:39:47 2002
@@ -66,8 +66,12 @@
 	atomic_dec(&p->user->processes);
 	security_ops->task_free_security(p);
 	free_uid(p->user);
-	BUG_ON(p->ptrace || !list_empty(&p->ptrace_list) ||
-					!list_empty(&p->ptrace_children));
+	if (unlikely(p->ptrace)) {
+		write_lock_irq(&tasklist_lock);
+		__ptrace_unlink(p);
+		write_unlock_irq(&tasklist_lock);
+	}
+	BUG_ON(!list_empty(&p->ptrace_list) || !list_empty(&p->ptrace_children));
 	unhash_process(p);
 
 	release_thread(p);
@@ -428,7 +432,7 @@
 	 * There are only two places where our children can be:
 	 *
 	 * - in our child list
-	 * - in the global ptrace list
+	 * - in our ptraced child list
 	 *
 	 * Search them and reparent children.
 	 */
@@ -443,14 +447,22 @@
 	read_unlock(&tasklist_lock);
 }
 
-static inline void zap_thread(task_t *p, task_t *father)
+static inline void zap_thread(task_t *p, task_t *father, int traced)
 {
-	ptrace_unlink(p);
-	list_del_init(&p->sibling);
-	p->ptrace = 0;
+	/* If we were tracing the thread, release it; otherwise preserve the
+	   ptrace links.  */
+	if (unlikely(traced)) {
+		task_t *trace_task = p->parent;
+		__ptrace_unlink(p);
+		p->ptrace = 1;
+		__ptrace_link(p, trace_task);
+	} else {
+		p->ptrace = 0;
+		list_del_init(&p->sibling);
+		p->parent = p->real_parent;
+		list_add_tail(&p->sibling, &p->parent->children);
+	}
 
-	p->parent = p->real_parent;
-	list_add_tail(&p->sibling, &p->parent->children);
 	if (p->state == TASK_ZOMBIE && p->exit_signal != -1)
 		do_notify_parent(p, p->exit_signal);
 	/*
@@ -541,11 +553,11 @@
 
 zap_again:
 	list_for_each_safe(_p, _n, &current->children)
-		zap_thread(list_entry(_p,struct task_struct,sibling), current);
+		zap_thread(list_entry(_p,struct task_struct,sibling), current, 0);
 	list_for_each_safe(_p, _n, &current->ptrace_children)
-		zap_thread(list_entry(_p,struct task_struct,ptrace_list), current);
+		zap_thread(list_entry(_p,struct task_struct,ptrace_list), current, 1);
 	/*
-	 * reparent_thread might drop the tasklist lock, thus we could
+	 * zap_thread might drop the tasklist lock, thus we could
 	 * have new children queued back from the ptrace list into the
 	 * child list:
 	 */
@@ -716,17 +728,11 @@
 				retval = p->pid;
 				if (p->real_parent != p->parent) {
 					write_lock_irq(&tasklist_lock);
-					ptrace_unlink(p);
+					__ptrace_unlink(p);
 					do_notify_parent(p, SIGCHLD);
 					write_unlock_irq(&tasklist_lock);
-				} else {
-					if (p->ptrace) {
-						write_lock_irq(&tasklist_lock);
-						ptrace_unlink(p);
-						write_unlock_irq(&tasklist_lock);
-					}
+				} else
 					release_task(p);
-				}
 				goto end_wait4;
 			default:
 				continue;

Re: [patch] ptrace-fix-2.5.33-A1

[OGAWA Hirofumi]

Daniel Jacobowitz <dan@debian.org> writes:

> > 	 * Search them and reparent children.
> > 	 */
> > 	list_for_each(_p, &father->children) {
> > 		p = list_entry(_p,struct task_struct,sibling);
> > 		reparent_thread(p, reaper, child_reaper);
> > 	}
> > 
> > Looks like that tracer deprive a process from real parent.
> 
> Oh - when the tracer exits the original parent may be corrupted, you
> mean?  I guess you're right.  But I've made so many changes to this bit
> of code that I'd like to wait until it settles before we fix this -
> it's not a new problem.

Yes, this isn't new problem. However, since other place may be
affected by this, I think should fix this first.

> > 	list_for_each(_p, &father->ptrace_children) {
> > 		p = list_entry(_p,struct task_struct,ptrace_list);
> > 		reparent_thread(p, reaper, child_reaper);
> > 	}
> > 
> > Thread group makes the child which links both ->children and
> > ->ptrace_children.
> 
> I don't understand what you mean.

Sorry, forget this.

> > >  {
> > > -	ptrace_unlink(p);
> > > -	list_del_init(&p->sibling);
> > > -	p->ptrace = 0;
> > > +	/* If we were tracing the thread, release it; otherwise preserve the
> > > +	   ptrace links.  */
> > > +	if (unlikely(traced)) {
> > > +		task_t *trace_task = p->parent;
> > > +		__ptrace_unlink(p);
> > > +		p->ptrace = 1;
> > 
> > Unexpected change of ptrace flag.
> 
> I should've caught that, I actually use the ptrace flags here.  But the
> code that uses them is suffering some other BUG() right now.

I forgot I say another point. This path shouldn't send signal to
parent.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [patch] ptrace-fix-2.5.33-A1

[Ingo Molnar]

> > > Right - let me rephrase.  Tasks which are either:
> > >   - untraced, normal
> > >   - traced, but traced _by their parent_
> > > are on the sibling/children list.

okay, agreed, this description is correct. Obviously if the parent is the
debugger then a traced task will be in the ->children list.

	Ingo

Re: [patch] ptrace-fix-2.5.33-A1

[Daniel Jacobowitz]

On Fri, Sep 06, 2002 at 12:25:32AM +0200, Ingo Molnar wrote:
> > > this splitup of the lists makes it possible for the debugger to do a wait4
> > > that will get events from the debugged task, and for the debugged task to
> > > also be available to the real parent.
> > 
> > Great.  I'm not exactly sure on how this works right now: sys_wait4 only
> > iterates over ->children, with the exception of the special code in
> > TASK_ZOMBIE.  I'm not quite sure when events from a traced process get
> > to the normal parent of that process, or when they're supposed to.
> 
> i'm not sure about this either. What happens if an (untraced) parent has
> traced and untraced children, and does a wait4. Would it confuse the
> debugger if the parent could get one of the traced tasks as a result in
> wait4? And how does the debugger solve this problem?

Not a problem.  The parent never gets an event that is not first
reported to the debugger, via the hook in sys_wait4... A debugger that
runs other processes has to play some games with wait loops in order to
manage its children and its debugees, but that's old news.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch] ptrace-fix-2.5.33-A1

[Ingo Molnar]

there are two kinds of wait4 calls, one that gets the WIFSTOPPED event
from the debugged task, for this the traced task has to be in the
debugger's ->children list.

Another one is when a debugged task exits and its parent wants the exit
event. But in this case the task is untraced already, so it gets back into
the parent's ->children list.

ie. wait4 should only look at the ->children list - zombies (or traced
tasks debugged by this task) can only be there.

The only addition is that in the wait4 non-blocking case we need to look
at the traced list as well - since a non-blocking wait4 is a 'could there
be any children exiting' kind of query.

	Ingo

Re: [patch] ptrace-fix-2.5.33-A1

[Daniel Jacobowitz]

On Fri, Sep 06, 2002 at 12:35:22AM +0200, Ingo Molnar wrote:
> 
> there are two kinds of wait4 calls, one that gets the WIFSTOPPED event
> from the debugged task, for this the traced task has to be in the
> debugger's ->children list.
> 
> Another one is when a debugged task exits and its parent wants the exit
> event. But in this case the task is untraced already, so it gets back into
> the parent's ->children list.
> 
> ie. wait4 should only look at the ->children list - zombies (or traced
> tasks debugged by this task) can only be there.
> 
> The only addition is that in the wait4 non-blocking case we need to look
> at the traced list as well - since a non-blocking wait4 is a 'could there
> be any children exiting' kind of query.

OK.  I think that the !list_empty (ptrace_children) isn't really enough
- since there can be things on our children list that we will not wait
for - should we be iterating over it making the same checks we do
above?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch] ptrace-fix-2.5.33-A1

[Ingo Molnar]

On Thu, 5 Sep 2002, Daniel Jacobowitz wrote:

> OK.  I think that the !list_empty (ptrace_children) isn't really enough
> - since there can be things on our children list that we will not wait
> for - should we be iterating over it making the same checks we do above?

yes, i think so.

	Ingo

Re: [patch] ptrace-fix-2.5.33-A1

[Ingo Molnar]

> > tsk->children: tsk's children, which are either untraced or traced by
> > 	tsk.  They have p->parent == p->real_parent == tsk.
> > 	Chained in p->sibling.
> 
> no - the way i wrote it originally was that only untraced children should
> be on the tsk->children list. Traced tasks will be on the debugger's
> ->children list, plus will be on the real parent's ->ptrace_children list.

i cant see how a ->children list that includes all children (traced and
untraced) can be useful. In fact it can be harmful: the debugger needs the
traced children to be on his own child-list, ie. there's a conflict of use
on the ->sibling list field. The ->ptrace_list entry is supposed to help
this situation - obviously use of ->ptrace_list and ->sibling has to be
mutually exclusive, no two parents can have the same task linked over
->sibling.

	Ingo

[patch] ptrace-fix-2.5.33-A1

[Ingo Molnar]

Linus,

the attached patch (against BK-curr) collects two ptrace related fixes:  
first it undoes Ogawa's change (so various uses of ptrace works again),
plus it adds Daniel's suggested fix that allows a parent to PTRACE_ATTACH
to a child it forked. (this also fixes the incorrect BUG_ON() assert
Ogawa's patch was intended to fix in the first place.)

i've tested various ptrace uses and they appear to work just fine.

(Daniel, let us know if you can still see anything questionable in this
area - or if the ptrace list could be managed in a cleaner way.)

please apply,

	Ingo

--- linux/kernel/exit.c.orig	Thu Sep  5 17:25:47 2002
+++ linux/kernel/exit.c	Thu Sep  5 17:27:06 2002
@@ -66,7 +66,8 @@
 	atomic_dec(&p->user->processes);
 	security_ops->task_free_security(p);
 	free_uid(p->user);
-	BUG_ON(!list_empty(&p->ptrace_list) || !list_empty(&p->ptrace_children));
+	BUG_ON(p->ptrace || !list_empty(&p->ptrace_list) ||
+					!list_empty(&p->ptrace_children));
 	unhash_process(p);
 
 	release_thread(p);
@@ -718,8 +719,14 @@
 					ptrace_unlink(p);
 					do_notify_parent(p, SIGCHLD);
 					write_unlock_irq(&tasklist_lock);
-				} else
+				} else {
+					if (p->ptrace) {
+						write_lock_irq(&tasklist_lock);
+						ptrace_unlink(p);
+						write_unlock_irq(&tasklist_lock);
+					}
 					release_task(p);
+				}
 				goto end_wait4;
 			default:
 				continue;
--- linux/kernel/ptrace.c.orig	Thu Sep  5 17:28:40 2002
+++ linux/kernel/ptrace.c	Thu Sep  5 17:28:47 2002
@@ -29,7 +29,7 @@
 	if (!list_empty(&child->ptrace_list))
 		BUG();
 	if (child->parent == new_parent)
-		BUG();
+		return;
 	list_add(&child->ptrace_list, &child->parent->ptrace_children);
 	REMOVE_LINKS(child);
 	child->parent = new_parent;

Re: [patch] ptrace-fix-2.5.33-A1

[OGAWA Hirofumi]

Ingo Molnar <mingo@elte.hu> writes:

> Linus,
> 
> the attached patch (against BK-curr) collects two ptrace related fixes:  
> first it undoes Ogawa's change (so various uses of ptrace works again),
> plus it adds Daniel's suggested fix that allows a parent to PTRACE_ATTACH
> to a child it forked. (this also fixes the incorrect BUG_ON() assert
> Ogawa's patch was intended to fix in the first place.)
> 
> i've tested various ptrace uses and they appear to work just fine.
> 
> (Daniel, let us know if you can still see anything questionable in this
> area - or if the ptrace list could be managed in a cleaner way.)

I think I found some bugs.

in forget_original_parent()

	/*
	 * There are only two places where our children can be:
	 *
	 * - in our child list
	 * - in the global ptrace list
	 *
	 * Search them and reparent children.
	 */
	list_for_each(_p, &father->children) {
		p = list_entry(_p,struct task_struct,sibling);
		reparent_thread(p, reaper, child_reaper);
	}

Looks like that tracer change the real parent.


in exit_notify()

	list_for_each_safe(_p, _n, &current->ptrace_children)
		zap_thread(list_entry(_p,struct task_struct,ptrace_list), current);

Looks like that real parent deprive a process from tracer.


in sys_wait4()

+				} else {
+					if (p->ptrace) {
+						write_lock_irq(&tasklist_lock);
+						ptrace_unlink(p);
+						write_unlock_irq(&tasklist_lock);
+					}
 					release_task(p);
+				}

Umm, why needed this? If ->real_parent == ->parent, it's real
child. So this child don't use ->ptrace_list.


			break;
		tsk = next_thread(tsk);
	} while (tsk != current);
	read_unlock(&tasklist_lock);
	if (flag || !list_empty(&current->ptrace_children)) {

Until now, this case wasn't blocked. However, I like this behavior.


If these are right, can you look the following patch? I think this patch 
fixes problems.

===== kernel/exit.c 1.43 vs edited =====
--- 1.43/kernel/exit.c	Mon Sep  2 00:54:47 2002
+++ edited/kernel/exit.c	Fri Sep  6 01:58:52 2002
@@ -403,10 +403,10 @@
  * group, and if no such member exists, give it to
  * the global child reaper process (ie "init")
  */
-static inline void forget_original_parent(struct task_struct * father)
+static inline void forget_real_parent(struct task_struct * father)
 {
 	struct task_struct *p, *reaper;
-	list_t *_p;
+	list_t *_p, *_n;
 
 	read_lock(&tasklist_lock);
 
@@ -425,17 +425,24 @@
 	 * There are only two places where our children can be:
 	 *
 	 * - in our child list
-	 * - in the global ptrace list
+	 * - in the ptraced child list
 	 *
 	 * Search them and reparent children.
 	 */
 	list_for_each(_p, &father->children) {
 		p = list_entry(_p,struct task_struct,sibling);
-		reparent_thread(p, reaper, child_reaper);
+		if (p->real_parent == father)
+			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);
+
+		/* This is needed for thread group reparent */
+		if (p->real_parent != child_reaper &&
+		    p->real_parent != p->parent)
+			list_add(&p->ptrace_list, &p->real_parent->ptrace_children);
 	}
 	read_unlock(&tasklist_lock);
 }
@@ -443,9 +450,8 @@
 static inline void zap_thread(task_t *p, task_t *father)
 {
 	ptrace_unlink(p);
-	list_del_init(&p->sibling);
-	p->ptrace = 0;
 
+	list_del_init(&p->sibling);
 	p->parent = p->real_parent;
 	list_add_tail(&p->sibling, &p->parent->children);
 	if (p->state == TASK_ZOMBIE && p->exit_signal != -1)
@@ -478,7 +484,7 @@
 	struct task_struct *t;
 	list_t *_p, *_n;
 
-	forget_original_parent(current);
+	forget_real_parent(current);
 	/*
 	 * Check to see if any process groups have become orphaned
 	 * as a result of our exiting, and if they have any stopped
@@ -539,15 +545,13 @@
 zap_again:
 	list_for_each_safe(_p, _n, &current->children)
 		zap_thread(list_entry(_p,struct task_struct,sibling), current);
-	list_for_each_safe(_p, _n, &current->ptrace_children)
-		zap_thread(list_entry(_p,struct task_struct,ptrace_list), current);
+
 	/*
 	 * reparent_thread might drop the tasklist lock, thus we could
 	 * have new children queued back from the ptrace list into the
 	 * child list:
 	 */
-	if (unlikely(!list_empty(&current->children) ||
-			!list_empty(&current->ptrace_children)))
+	if (unlikely(!list_empty(&current->children)))
 		goto zap_again;
 	/*
 	 * No need to unlock IRQs, we'll schedule() immediately
@@ -598,8 +602,10 @@
 	tsk->exit_code = code;
 	exit_notify();
 	preempt_disable();
-	if (current->exit_signal == -1)
+	if (current->exit_signal == -1) {
+		ptrace_unlink(current);
 		release_task(current);
+	}
 	schedule();
 	BUG();
 /*
===== kernel/ptrace.c 1.16 vs edited =====
--- 1.16/kernel/ptrace.c	Tue Aug 20 03:12:27 2002
+++ edited/kernel/ptrace.c	Wed Sep  4 03:00:53 2002
@@ -26,11 +26,12 @@
  */
 void __ptrace_link(task_t *child, task_t *new_parent)
 {
-	if (!list_empty(&child->ptrace_list))
-		BUG();
+	BUG_ON(!list_empty(&child->ptrace_list));
+	BUG_ON(child->parent != child->real_parent);
+
 	if (child->parent == new_parent)
-		BUG();
-	list_add(&child->ptrace_list, &child->parent->ptrace_children);
+		return;
+	list_add(&child->ptrace_list, &child->real_parent->ptrace_children);
 	REMOVE_LINKS(child);
 	child->parent = new_parent;
 	SET_LINKS(child);
@@ -44,10 +45,10 @@
  */
 void __ptrace_unlink(task_t *child)
 {
-	if (!child->ptrace)
-		BUG();
+	BUG_ON(!child->ptrace);
+
 	child->ptrace = 0;
-	if (list_empty(&child->ptrace_list))
+	if (child->parent == child->real_parent)
 		return;
 	list_del_init(&child->ptrace_list);
 	REMOVE_LINKS(child);
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [patch] ptrace-fix-2.5.33-A1

[Daniel Jacobowitz]

I'll reply to the rest of this in a moment, but one thing at a time...

On Fri, Sep 06, 2002 at 02:08:13AM +0900, OGAWA Hirofumi wrote:
> Ingo Molnar <mingo@elte.hu> writes:
> 
> > Linus,
> > 
> > the attached patch (against BK-curr) collects two ptrace related fixes:  
> > first it undoes Ogawa's change (so various uses of ptrace works again),
> > plus it adds Daniel's suggested fix that allows a parent to PTRACE_ATTACH
> > to a child it forked. (this also fixes the incorrect BUG_ON() assert
> > Ogawa's patch was intended to fix in the first place.)
> > 
> > i've tested various ptrace uses and they appear to work just fine.
> > 
> > (Daniel, let us know if you can still see anything questionable in this
> > area - or if the ptrace list could be managed in a cleaner way.)


> in sys_wait4()
> 
> +				} else {
> +					if (p->ptrace) {
> +						write_lock_irq(&tasklist_lock);
> +						ptrace_unlink(p);
> +						write_unlock_irq(&tasklist_lock);
> +					}
>  					release_task(p);
> +				}
> 
> Umm, why needed this? If ->real_parent == ->parent, it's real
> child. So this child don't use ->ptrace_list.

You're right, we just need to clear p->ptrace.  And there was a problem
with debugged detached tasks.  Ingo, does this look right to you?  It
passes my testing.  Handle unlinking in release_task instead of at both
call sites, since they both need it.

===== exit.c 1.45 vs edited =====
*** /tmp/exit.c-1.45-26998	Mon Sep  2 01:15:09 2002
--- exit.c	Thu Sep  5 14:23:32 2002
*************** static void release_task(struct task_str
*** 66,71 ****
--- 66,76 ----
  	atomic_dec(&p->user->processes);
  	security_ops->task_free_security(p);
  	free_uid(p->user);
+ 	if (unlikely(p->ptrace)) {
+ 		write_lock_irq(&tasklist_lock);
+ 		ptrace_unlink(p);
+ 		write_unlock_irq(&tasklist_lock);
+ 	}
  	BUG_ON(!list_empty(&p->ptrace_list) || !list_empty(&p->ptrace_children));
  	unhash_process(p);
  
===== ptrace.c 1.16 vs edited =====
*** /tmp/ptrace.c-1.16-26998	Mon Aug 19 14:12:27 2002
--- ptrace.c	Thu Sep  5 14:18:05 2002
*************** void __ptrace_link(task_t *child, task_t
*** 29,35 ****
  	if (!list_empty(&child->ptrace_list))
  		BUG();
  	if (child->parent == new_parent)
! 		BUG();
  	list_add(&child->ptrace_list, &child->parent->ptrace_children);
  	REMOVE_LINKS(child);
  	child->parent = new_parent;
--- 29,35 ----
  	if (!list_empty(&child->ptrace_list))
  		BUG();
  	if (child->parent == new_parent)
! 		return;
  	list_add(&child->ptrace_list, &child->parent->ptrace_children);
  	REMOVE_LINKS(child);
  	child->parent = new_parent;


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch] ptrace-fix-2.5.33-A1

[Ingo Molnar]

On Thu, 5 Sep 2002, Daniel Jacobowitz wrote:

> You're right, we just need to clear p->ptrace.  And there was a problem
> with debugged detached tasks.  Ingo, does this look right to you?  It
> passes my testing.  Handle unlinking in release_task instead of at both
> call sites, since they both need it.

your patch works for me - in fact it works better than the one i sent. I
had another (largely unrelated) patch which i suspected to cause hung
unkillable zombies while stracing an application - it turned out that your
patch fixes this artifact as well.

(Linus, please apply Daniel's patch, not mine.)

	Ingo

Re: [patch] ptrace-fix-2.5.33-A1

[Daniel Jacobowitz]

Linus, please apply this patch.  It fixes debugging a process when the
process's original parent exits; we shouldn't detach the debugger.

On Fri, Sep 06, 2002 at 02:08:13AM +0900, OGAWA Hirofumi wrote:
> Ingo Molnar <mingo@elte.hu> writes:
> 
> > Linus,
> > 
> > the attached patch (against BK-curr) collects two ptrace related fixes:  
> > first it undoes Ogawa's change (so various uses of ptrace works again),
> > plus it adds Daniel's suggested fix that allows a parent to PTRACE_ATTACH
> > to a child it forked. (this also fixes the incorrect BUG_ON() assert
> > Ogawa's patch was intended to fix in the first place.)
> > 
> > i've tested various ptrace uses and they appear to work just fine.
> > 
> > (Daniel, let us know if you can still see anything questionable in this
> > area - or if the ptrace list could be managed in a cleaner way.)
> 
> I think I found some bugs.

There's definitely still something wrong... let me just run through my
understanding of these lists, to make sure we're on the same page.

tsk->children: tsk's children, which are either untraced or traced by
	tsk.  They have p->parent == p->real_parent == tsk.
	Chained in p->sibling.
tsk->ptrace_children: tsk's children, which are traced by some other
	process.  They have p->real_parent == tsk and p->parent != tsk.
	Chained in p->ptrace_list.

When a parent dies, its traced children should continue to be traced
even though they are reparented.  That's broken right now - you can
test that easily enough.  When a tracer dies, all processes it is
tracing should be marked untraced; that's also broken right now, I
think but have not tested.

> in sys_wait4()
> 
> +				} else {
> +					if (p->ptrace) {
> +						write_lock_irq(&tasklist_lock);
> +						ptrace_unlink(p);
> +						write_unlock_irq(&tasklist_lock);
> +					}
>  					release_task(p);
> +				}
> 
> Umm, why needed this? If ->real_parent == ->parent, it's real
> child. So this child don't use ->ptrace_list.

You're right.  I was just using ptrace_unlink to clear p->ptrace. 
Fixed in my earlier patch.

>  	list_for_each(_p, &father->children) {
>  		p = list_entry(_p,struct task_struct,sibling);
> -		reparent_thread(p, reaper, child_reaper);
> +		if (p->real_parent == father)
> +			reparent_thread(p, reaper, child_reaper);
>  	}

This should never be necessary.  If something is on the ->children
list, p->parent == father.  There should be no exceptions until after
reparent_thread; do you see one?

> -	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);
> +
> +		/* This is needed for thread group reparent */
> +		if (p->real_parent != child_reaper &&
> +		    p->real_parent != p->parent)
> +			list_add(&p->ptrace_list, &p->real_parent->ptrace_children);
>  	}
>  	read_unlock(&tasklist_lock);
>  }

Something is wrong here but I don't think this is the right place to
fix it - it isn't safe.  If we have traced children, and their parent
dies... well, currently they become untraced, and that certainly is not
right.  I like this patch a little more; cleaner and saves some cycles
(probably).  Passed my stress testing with flying colors.

===== kernel/exit.c 1.46 vs edited =====
*** /tmp/exit.c-1.46-10686	Thu Sep  5 14:41:56 2002
--- kernel/exit.c	Thu Sep  5 16:58:30 2002
*************** static void release_task(struct task_str
*** 68,74 ****
  	free_uid(p->user);
  	if (unlikely(p->ptrace)) {
  		write_lock_irq(&tasklist_lock);
! 		ptrace_unlink(p);
  		write_unlock_irq(&tasklist_lock);
  	}
  	BUG_ON(!list_empty(&p->ptrace_list) || !list_empty(&p->ptrace_children));
--- 68,74 ----
  	free_uid(p->user);
  	if (unlikely(p->ptrace)) {
  		write_lock_irq(&tasklist_lock);
! 		__ptrace_unlink(p);
  		write_unlock_irq(&tasklist_lock);
  	}
  	BUG_ON(!list_empty(&p->ptrace_list) || !list_empty(&p->ptrace_children));
*************** static inline void forget_original_paren
*** 432,438 ****
  	 * There are only two places where our children can be:
  	 *
  	 * - in our child list
! 	 * - in the global ptrace list
  	 *
  	 * Search them and reparent children.
  	 */
--- 432,438 ----
  	 * There are only two places where our children can be:
  	 *
  	 * - in our child list
! 	 * - in our ptraced child list
  	 *
  	 * Search them and reparent children.
  	 */
*************** static inline void forget_original_paren
*** 447,460 ****
  	read_unlock(&tasklist_lock);
  }
  
! static inline void zap_thread(task_t *p, task_t *father)
  {
! 	ptrace_unlink(p);
! 	list_del_init(&p->sibling);
! 	p->ptrace = 0;
  
- 	p->parent = p->real_parent;
- 	list_add_tail(&p->sibling, &p->parent->children);
  	if (p->state == TASK_ZOMBIE && p->exit_signal != -1)
  		do_notify_parent(p, p->exit_signal);
  	/*
--- 447,468 ----
  	read_unlock(&tasklist_lock);
  }
  
! static inline void zap_thread(task_t *p, task_t *father, int traced)
  {
! 	/* If we were tracing the thread, release it; otherwise preserve the
! 	   ptrace links.  */
! 	if (unlikely(traced)) {
! 		task_t *trace_task = p->parent;
! 		__ptrace_unlink(p);
! 		p->ptrace = 1;
! 		__ptrace_link(p, trace_task);
! 	} else {
! 		p->ptrace = 0;
! 		list_del_init(&p->sibling);
! 		p->parent = p->real_parent;
! 		list_add_tail(&p->sibling, &p->parent->children);
! 	}
  
  	if (p->state == TASK_ZOMBIE && p->exit_signal != -1)
  		do_notify_parent(p, p->exit_signal);
  	/*
*************** static void exit_notify(void)
*** 545,555 ****
  
  zap_again:
  	list_for_each_safe(_p, _n, &current->children)
! 		zap_thread(list_entry(_p,struct task_struct,sibling), current);
  	list_for_each_safe(_p, _n, &current->ptrace_children)
! 		zap_thread(list_entry(_p,struct task_struct,ptrace_list), current);
  	/*
! 	 * reparent_thread might drop the tasklist lock, thus we could
  	 * have new children queued back from the ptrace list into the
  	 * child list:
  	 */
--- 553,563 ----
  
  zap_again:
  	list_for_each_safe(_p, _n, &current->children)
! 		zap_thread(list_entry(_p,struct task_struct,sibling), current, 0);
  	list_for_each_safe(_p, _n, &current->ptrace_children)
! 		zap_thread(list_entry(_p,struct task_struct,ptrace_list), current, 1);
  	/*
! 	 * zap_thread might drop the tasklist lock, thus we could
  	 * have new children queued back from the ptrace list into the
  	 * child list:
  	 */
*************** repeat:
*** 720,726 ****
  				retval = p->pid;
  				if (p->real_parent != p->parent) {
  					write_lock_irq(&tasklist_lock);
! 					ptrace_unlink(p);
  					do_notify_parent(p, SIGCHLD);
  					write_unlock_irq(&tasklist_lock);
  				} else
--- 728,734 ----
  				retval = p->pid;
  				if (p->real_parent != p->parent) {
  					write_lock_irq(&tasklist_lock);
! 					__ptrace_unlink(p);
  					do_notify_parent(p, SIGCHLD);
  					write_unlock_irq(&tasklist_lock);
  				} else


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch] ptrace-fix-2.5.33-A1

[Daniel Jacobowitz]

On Thu, Sep 05, 2002 at 05:44:59PM -0400, Daniel Jacobowitz wrote:
> Linus, please apply this patch.  It fixes debugging a process when the
> process's original parent exits; we shouldn't detach the debugger.

Or this copy, properly unified.  Larry, it would be nice if 'bk diff
-up' behaved like 'diff -up' does - a unified diff with function
markers.

===== exit.c 1.46 vs 1.47 =====
--- 1.46/kernel/exit.c	Thu Sep  5 14:41:56 2002
+++ 1.47/kernel/exit.c	Thu Sep  5 17:23:57 2002
@@ -68,7 +68,7 @@
 	free_uid(p->user);
 	if (unlikely(p->ptrace)) {
 		write_lock_irq(&tasklist_lock);
-		ptrace_unlink(p);
+		__ptrace_unlink(p);
 		write_unlock_irq(&tasklist_lock);
 	}
 	BUG_ON(!list_empty(&p->ptrace_list) || !list_empty(&p->ptrace_children));
@@ -432,7 +432,7 @@
 	 * There are only two places where our children can be:
 	 *
 	 * - in our child list
-	 * - in the global ptrace list
+	 * - in our ptraced child list
 	 *
 	 * Search them and reparent children.
 	 */
@@ -447,14 +447,22 @@
 	read_unlock(&tasklist_lock);
 }
 
-static inline void zap_thread(task_t *p, task_t *father)
+static inline void zap_thread(task_t *p, task_t *father, int traced)
 {
-	ptrace_unlink(p);
-	list_del_init(&p->sibling);
-	p->ptrace = 0;
+	/* If we were tracing the thread, release it; otherwise preserve the
+	   ptrace links.  */
+	if (unlikely(traced)) {
+		task_t *trace_task = p->parent;
+		__ptrace_unlink(p);
+		p->ptrace = 1;
+		__ptrace_link(p, trace_task);
+	} else {
+		p->ptrace = 0;
+		list_del_init(&p->sibling);
+		p->parent = p->real_parent;
+		list_add_tail(&p->sibling, &p->parent->children);
+	}
 
-	p->parent = p->real_parent;
-	list_add_tail(&p->sibling, &p->parent->children);
 	if (p->state == TASK_ZOMBIE && p->exit_signal != -1)
 		do_notify_parent(p, p->exit_signal);
 	/*
@@ -545,11 +553,11 @@
 
 zap_again:
 	list_for_each_safe(_p, _n, &current->children)
-		zap_thread(list_entry(_p,struct task_struct,sibling), current);
+		zap_thread(list_entry(_p,struct task_struct,sibling), current, 0);
 	list_for_each_safe(_p, _n, &current->ptrace_children)
-		zap_thread(list_entry(_p,struct task_struct,ptrace_list), current);
+		zap_thread(list_entry(_p,struct task_struct,ptrace_list), current, 1);
 	/*
-	 * reparent_thread might drop the tasklist lock, thus we could
+	 * zap_thread might drop the tasklist lock, thus we could
 	 * have new children queued back from the ptrace list into the
 	 * child list:
 	 */
@@ -720,7 +728,7 @@
 				retval = p->pid;
 				if (p->real_parent != p->parent) {
 					write_lock_irq(&tasklist_lock);
-					ptrace_unlink(p);
+					__ptrace_unlink(p);
 					do_notify_parent(p, SIGCHLD);
 					write_unlock_irq(&tasklist_lock);
 				} else



> 
> On Fri, Sep 06, 2002 at 02:08:13AM +0900, OGAWA Hirofumi wrote:
> > Ingo Molnar <mingo@elte.hu> writes:
> > 
> > > Linus,
> > > 
> > > the attached patch (against BK-curr) collects two ptrace related fixes:  
> > > first it undoes Ogawa's change (so various uses of ptrace works again),
> > > plus it adds Daniel's suggested fix that allows a parent to PTRACE_ATTACH
> > > to a child it forked. (this also fixes the incorrect BUG_ON() assert
> > > Ogawa's patch was intended to fix in the first place.)
> > > 
> > > i've tested various ptrace uses and they appear to work just fine.
> > > 
> > > (Daniel, let us know if you can still see anything questionable in this
> > > area - or if the ptrace list could be managed in a cleaner way.)
> > 
> > I think I found some bugs.
> 
> There's definitely still something wrong... let me just run through my
> understanding of these lists, to make sure we're on the same page.
> 
> tsk->children: tsk's children, which are either untraced or traced by
> 	tsk.  They have p->parent == p->real_parent == tsk.
> 	Chained in p->sibling.
> tsk->ptrace_children: tsk's children, which are traced by some other
> 	process.  They have p->real_parent == tsk and p->parent != tsk.
> 	Chained in p->ptrace_list.
> 
> When a parent dies, its traced children should continue to be traced
> even though they are reparented.  That's broken right now - you can
> test that easily enough.  When a tracer dies, all processes it is
> tracing should be marked untraced; that's also broken right now, I
> think but have not tested.
> 
> > in sys_wait4()
> > 
> > +				} else {
> > +					if (p->ptrace) {
> > +						write_lock_irq(&tasklist_lock);
> > +						ptrace_unlink(p);
> > +						write_unlock_irq(&tasklist_lock);
> > +					}
> >  					release_task(p);
> > +				}
> > 
> > Umm, why needed this? If ->real_parent == ->parent, it's real
> > child. So this child don't use ->ptrace_list.
> 
> You're right.  I was just using ptrace_unlink to clear p->ptrace. 
> Fixed in my earlier patch.
> 
> >  	list_for_each(_p, &father->children) {
> >  		p = list_entry(_p,struct task_struct,sibling);
> > -		reparent_thread(p, reaper, child_reaper);
> > +		if (p->real_parent == father)
> > +			reparent_thread(p, reaper, child_reaper);
> >  	}
> 
> This should never be necessary.  If something is on the ->children
> list, p->parent == father.  There should be no exceptions until after
> reparent_thread; do you see one?
> 
> > -	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);
> > +
> > +		/* This is needed for thread group reparent */
> > +		if (p->real_parent != child_reaper &&
> > +		    p->real_parent != p->parent)
> > +			list_add(&p->ptrace_list, &p->real_parent->ptrace_children);
> >  	}
> >  	read_unlock(&tasklist_lock);
> >  }
> 
> Something is wrong here but I don't think this is the right place to
> fix it - it isn't safe.  If we have traced children, and their parent
> dies... well, currently they become untraced, and that certainly is not
> right.  I like this patch a little more; cleaner and saves some cycles
> (probably).  Passed my stress testing with flying colors.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer