Messages in this thread Patch in this message | | | Date | Fri, 6 Sep 2002 11:45:08 -0400 | From | Daniel Jacobowitz <> | Subject | Re: [patch] ptrace-fix-2.5.33-A1 |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |