perf: Fix perf_event_exit_task() race
authorPeter Zijlstra <peterz@infradead.org>
Thu, 14 Jan 2016 15:05:37 +0000 (16:05 +0100)
committerIngo Molnar <mingo@kernel.org>
Thu, 21 Jan 2016 17:54:25 +0000 (18:54 +0100)
There is a race against perf_event_exit_task() vs
event_function_call(),find_get_context(),perf_install_in_context()
(iow, everyone).

Since there is no permanent marker on a context that its dead, it is
quite possible that we access (and even modify) a context after its
passed through perf_event_exit_task().

For instance, find_get_context() might find the context still
installed, but by the time we get to perf_install_in_context() it
might already have passed through perf_event_exit_task() and be
considered dead, we will however still add the event to it.

Solve this by marking a ctx dead by setting its ctx->task value to -1,
it must be !0 so we still know its a (former) task context.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
kernel/events/core.c

index 3eaf91b..9de4d35 100644 (file)
@@ -148,6 +148,13 @@ static void perf_ctx_unlock(struct perf_cpu_context *cpuctx,
        raw_spin_unlock(&cpuctx->ctx.lock);
 }
 
+#define TASK_TOMBSTONE ((void *)-1L)
+
+static bool is_kernel_event(struct perf_event *event)
+{
+       return event->owner == TASK_TOMBSTONE;
+}
+
 /*
  * On task ctx scheduling...
  *
@@ -196,31 +203,21 @@ static int event_function(void *info)
        struct perf_event_context *ctx = event->ctx;
        struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
        struct perf_event_context *task_ctx = cpuctx->task_ctx;
+       int ret = 0;
 
        WARN_ON_ONCE(!irqs_disabled());
 
+       perf_ctx_lock(cpuctx, task_ctx);
        /*
         * Since we do the IPI call without holding ctx->lock things can have
         * changed, double check we hit the task we set out to hit.
-        *
-        * If ctx->task == current, we know things must remain valid because
-        * we have IRQs disabled so we cannot schedule.
         */
        if (ctx->task) {
-               if (ctx->task != current)
-                       return -EAGAIN;
-
-               WARN_ON_ONCE(task_ctx != ctx);
-       } else {
-               WARN_ON_ONCE(&cpuctx->ctx != ctx);
-       }
+               if (ctx->task != current) {
+                       ret = -EAGAIN;
+                       goto unlock;
+               }
 
-       perf_ctx_lock(cpuctx, task_ctx);
-       /*
-        * Now that we hold locks, double check state. Paranoia pays.
-        */
-       if (task_ctx) {
-               WARN_ON_ONCE(task_ctx->task != current);
                /*
                 * We only use event_function_call() on established contexts,
                 * and event_function() is only ever called when active (or
@@ -233,12 +230,16 @@ static int event_function(void *info)
                 * And since we have ctx->is_active, cpuctx->task_ctx must
                 * match.
                 */
-               WARN_ON_ONCE(cpuctx->task_ctx != task_ctx);
+               WARN_ON_ONCE(task_ctx != ctx);
+       } else {
+               WARN_ON_ONCE(&cpuctx->ctx != ctx);
        }
+
        efs->func(event, cpuctx, ctx, efs->data);
+unlock:
        perf_ctx_unlock(cpuctx, task_ctx);
 
-       return 0;
+       return ret;
 }
 
 static void event_function_local(struct perf_event *event, event_f func, void *data)
@@ -256,7 +257,7 @@ static void event_function_local(struct perf_event *event, event_f func, void *d
 static void event_function_call(struct perf_event *event, event_f func, void *data)
 {
        struct perf_event_context *ctx = event->ctx;
-       struct task_struct *task = ctx->task;
+       struct task_struct *task = READ_ONCE(ctx->task); /* verified in event_function */
        struct event_function_struct efs = {
                .event = event,
                .func = func,
@@ -278,30 +279,28 @@ static void event_function_call(struct perf_event *event, event_f func, void *da
        }
 
 again:
+       if (task == TASK_TOMBSTONE)
+               return;
+
        if (!task_function_call(task, event_function, &efs))
                return;
 
        raw_spin_lock_irq(&ctx->lock);
-       if (ctx->is_active) {
-               /*
-                * Reload the task pointer, it might have been changed by
-                * a concurrent perf_event_context_sched_out().
-                */
-               task = ctx->task;
-               raw_spin_unlock_irq(&ctx->lock);
-               goto again;
+       /*
+        * Reload the task pointer, it might have been changed by
+        * a concurrent perf_event_context_sched_out().
+        */
+       task = ctx->task;
+       if (task != TASK_TOMBSTONE) {
+               if (ctx->is_active) {
+                       raw_spin_unlock_irq(&ctx->lock);
+                       goto again;
+               }
+               func(event, NULL, ctx, data);
        }
-       func(event, NULL, ctx, data);
        raw_spin_unlock_irq(&ctx->lock);
 }
 
-#define EVENT_OWNER_KERNEL ((void *) -1)
-
-static bool is_kernel_event(struct perf_event *event)
-{
-       return event->owner == EVENT_OWNER_KERNEL;
-}
-
 #define PERF_FLAG_ALL (PERF_FLAG_FD_NO_GROUP |\
                       PERF_FLAG_FD_OUTPUT  |\
                       PERF_FLAG_PID_CGROUP |\
@@ -1025,7 +1024,7 @@ static void put_ctx(struct perf_event_context *ctx)
        if (atomic_dec_and_test(&ctx->refcount)) {
                if (ctx->parent_ctx)
                        put_ctx(ctx->parent_ctx);
-               if (ctx->task)
+               if (ctx->task && ctx->task != TASK_TOMBSTONE)
                        put_task_struct(ctx->task);
                call_rcu(&ctx->rcu_head, free_ctx);
        }
@@ -1186,6 +1185,7 @@ static u64 primary_event_id(struct perf_event *event)
 
 /*
  * Get the perf_event_context for a task and lock it.
+ *
  * This has to cope with with the fact that until it is locked,
  * the context could get moved to another task.
  */
@@ -1226,10 +1226,13 @@ retry:
                        goto retry;
                }
 
-               if (!atomic_inc_not_zero(&ctx->refcount)) {
+               if (ctx->task == TASK_TOMBSTONE ||
+                   !atomic_inc_not_zero(&ctx->refcount)) {
                        raw_spin_unlock(&ctx->lock);
                        ctx = NULL;
                }
+
+               WARN_ON_ONCE(ctx->task != task);
        }
        rcu_read_unlock();
        if (!ctx)
@@ -2140,23 +2143,27 @@ static int  __perf_install_in_context(void *info)
        struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
        struct perf_event_context *task_ctx = cpuctx->task_ctx;
 
+       raw_spin_lock(&cpuctx->ctx.lock);
        if (ctx->task) {
+               raw_spin_lock(&ctx->lock);
                /*
                 * If we hit the 'wrong' task, we've since scheduled and
                 * everything should be sorted, nothing to do!
                 */
+               task_ctx = ctx;
                if (ctx->task != current)
-                       return 0;
+                       goto unlock;
 
                /*
                 * If task_ctx is set, it had better be to us.
                 */
                WARN_ON_ONCE(cpuctx->task_ctx != ctx && cpuctx->task_ctx);
-               task_ctx = ctx;
+       } else if (task_ctx) {
+               raw_spin_lock(&task_ctx->lock);
        }
 
-       perf_ctx_lock(cpuctx, task_ctx);
        ctx_resched(cpuctx, task_ctx);
+unlock:
        perf_ctx_unlock(cpuctx, task_ctx);
 
        return 0;
@@ -2188,6 +2195,17 @@ perf_install_in_context(struct perf_event_context *ctx,
         * happened and that will have taken care of business.
         */
        raw_spin_lock_irq(&ctx->lock);
+       task = ctx->task;
+       /*
+        * Worse, we cannot even rely on the ctx actually existing anymore. If
+        * between find_get_context() and perf_install_in_context() the task
+        * went through perf_event_exit_task() its dead and we should not be
+        * adding new events.
+        */
+       if (task == TASK_TOMBSTONE) {
+               raw_spin_unlock_irq(&ctx->lock);
+               return;
+       }
        update_context_time(ctx);
        /*
         * Update cgrp time only if current cgrp matches event->cgrp.
@@ -2195,7 +2213,6 @@ perf_install_in_context(struct perf_event_context *ctx,
         */
        update_cgrp_time_from_event(event);
        add_event_to_ctx(event, ctx);
-       task = ctx->task;
        raw_spin_unlock_irq(&ctx->lock);
 
        if (task)
@@ -2538,17 +2555,21 @@ static void perf_event_context_sched_out(struct task_struct *task, int ctxn,
                raw_spin_lock(&ctx->lock);
                raw_spin_lock_nested(&next_ctx->lock, SINGLE_DEPTH_NESTING);
                if (context_equiv(ctx, next_ctx)) {
-                       /*
-                        * XXX do we need a memory barrier of sorts
-                        * wrt to rcu_dereference() of perf_event_ctxp
-                        */
-                       task->perf_event_ctxp[ctxn] = next_ctx;
-                       next->perf_event_ctxp[ctxn] = ctx;
-                       ctx->task = next;
-                       next_ctx->task = task;
+                       WRITE_ONCE(ctx->task, next);
+                       WRITE_ONCE(next_ctx->task, task);
 
                        swap(ctx->task_ctx_data, next_ctx->task_ctx_data);
 
+                       /*
+                        * RCU_INIT_POINTER here is safe because we've not
+                        * modified the ctx and the above modification of
+                        * ctx->task and ctx->task_ctx_data are immaterial
+                        * since those values are always verified under
+                        * ctx->lock which we're now holding.
+                        */
+                       RCU_INIT_POINTER(task->perf_event_ctxp[ctxn], next_ctx);
+                       RCU_INIT_POINTER(next->perf_event_ctxp[ctxn], ctx);
+
                        do_switch = 0;
 
                        perf_event_sync_stat(ctx, next_ctx);
@@ -8545,7 +8566,7 @@ perf_event_create_kernel_counter(struct perf_event_attr *attr, int cpu,
        }
 
        /* Mark owner so we could distinguish it from user events. */
-       event->owner = EVENT_OWNER_KERNEL;
+       event->owner = TASK_TOMBSTONE;
 
        account_event(event);
 
@@ -8725,28 +8746,26 @@ __perf_event_exit_task(struct perf_event *child_event,
 
 static void perf_event_exit_task_context(struct task_struct *child, int ctxn)
 {
-       struct perf_event *child_event, *next;
        struct perf_event_context *child_ctx, *clone_ctx = NULL;
+       struct perf_event *child_event, *next;
+       unsigned long flags;
 
-       if (likely(!child->perf_event_ctxp[ctxn]))
+       WARN_ON_ONCE(child != current);
+
+       child_ctx = perf_lock_task_context(child, ctxn, &flags);
+       if (!child_ctx)
                return;
 
-       local_irq_disable();
-       WARN_ON_ONCE(child != current);
-       /*
-        * We can't reschedule here because interrupts are disabled,
-        * and child must be current.
-        */
-       child_ctx = rcu_dereference_raw(child->perf_event_ctxp[ctxn]);
+       task_ctx_sched_out(__get_cpu_context(child_ctx), child_ctx);
 
        /*
-        * Take the context lock here so that if find_get_context is
-        * reading child->perf_event_ctxp, we wait until it has
-        * incremented the context's refcount before we do put_ctx below.
+        * Now that the context is inactive, destroy the task <-> ctx relation
+        * and mark the context dead.
         */
-       raw_spin_lock(&child_ctx->lock);
-       task_ctx_sched_out(__get_cpu_context(child_ctx), child_ctx);
-       child->perf_event_ctxp[ctxn] = NULL;
+       RCU_INIT_POINTER(child->perf_event_ctxp[ctxn], NULL);
+       put_ctx(child_ctx); /* cannot be last */
+       WRITE_ONCE(child_ctx->task, TASK_TOMBSTONE);
+       put_task_struct(current); /* cannot be last */
 
        /*
         * If this context is a clone; unclone it so it can't get
@@ -8755,7 +8774,7 @@ static void perf_event_exit_task_context(struct task_struct *child, int ctxn)
         */
        clone_ctx = unclone_ctx(child_ctx);
        update_context_time(child_ctx);
-       raw_spin_unlock_irq(&child_ctx->lock);
+       raw_spin_unlock_irqrestore(&child_ctx->lock, flags);
 
        if (clone_ctx)
                put_ctx(clone_ctx);