Messages in this thread |  | | Date | Mon, 16 Sep 2002 09:07:35 -0400 | From | Daniel Jacobowitz <> | Subject | Re: [PATCH] Fix for ptrace breakage |
| |
On Mon, Sep 16, 2002 at 09:44:34PM +0900, OGAWA Hirofumi wrote: > OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes: > > > Grr, sorry. This patch is bad version. > > > > list_for_each(_p, &father->ptrace_children) { > > > > of course, this should > > > > list_for_each_safe(_p, _n, &father->ptrace_children) { > > This is a patch which fixed the above. Please apply.
Some comments. First of all, you said you fixed a race on current->ptrace and some other bugs - would you mind saying where they were? It's definitely cleaner after your patch but I'd like to understand where you found bugs, since I think you're introducing more.
> -- > OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> > > --- linux-2.5.35/kernel/exit.c~ 2002-09-16 17:41:59.000000000 +0900 > +++ linux-2.5.35/kernel/exit.c 2002-09-16 20:55:43.000000000 +0900 > @@ -221,7 +221,6 @@ void reparent_to_init(void) > /* Set the exit signal to SIGCHLD so we signal init on exit */ > current->exit_signal = SIGCHLD; > > - current->ptrace = 0; > if ((current->policy == SCHED_NORMAL) && (task_nice(current) < 0)) > set_user_nice(current, 0); > /* cpus_allowed? */
If this is unnecessary, perhaps a BUG_ON()? This function is called from daemonize at which point the ptrace flag should be clear, and that's it.
> @@ -443,7 +442,7 @@ void exit_mm(struct task_struct *tsk) > static inline void forget_original_parent(struct task_struct * father) > { > struct task_struct *p, *reaper = father; > - struct list_head *_p; > + struct list_head *_p, *_n; > > reaper = father->group_leader; > if (reaper == father) > @@ -462,52 +461,37 @@ static inline void forget_original_paren > if (father == p->real_parent) > 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); > + if (p->parent != p->real_parent) > + list_add(&p->ptrace_list, &p->real_parent->ptrace_children); > } > }
So you reparent children on the ptrace_list right here. But they still need to go through zap_thread! You're right, the do_notify_parent in zap_thread isn't necessary; it'll be taken care of in sys_wait4. The orphaned pgrp check is still relevant though.
> -static inline void zap_thread(task_t *p, task_t *father, int traced) > +static inline void zap_thread(task_t *p, task_t *father) > { > - /* 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 = ptrace_flag; > - __ptrace_link(p, trace_task); > - } else { > - /* > - * Otherwise, if we were tracing this thread, untrace it. > - * If we were only tracing the thread (i.e. not its real > - * parent), stop here. > - */ > - ptrace_unlink (p); > - if (p->parent != father) { > - BUG_ON(p->parent != p->real_parent); > - return; > - } > - list_del_init(&p->sibling); > - p->parent = p->real_parent; > - list_add_tail(&p->sibling, &p->parent->children); > - } > + ptrace_unlink(p); > > + remove_parent(p); > + p->parent = p->real_parent; > + add_parent(p, p->parent); > if (p->state == TASK_ZOMBIE && p->exit_signal != -1) > do_notify_parent(p, p->exit_signal); > +
If you're going to remove the if, you need to maintain its effect! See: > - if (p->parent != father) { > - BUG_ON(p->parent != p->real_parent); > - return; > - }
This is the case where we were tracing something. The ptrace_unlink returned it to its original parent. It doesn't need the remove_parent/add_parent (though they are harmless); it does need to avoid the orphaned pgrp check. It may need the do_notify_parent check, which was a bug in the previous code.
> /* > * process group orphan check > * Case ii: Our child is in a different pgrp > * than we are, and it was the only connection > * outside, so the child pgrp is now orphaned. > */ > - if ((p->pgrp != current->pgrp) && > - (p->session == current->session)) { > + if ((p->pgrp != father->pgrp) && > + (p->session == father->session)) { > int pgrp = p->pgrp; > > - if (__will_become_orphaned_pgrp(pgrp, 0) && __has_stopped_jobs(pgrp)) { > + if (__will_become_orphaned_pgrp(pgrp, 0) && > + __has_stopped_jobs(pgrp)) { > __kill_pg_info(SIGHUP, (void *)1, pgrp); > __kill_pg_info(SIGCONT, (void *)1, pgrp); > } > @@ -520,7 +504,7 @@ static inline void zap_thread(task_t *p, > */ > static void exit_notify(void) > { > - struct task_struct *t; > + struct task_struct *t, *p; > > write_lock_irq(&tasklist_lock); > > @@ -580,10 +564,8 @@ static void exit_notify(void) > if (current->exit_signal != -1) > do_notify_parent(current, current->exit_signal); > > - while (!list_empty(¤t->children)) > - zap_thread(list_entry(current->children.next,struct task_struct,sibling), current, 0); > - while (!list_empty(¤t->ptrace_children)) > - zap_thread(list_entry(current->ptrace_children.next,struct task_struct,ptrace_list), current, 1); > + while ((p = eldest_child(current)) != NULL) > + zap_thread(p, current); > BUG_ON(!list_empty(¤t->children)); > > current->state = TASK_ZOMBIE; >
This part's nice, though.
-- 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/
|  |