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, ¤t->children)
! zap_thread(list_entry(_p,struct task_struct,sibling), current);
list_for_each_safe(_p, _n, ¤t->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, ¤t->children)
! zap_thread(list_entry(_p,struct task_struct,sibling), current, 0);
list_for_each_safe(_p, _n, ¤t->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 - 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:27 EST