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/
This archive was generated by hypermail 2b29 : Sat Sep 07 2002 - 22:00:29 EST