Skip to contentSkip to conversation

Move utrace into task_struct

PATCH In review Alexey Dobriyan 4 participants 5 messages May 2, 2007 – May 8, 2007

Subsystems (4)
MM core Tracing Scheduler exec and binfmt
lore
Share & tools
lore

Alexey Dobriyan

author
#0 lore raw

PATCH 1/1 Move utrace into task_struct

This patch moves "struct utrace" into "struct task_struct" directly instead of being referenced by a pointer from task_struct. The main reason is utrace code leaving stale ->utrace pointer and freeing "struct utrace" itself. This manifests as crashes in __rcu_process_callbacks() and other nasties.

As side effect this makes attaching and detaching logic much simpler and streamlined. As you see ~200 lines are gone.

I can't oops 2-way Opteron box with this patch applied.

NOTE: patch is against (utrace patch against 2.6.21)
NOTE: "utrace_engine_cache" still leaks
NOTE: "XXX ptrace_report_death" leaks still happen
NOTE: most certainly some locking is still missed and I need to test
on a couple more boxes.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 include/linux/sched.h     |    7 
 include/linux/tracehook.h |   15 -
 include/linux/utrace.h    |   39 ++-
 kernel/fork.c             |    9 
 kernel/utrace.c           |  457 ++++++++++++----------------------------------
 5 files changed, 157 insertions(+), 370 deletions(-)
include/linux/sched.h +2 -5
@@ -83,6 +83,7 @@ #include <linux/resource.h>
 #include <linux/timer.h>
 #include <linux/hrtimer.h>
 #include <linux/task_io_accounting.h>
+#include <linux/utrace.h>
 
 #include <asm/processor.h>
 

@@ -940,11 +941,7 @@ #endif
 	void *security;
 	struct audit_context *audit_context;
 	seccomp_t seccomp;
-
-#ifdef CONFIG_UTRACE
-	struct utrace *utrace;
-	unsigned long utrace_flags;
-#endif
+	struct utrace utrace;
 
 /* Thread group tracking */
    	u32 parent_exec_id;
include/linux/tracehook.h +2 -13
@@ -310,16 +310,6 @@ utrace_regset_copyin_ignore(unsigned int
  ***
  ***/
 
-
-/*
- * Called in copy_process when setting up the copied task_struct,
- * with tasklist_lock held for writing.
- */
-static inline void tracehook_init_task(struct task_struct *child)
-{
-	utrace_init_task(child);
-}
-
 /*
  * Called from release_task, no locks held.
  * After this, there should be no tracing entanglements.

@@ -327,8 +317,7 @@ static inline void tracehook_init_task(s
 static inline void tracehook_release_task(struct task_struct *p)
 {
 	smp_mb();
-	if (tsk_utrace_struct(p) != NULL)
-		utrace_release_task(p);
+	utrace_release_task(p);
 }
 
 /*

@@ -339,7 +328,7 @@ static inline void tracehook_release_tas
  */
 static inline int tracehook_check_released(struct task_struct *p)
 {
-	return unlikely(tsk_utrace_struct(p) != NULL);
+	return 0;
 }
 
 /*
include/linux/utrace.h +22 -17
@@ -50,11 +50,30 @@ #include <linux/sched.h>
 
 struct linux_binprm;
 struct pt_regs;
-struct utrace;
+struct task_struct;
 struct utrace_signal;
 struct utrace_regset;
 struct utrace_regset_view;
 
+#ifdef CONFIG_UTRACE
+struct utrace {
+	unsigned long flags;
+	union {
+		struct {
+			struct task_struct *cloning;
+			struct utrace_signal *signal;
+		} live;
+		struct {
+			unsigned long flags;
+		} exit;
+	} u;
+	struct list_head engines;
+	spinlock_t lock;
+};
+#else
+struct utrace {
+};
+#endif
 
 /*
  * Flags in task_struct.utrace_flags and utrace_attached_engine.flags.

@@ -493,19 +512,8 @@ void utrace_signal_handler_singlestep(st
 /*
  * <linux/tracehook.h> uses these accessors to avoid #ifdef CONFIG_UTRACE.
  */
-static inline unsigned long tsk_utrace_flags(struct task_struct *tsk)
-{
-	return tsk->utrace_flags;
-}
-static inline struct utrace *tsk_utrace_struct(struct task_struct *tsk)
-{
-	return tsk->utrace;
-}
-static inline void utrace_init_task(struct task_struct *child)
-{
-	child->utrace_flags = 0;
-	child->utrace = NULL;
-}
+#define tsk_utrace_flags(tsk)	((tsk)->utrace.flags)
+#define tsk_utrace_struct(tsk)	(&(tsk)->utrace)
 
 #else  /* !CONFIG_UTRACE */
 

@@ -517,9 +525,6 @@ static struct utrace *tsk_utrace_struct(
 {
 	return NULL;
 }
-static inline void utrace_init_task(struct task_struct *child)
-{
-}
 
 /*
  * The calls to these should all be in if (0) and optimized out entirely.
kernel/fork.c +7 -2
@@ -1241,8 +1241,13 @@ #endif
 
 	if (likely(p->pid)) {
 		add_parent(p);
-		tracehook_init_task(p);
-
+#ifdef CONFIG_UTRACE
+		p->utrace.flags = 0;
+		p->utrace.u.live.cloning = NULL;
+		p->utrace.u.live.signal = NULL;
+		INIT_LIST_HEAD(&p->utrace.engines);
+		spin_lock_init(&p->utrace.lock);
+#endif
 		if (thread_group_leader(p)) {
 			p->signal->tty = current->signal->tty;
 			p->signal->pgrp = process_group(current);
kernel/utrace.c +124 -333

766 lines not on this page · raw message

#1 lore raw
Quoted 14 lines · Alexey Dobriyan
include/linux/utrace.h +3 -1
@@ -50,11 +50,30 @@ #include <linux/sched.h>
 
 struct linux_binprm;
 struct pt_regs;
-struct utrace;
+struct task_struct;
 struct utrace_signal;
 struct utrace_regset;
 struct utrace_regset_view;
 
+#ifdef CONFIG_UTRACE
+struct utrace {

include/linux/utrace.h #include <linux/sched.h> · line 69

Quoted 10 lines
include/linux/utrace.h +10 -0
+	unsigned long flags;
+	union {
+		struct {
+			struct task_struct *cloning;
+			struct utrace_signal *signal;
+		} live;
+		struct {
+			unsigned long flags;
+		} exit;
+	} u;

You can have anonymous unions. (Though that won't work with static initializers - are there any?)

Jan

#3 lore raw
Quoted 2 lines · Roland McGrath

That's just a workaround for an actual bug that I need to fix. You wouldn't really want to do it that way.

It's actuall a very nice and needed simplification. It gets rid of an object with subtle life time rules, which is always a good thing. The utrace code really needs more patches in the style of Alexey's as this kind of overengineering is what will cause hard to debug problems over the long run.

author
#4 lore raw

Regardless of future of "struct utrace utrace;" patch looks like there is another race: engine's flags and ops settings in utrace_detach() and acting on them in report_quiescent():

utrace_detach() report_quiescent()
--------------- ------------------
[utrace lock held] [utrace lock is not held]

engine->flags = UTRACE_EVENT(QUIESCE) |

		UTRACE_ACTION_QUIESCE;
					if (engine->flags & UTRACE_EVENT(QUIESCE))
						REPORT(report_quiesce);

rcu_assign_pointer(engine->ops, &dead_engine_ops);

At the moment of REPORT call engine's ops are still "live" ptrace ops which do not have ->report_quiesce callback. So, there will oops while calling function at NULL address. "Dead" ptrace engine ops do have dummy callback but it wasn't yet glued.

I hit this once with "struct utrace utrace;" patch applied, but this bug is also present in stock utrace, I'm sure.