Re: copy_process utrace_init_task (Was: [PATCH 133] stepping, accommodate to utrace-cleanup changes)

[Roland McGrath]

 Found the trivial but nasty problem.

Ah!  Good catch.

I added tracehook_init_task() in my tree.  I don't see much benefit in
sending any tracehook patch upstream for this.  tracehook_init_task()
corresponds to tracehook_free_task(), which is only added by utrace
(and both would just be empty in a separate preparatory patch).

I don't see any reason to fiddle the ptrace_init_task() call.  The
setup it does is all stuff that only matters for teardown done by
release_task() or even earlier in the exit sequence.  So it makes
enough sense that the setup side should happen later as it does now.
In the long run, the ptrace init stuff will all just go away.  Before
then I can't see any rationale for touching it.


Thanks,
Roland

Re: copy_process utrace_init_task (Was: [PATCH 133] stepping, accommodate to utrace-cleanup changes)

[Oleg Nesterov]

On 11/18, Roland McGrath wrote:

 I added tracehook_init_task() in my tree.  I don't see much benefit in
 sending any tracehook patch upstream for this.  tracehook_init_task()
 corresponds to tracehook_free_task(), which is only added by utrace
 (and both would just be empty in a separate preparatory patch).

 I don't see any reason to fiddle the ptrace_init_task() call.
 ...
 In the long run, the ptrace init stuff will all just go away.

OK, agreed.

Oleg.

copy_process utrace_init_task (Was: [PATCH 133] stepping, accommodate to utrace-cleanup changes)

[Oleg Nesterov]

On 11/16, Oleg Nesterov wrote:

 On 11/16, Oleg Nesterov wrote:
 
  And I didn't check make xcheck, I guess it still crashes the kernel.

 Yes it does. I am almost sure the bug should be trivial, but
 somehow can't find find it.

Found the trivial but nasty problem.

tracehook_finish_clone()-utrace_init_task() sets -utrace = NULL, but
this is too late. If copy_process() fails before, tracehook_free_task()
will free parent's -utrace copied by dup_task_struct().

This is nasty because we need some changes outside of utrace/tracehook
files. Perhaps we should send something like the patch below to Andrew
right now?


And! While this bug could perfectly explain the crash, it doesn't.
I appiled this patch

--- UTRACE-PTRACE/kernel/fork.c~XXX_CRASH   2009-11-16 
20:26:23.0 +0100
+++ UTRACE-PTRACE/kernel/fork.c 2009-11-17 20:33:50.0 +0100
@@ -1019,6 +1019,7 @@ static struct task_struct *copy_process(
if (!p)
goto fork_out;
 
+p-utrace = NULL;
ftrace_graph_init_task(p);
 
rt_mutex_init_task(p);

but make xcheck still crashes. Still investigating.

Oleg.

--- TH/include/linux/ptrace.h~TH_INIT_TASK  2009-11-10 21:31:25.0 
+0100
+++ TH/include/linux/ptrace.h   2009-11-17 20:43:42.0 +0100
@@ -148,6 +148,14 @@ static inline int ptrace_event(int mask,
return 1;
 }
 
+static inline void ptrace_init_task(struct task_struct *child)
+{
+   INIT_LIST_HEAD(child-ptrace_entry);
+   INIT_LIST_HEAD(child-ptraced);
+   child-parent = child-real_parent;
+   child-ptrace = 0;
+}
+
 /**
  * ptrace_init_task - initialize ptrace state for a new child
  * @child: new child task
@@ -158,12 +166,8 @@ static inline int ptrace_event(int mask,
  *
  * Called with current's siglock and write_lock_irq(tasklist_lock) held.
  */
-static inline void ptrace_init_task(struct task_struct *child, bool ptrace)
+static inline void ptrace_finish_clone(struct task_struct *child, bool ptrace)
 {
-   INIT_LIST_HEAD(child-ptrace_entry);
-   INIT_LIST_HEAD(child-ptraced);
-   child-parent = child-real_parent;
-   child-ptrace = 0;
if (unlikely(ptrace)  (current-ptrace  PT_PTRACED)) {
child-ptrace = current-ptrace;
__ptrace_link(child, current-parent);
--- TH/include/linux/tracehook.h~TH_INIT_TASK   2009-11-11 21:49:42.0 
+0100
+++ TH/include/linux/tracehook.h2009-11-17 20:42:43.0 +0100
@@ -247,6 +247,11 @@ static inline int tracehook_prepare_clon
return 0;
 }
 
+static inline void tracehook_init_task(struct task_struct *child)
+{
+   ptrace_init_task(child);
+}
+
 /**
  * tracehook_finish_clone - new child created and being attached
  * @child: new child task
@@ -261,7 +266,7 @@ static inline int tracehook_prepare_clon
 static inline void tracehook_finish_clone(struct task_struct *child,
  unsigned long clone_flags, int trace)
 {
-   ptrace_init_task(child, (clone_flags  CLONE_PTRACE) || trace);
+   ptrace_finish_clone(child, (clone_flags  CLONE_PTRACE) || trace);
 }
 
 /**
--- TH/kernel/fork.c~TH_INIT_TASK   2009-11-07 22:15:15.0 +0100
+++ TH/kernel/fork.c2009-11-17 20:40:52.0 +0100
@@ -1018,8 +1018,8 @@ static struct task_struct *copy_process(
if (!p)
goto fork_out;
 
+   tracehook_init_task(p);
ftrace_graph_init_task(p);
-
rt_mutex_init_task(p);
 
 #ifdef CONFIG_PROVE_LOCKING