Hi,
This patch fixes the following,
- race condition of ptrace flag
- sent odd signal to the tracer
- broken before behavior
And some cleanup.
Please apply.
--
OGAWA Hirofumi <[email protected]>
--- 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 17:28:01.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? */
@@ -464,50 +463,35 @@ static inline void forget_original_paren
}
list_for_each(_p, &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);
}
}
-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);
+
/*
* 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 patch fixes the following,
>
> - race condition of ptrace flag
> - sent odd signal to the tracer
> - broken before behavior
(looks good to me). I'm wondering about the following:
- 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));
is it guaranteed that at this point current->ptrace_children is empty?
Ingo
On Mon, 16 Sep 2002, OGAWA Hirofumi wrote:
> list_for_each(_p, &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);
> }
>
> current->ptrace_children should be empty after this reparent.
oh, okay. It's also cleaner this way.
Ingo
Ingo Molnar <[email protected]> writes:
> > This patch fixes the following,
> >
> > - race condition of ptrace flag
> > - sent odd signal to the tracer
> > - broken before behavior
>
> (looks good to me). I'm wondering about the following:
>
> - 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));
>
> is it guaranteed that at this point current->ptrace_children is empty?
Yes, I think so.
list_for_each(_p, &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);
}
current->ptrace_children should be empty after this reparent.
--
OGAWA Hirofumi <[email protected]>
Ingo Molnar <[email protected]> writes:
> On Mon, 16 Sep 2002, OGAWA Hirofumi wrote:
>
> > list_for_each(_p, &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);
> > }
> >
> > current->ptrace_children should be empty after this reparent.
>
> oh, okay. It's also cleaner this way.
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) {
I'll resend patch.
--
OGAWA Hirofumi <[email protected]>
OGAWA Hirofumi <[email protected]> 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.
--
OGAWA Hirofumi <[email protected]>
--- 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? */
@@ -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);
}
}
-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);
+
/*
* 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;
On Mon, Sep 16, 2002 at 09:44:34PM +0900, OGAWA Hirofumi wrote:
> OGAWA Hirofumi <[email protected]> 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 <[email protected]>
>
> --- 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
Daniel Jacobowitz <[email protected]> writes:
> On Mon, Sep 16, 2002 at 09:44:34PM +0900, OGAWA Hirofumi wrote:
> > OGAWA Hirofumi <[email protected]> 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.
It's the following
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);
> 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.
You aren't looking the source.
void reparent_to_init(void)
{
write_lock_irq(&tasklist_lock);
ptrace_unlink(current);
/* Reparent to init */
REMOVE_LINKS(current);
current->parent = child_reaper;
current->real_parent = child_reaper;
SET_LINKS(current);
/* Set the exit signal to SIGCHLD so we signal init on exit */
current->exit_signal = SIGCHLD;
current->ptrace = 0;
> > @@ -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.
??? You forget tasklist_lock?
> 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.
What is the basis which you think it is bug?
--
OGAWA Hirofumi <[email protected]>
On Mon, Sep 16, 2002 at 11:28:57PM +0900, OGAWA Hirofumi wrote:
> Daniel Jacobowitz <[email protected]> writes:
>
> > On Mon, Sep 16, 2002 at 09:44:34PM +0900, OGAWA Hirofumi wrote:
> > > OGAWA Hirofumi <[email protected]> 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.
>
> It's the following
>
> 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);
We have the tasklist lock. How can there be a race here? The parent
can't detach while we're holding the tasklist lock. If there is a race
with PTRACE_SETOPTIONS, then PTRACE_SETOPTIONS should take the lock.
> > 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.
>
> You aren't looking the source.
>
> void reparent_to_init(void)
> {
> write_lock_irq(&tasklist_lock);
>
> ptrace_unlink(current);
You're right on this one, I missed ptrace_unlink. Thanks.
> > > @@ -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.
>
> ??? You forget tasklist_lock?
Huh?
The problem I am describing is when a child - which will become an
orphaned pgrp when ``father'' dies - is being ptraced at the moment of
``father''s death. With your patch it will be moved to
reaper->ptrace_children (or child_reaper->ptrace_children) but never
orphaned properly. It'll miss a 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.
>
> What is the basis which you think it is bug?
The death of a tracing process should not have any effect on the traced
process except to untrace it. It should not go through the orphaning
checks. The orphaning checks assume that the exiting process is the
real parent, and will orphan the pgrp if it is not in the same
session... as its tracer! That's a bug.
--
Daniel Jacobowitz
MontaVista Software Debian GNU/Linux Developer
Daniel Jacobowitz <[email protected]> writes:
> > > 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.
> >
> > It's the following
> >
> > 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);
>
> We have the tasklist lock. How can there be a race here? The parent
> can't detach while we're holding the tasklist lock. If there is a race
> with PTRACE_SETOPTIONS, then PTRACE_SETOPTIONS should take the lock.
No. If the real parent don't change ->ptrace, it doesn't need
lock.
> > > 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.
> >
> > ??? You forget tasklist_lock?
>
> Huh?
>
> The problem I am describing is when a child - which will become an
> orphaned pgrp when ``father'' dies - is being ptraced at the moment of
> ``father''s death. With your patch it will be moved to
> reaper->ptrace_children (or child_reaper->ptrace_children) but never
> orphaned properly. It'll miss a 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.
> >
> > What is the basis which you think it is bug?
>
> The death of a tracing process should not have any effect on the traced
> process except to untrace it. It should not go through the orphaning
> checks. The orphaning checks assume that the exiting process is the
> real parent, and will orphan the pgrp if it is not in the same
> session... as its tracer! That's a bug.
Ah, ok. I think, it's longtime (odd) behavior. And you think, it's
a bug. Right?
And, both of your and old code has odd behavior. yes?
--
OGAWA Hirofumi <[email protected]>
On Tue, Sep 17, 2002 at 12:57:57AM +0900, OGAWA Hirofumi wrote:
> Daniel Jacobowitz <[email protected]> writes:
>
> > > > 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.
> > >
> > > It's the following
> > >
> > > 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);
> >
> > We have the tasklist lock. How can there be a race here? The parent
> > can't detach while we're holding the tasklist lock. If there is a race
> > with PTRACE_SETOPTIONS, then PTRACE_SETOPTIONS should take the lock.
>
> No. If the real parent don't change ->ptrace, it doesn't need
> lock.
I don't understand what you mean by that. Do you mean, "if it does
change ->ptrace, it doesn't need a lock"?
The locking requirements for tsk->ptrace are not documented anywhere
right now; we basically don't have any.
> > > > 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.
> > >
> > > ??? You forget tasklist_lock?
> >
> > Huh?
> >
> > The problem I am describing is when a child - which will become an
> > orphaned pgrp when ``father'' dies - is being ptraced at the moment of
> > ``father''s death. With your patch it will be moved to
> > reaper->ptrace_children (or child_reaper->ptrace_children) but never
> > orphaned properly. It'll miss a 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.
> > >
> > > What is the basis which you think it is bug?
> >
> > The death of a tracing process should not have any effect on the traced
> > process except to untrace it. It should not go through the orphaning
> > checks. The orphaning checks assume that the exiting process is the
> > real parent, and will orphan the pgrp if it is not in the same
> > session... as its tracer! That's a bug.
>
> Ah, ok. I think, it's longtime (odd) behavior. And you think, it's
> a bug. Right?
>
> And, both of your and old code has odd behavior. yes?
Before your patch, do_notify_parent didn't get called; I think that
perhaps it should be. I'll think about that. After your patch the
process group will be unexpectedly orphaned, which is not now the case.
Let me sit on this for a couple of hours. I'll send you an alternative
patch to look at.
--
Daniel Jacobowitz
MontaVista Software Debian GNU/Linux Developer
Daniel Jacobowitz <[email protected]> writes:
> > > We have the tasklist lock. How can there be a race here? The parent
> > > can't detach while we're holding the tasklist lock. If there is a race
> > > with PTRACE_SETOPTIONS, then PTRACE_SETOPTIONS should take the lock.
> >
> > No. If the real parent don't change ->ptrace, it doesn't need
> > lock.
>
> I don't understand what you mean by that. Do you mean, "if it does
> change ->ptrace, it doesn't need a lock"?
Basically, only tracer can change ->ptrace of traced child. And, it
doesn't need lock. (there are some exceptions)
> > Ah, ok. I think, it's longtime (odd) behavior. And you think, it's
> > a bug. Right?
> >
> > And, both of your and old code has odd behavior. yes?
>
> Before your patch, do_notify_parent didn't get called; I think that
> perhaps it should be. I'll think about that. After your patch the
> process group will be unexpectedly orphaned, which is not now the case.
>
> Let me sit on this for a couple of hours. I'll send you an alternative
> patch to look at.
Ok. But I'll sleep soon. So, I'll look it, after having come back from
the office.
--
OGAWA Hirofumi <[email protected]>