OpenVZ Forum


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 Go to next message
Alexey Dobriyan is currently offline  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 #12549 is a reply to message #12540] Wed, 02 May 2007 11:53 Go to previous messageGo to next message
Jan Engelhardt is currently offline  Jan Engelhardt
Messages: 18
Registered: August 2006
Junior Member
On May 2 2007 15:32, Alexey Dobriyan wrote:
>--- 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;

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


Jan
--
Re: [PATCH -utrace] Move utrace into task_struct [message #12565 is a reply to message #12540] Wed, 02 May 2007 22:02 Go to previous messageGo to next message
Roland McGrath is currently offline  Roland McGrath
Messages: 7
Registered: February 2007
Junior Member
That's just a workaround for an actual bug that I need to fix.
You wouldn't really want to do it that way.


Thanks,
Roland
Re: [PATCH -utrace] Move utrace into task_struct [message #12732 is a reply to message #12565] Thu, 03 May 2007 10:34 Go to previous messageGo to next message
Christoph Hellwig is currently offline  Christoph Hellwig
Messages: 59
Registered: April 2006
Member
On Wed, May 02, 2007 at 03:02:03PM -0700, Roland McGrath wrote:
> 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.
Re: [PATCH -utrace] Move utrace into task_struct [message #12744 is a reply to message #12732] Tue, 08 May 2007 14:34 Go to previous message
Alexey Dobriyan is currently offline  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.
Previous Topic: [patch 049/455] Merge sys_clone()/sys_unshare() nsproxy and namespace handling
Next Topic: [PATCH 0/9] Containers (V9): Generic Process Containers
Goto Forum:
  


Current Time: Sat Jul 26 13:10:04 GMT 2025

Total time taken to generate the page: 0.46495 seconds