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

[Daniel Jacobowitz <dan@debian.org>]

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