lkml.org 
[lkml]   [2002]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [patch] ptrace-fix-2.5.33-A1
From
Date
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.

in forget_original_parent()

/*
* There are only two places where our children can be:
*
* - in our child list
* - in the global ptrace list
*
* 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 change the real parent.


in exit_notify()

list_for_each_safe(_p, _n, &current->ptrace_children)
zap_thread(list_entry(_p,struct task_struct,ptrace_list), current);

Looks like that real parent deprive a process from tracer.


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.


break;
tsk = next_thread(tsk);
} while (tsk != current);
read_unlock(&tasklist_lock);
if (flag || !list_empty(&current->ptrace_children)) {

Until now, this case wasn't blocked. However, I like this behavior.


If these are right, can you look the following patch? I think this patch
fixes problems.

===== kernel/exit.c 1.43 vs edited =====
--- 1.43/kernel/exit.c Mon Sep 2 00:54:47 2002
+++ edited/kernel/exit.c Fri Sep 6 01:58:52 2002
@@ -403,10 +403,10 @@
* group, and if no such member exists, give it to
* the global child reaper process (ie "init")
*/
-static inline void forget_original_parent(struct task_struct * father)
+static inline void forget_real_parent(struct task_struct * father)
{
struct task_struct *p, *reaper;
- list_t *_p;
+ list_t *_p, *_n;

read_lock(&tasklist_lock);

@@ -425,17 +425,24 @@
* There are only two places where our children can be:
*
* - in our child list
- * - in the global ptrace list
+ * - in the ptraced child list
*
* 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);
+ if (p->real_parent == father)
+ 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);
+
+ /* 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);
}
@@ -443,9 +450,8 @@
static inline void zap_thread(task_t *p, task_t *father)
{
ptrace_unlink(p);
- list_del_init(&p->sibling);
- 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)
@@ -478,7 +484,7 @@
struct task_struct *t;
list_t *_p, *_n;

- forget_original_parent(current);
+ forget_real_parent(current);
/*
* Check to see if any process groups have become orphaned
* as a result of our exiting, and if they have any stopped
@@ -539,15 +545,13 @@
zap_again:
list_for_each_safe(_p, _n, &current->children)
zap_thread(list_entry(_p,struct task_struct,sibling), current);
- list_for_each_safe(_p, _n, &current->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:
*/
- if (unlikely(!list_empty(&current->children) ||
- !list_empty(&current->ptrace_children)))
+ if (unlikely(!list_empty(&current->children)))
goto zap_again;
/*
* No need to unlock IRQs, we'll schedule() immediately
@@ -598,8 +602,10 @@
tsk->exit_code = code;
exit_notify();
preempt_disable();
- if (current->exit_signal == -1)
+ if (current->exit_signal == -1) {
+ ptrace_unlink(current);
release_task(current);
+ }
schedule();
BUG();
/*
===== kernel/ptrace.c 1.16 vs edited =====
--- 1.16/kernel/ptrace.c Tue Aug 20 03:12:27 2002
+++ edited/kernel/ptrace.c Wed Sep 4 03:00:53 2002
@@ -26,11 +26,12 @@
*/
void __ptrace_link(task_t *child, task_t *new_parent)
{
- if (!list_empty(&child->ptrace_list))
- BUG();
+ BUG_ON(!list_empty(&child->ptrace_list));
+ BUG_ON(child->parent != child->real_parent);
+
if (child->parent == new_parent)
- BUG();
- list_add(&child->ptrace_list, &child->parent->ptrace_children);
+ return;
+ list_add(&child->ptrace_list, &child->real_parent->ptrace_children);
REMOVE_LINKS(child);
child->parent = new_parent;
SET_LINKS(child);
@@ -44,10 +45,10 @@
*/
void __ptrace_unlink(task_t *child)
{
- if (!child->ptrace)
- BUG();
+ BUG_ON(!child->ptrace);
+
child->ptrace = 0;
- if (list_empty(&child->ptrace_list))
+ if (child->parent == child->real_parent)
return;
list_del_init(&child->ptrace_list);
REMOVE_LINKS(child);
--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
-
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/
\
 
 \ /
  Last update: 2005-03-22 13:28    [W:0.065 / U:0.188 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site