Home » Mailing lists » Devel » [PATCH -utrace] Move utrace into task_struct
| [PATCH -utrace] Move utrace into task_struct [message #12540] |
Wed, 02 May 2007 11:24  |
Alexey Dobriyan
Messages: 195
Registered: August 2006
|
Senior Member |
|
|
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 <adobriyan@sw.ru>
---
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(-)
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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;
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -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;
}
/*
--- a/include/linux/utrace.h
+++ b/include/linux/utrace.h
@@ -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.
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -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);
--- a/kernel/utrace.c
+++ b/kernel/utrace.c
@@ -10,13 +10,13 @@
* Red Hat Author: Roland McGrath.
*/
-#include <linux/utrace.h>
#include <linux/tracehook.h>
#include <linux/err.h>
#include <linux/sched.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/slab.h>
+#include <linux/utrace.h>
#include <asm/tracehook.h>
@@ -29,48 +29,11 @@ #define CHECK_INIT(p) do { } while (0)
#define CHECK_DEAD(p) do { } while (0)
#endif
-/*
- * Per-thread structure task_struct.utrace points to.
- *
- * The task itself never has to worry about this going away after
- * some event is found set in task_struct.utrace_flags.
- * Once created, this pointer is changed only when the task is quiescent
- * (TASK_TRACED or TASK_STOPPED with the siglock held, or dead).
- *
- * For other parties, the pointer to this is protected by RCU and
- * task_lock. Since call_rcu is never used while the thread is alive and
- * using this struct utrace, we can overlay the RCU data structure used
- * only for a dead struct with some local state used only for a live utrace
- * on an active thread.
- */
-struct utrace
-{
- union {
- struct rcu_head dead;
- struct {
- struct task_struct *cloning;
- struct utrace_signal *signal;
- } live;
- struct {
- unsigned long flags;
- } exit;
- } u;
-
- struct list_head engines;
- spinlock_t lock;
- atomic_t check_dead;
-};
-
-static struct kmem_cache *utrace_cachep;
static struct kmem_cache *utrace_engine_cachep;
static int __init
utrace_init(void)
{
- utrace_cachep =
- kmem_cache_create("utrace_cache",
- sizeof(struct utrace), 0,
- SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL, NULL);
utrace_engine_cachep =
kmem_cache_create("utrace_engine_cache",
sizeof(struct utrace_attached_engine), 0,
@@ -79,106 +42,6 @@ utrace_init(void)
}
subsys_initcall(utrace_init);
-
-/*
- * Make sure target->utrace is allocated, and return with it locked on
- * success. This function mediates startup races. The creating parent
- * task has priority, and other callers will delay here to let its call
- * succeed and take the new utrace lock first.
- */
-static struct utrace *
-utrace_first_engine(struct task_struct *target,
- struct utrace_attached_engine *engine)
- __acquires(utrace->lock)
-{
- struct utrace *utrace, *ret;
-
- /*
- * If this is a newborn thread and we are not the creator,
- * we have to wait for it. The creator gets the first chance
- * to attach. The PF_STARTING flag is cleared after its
- * report_clone hook has had a chance to run.
- */
- if ((target->flags & PF_STARTING)
- && (current->utrace == NULL
- || current->utrace->u.live.cloning != target)) {
- yield();
- return (signal_pending(current)
- ? ERR_PTR(-ERESTARTNOINTR) : NULL);
- }
-
- utrace = kmem_cache_alloc(utrace_cachep, GFP_KERNEL);
- if (unlikely(utrace == NULL))
- return ERR_PTR(-ENOMEM);
-
- utrace->u.live.cloning = NULL;
- utrace->u.live.signal = NULL;
- INIT_LIST_HEAD(&utrace->engines);
- list_add(&engine->entry, &utrace->engines);
- spin_lock_init(&utrace->lock);
- CHECK_INIT(utrace);
-
- ret = utrace;
- spin_lock(&utrace->lock);
- task_lock(target);
- if (likely(target->utrace == NULL)) {
- rcu_assign_pointer(target->utrace, utrace);
- /*
- * The task_lock protects us against another thread doing
- * the same thing. We might still be racing against
- * tracehook_release_task. It's called with ->exit_state
- * set to EXIT_DEAD and then checks ->utrace with an
- * smp_mb() in between. If EXIT_DEAD is set, then
- * release_task might have checked ->utrace already and saw
- * it NULL; we can't attach. If we see EXIT_DEAD not yet
- * set after our barrier, then we know release_task will
- * see our target->utrace pointer.
- */
- smp_mb();
- if (target->exit_state == EXIT_DEAD) {
- /*
- * The target has already been through release_task.
- */
- target->utrace = NULL;
- goto cannot_attach;
- }
- task_unlock(target);
- }
- else {
- /*
- * Another engine attached first, so there is a struct already.
- * A null return says to restart looking for the existing one.
- */
- cannot_attach:
- ret = NULL;
- task_unlock(target);
- spin_unlock(&utrace->lock);
- kmem_cache_free(utrace_cachep, utrace);
- }
-
- return ret;
-}
-
-static void
-utrace_free(struct rcu_head *rhead)
-{
- struct utrace *utrace = container_of(rhead, struct utrace, u.dead);
- kmem_cache_free(utrace_cachep, utrace);
-}
-
-/*
- * Called with utrace locked. Clean it up and free it via RCU.
- */
-static void
-rcu_utrace_free(struct utrace *utrace)
- __releases(utrace->lock)
-{
- CHECK_DEAD(utrace);
- spin_unlock(&utrace->lock);
- INIT_RCU_HEAD(&utrace->u.dead);
- call_rcu(&utrace->u.dead, utrace_free);
-}
-
static void
utrace_engine_free(struct rcu_head *rhead)
{
@@ -200,16 +63,10 @@ rcu_engine_free(struct utrace_attached_e
* forced signal (or it's quiescent in utrace_get_signal).
*/
static inline void
-utrace_clear_tsk(struct task_struct *tsk, struct utrace *utrace)
+utrace_clear_tsk(struct utrace *utrace)
{
- if (utrace->u.live.signal == NULL) {
- task_lock(tsk);
- if (likely(tsk->utrace != NULL)) {
- rcu_assign
...
|
|
|
|
|
|
|
|
|
|
| Re: [PATCH -utrace] Move utrace into task_struct [message #12744 is a reply to message #12732] |
Tue, 08 May 2007 14:34  |
Alexey Dobriyan
Messages: 195
Registered: August 2006
|
Senior Member |
|
|
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.
|
|
|
|
Goto Forum:
Current Time: Sat Jul 26 13:10:04 GMT 2025
Total time taken to generate the page: 0.46495 seconds
|
OpenVZ Forum
Members
Search
Help
Register
Login
Home
