Skip to content

Commit 416bd4a

Browse files
Peter Zijlstragregkh
authored andcommitted
perf/core: Fix concurrent sys_perf_event_open() vs. 'move_group' race
commit 321027c1fe77f892f4ea07846aeae08cefbbb290 upstream. Di Shen reported a race between two concurrent sys_perf_event_open() calls where both try and move the same pre-existing software group into a hardware context. The problem is exactly that described in commit: f63a8da ("perf: Fix event->ctx locking") ... where, while we wait for a ctx->mutex acquisition, the event->ctx relation can have changed under us. That very same commit failed to recognise sys_perf_event_context() as an external access vector to the events and thereby didn't apply the established locking rules correctly. So while one sys_perf_event_open() call is stuck waiting on mutex_lock_double(), the other (which owns said locks) moves the group about. So by the time the former sys_perf_event_open() acquires the locks, the context we've acquired is stale (and possibly dead). Apply the established locking rules as per perf_event_ctx_lock_nested() to the mutex_lock_double() for the 'move_group' case. This obviously means we need to validate state after we acquire the locks. Reported-by: Di Shen (Keen Lab) Tested-by: John Dias <joaodias@google.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Kees Cook <keescook@chromium.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Min Chong <mchong@google.com> 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> Fixes: f63a8da ("perf: Fix event->ctx locking") Link: http://lkml.kernel.org/r/20170106131444.GZ3174@twins.programming.kicks-ass.net Signed-off-by: Ingo Molnar <mingo@kernel.org> [bwh: Backported to 4.4: - Test perf_event::group_flags instead of group_caps - Adjust context] Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent b7f47c7 commit 416bd4a

1 file changed

Lines changed: 53 additions & 4 deletions

File tree

kernel/events/core.c

Lines changed: 53 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8250,6 +8250,37 @@ static int perf_event_set_clock(struct perf_event *event, clockid_t clk_id)
82508250
return 0;
82518251
}
82528252

8253+
/*
8254+
* Variation on perf_event_ctx_lock_nested(), except we take two context
8255+
* mutexes.
8256+
*/
8257+
static struct perf_event_context *
8258+
__perf_event_ctx_lock_double(struct perf_event *group_leader,
8259+
struct perf_event_context *ctx)
8260+
{
8261+
struct perf_event_context *gctx;
8262+
8263+
again:
8264+
rcu_read_lock();
8265+
gctx = READ_ONCE(group_leader->ctx);
8266+
if (!atomic_inc_not_zero(&gctx->refcount)) {
8267+
rcu_read_unlock();
8268+
goto again;
8269+
}
8270+
rcu_read_unlock();
8271+
8272+
mutex_lock_double(&gctx->mutex, &ctx->mutex);
8273+
8274+
if (group_leader->ctx != gctx) {
8275+
mutex_unlock(&ctx->mutex);
8276+
mutex_unlock(&gctx->mutex);
8277+
put_ctx(gctx);
8278+
goto again;
8279+
}
8280+
8281+
return gctx;
8282+
}
8283+
82538284
/**
82548285
* sys_perf_event_open - open a performance event, associate it to a task/cpu
82558286
*
@@ -8486,8 +8517,26 @@ SYSCALL_DEFINE5(perf_event_open,
84868517
}
84878518

84888519
if (move_group) {
8489-
gctx = group_leader->ctx;
8490-
mutex_lock_double(&gctx->mutex, &ctx->mutex);
8520+
gctx = __perf_event_ctx_lock_double(group_leader, ctx);
8521+
8522+
/*
8523+
* Check if we raced against another sys_perf_event_open() call
8524+
* moving the software group underneath us.
8525+
*/
8526+
if (!(group_leader->group_flags & PERF_GROUP_SOFTWARE)) {
8527+
/*
8528+
* If someone moved the group out from under us, check
8529+
* if this new event wound up on the same ctx, if so
8530+
* its the regular !move_group case, otherwise fail.
8531+
*/
8532+
if (gctx != ctx) {
8533+
err = -EINVAL;
8534+
goto err_locked;
8535+
} else {
8536+
perf_event_ctx_unlock(group_leader, gctx);
8537+
move_group = 0;
8538+
}
8539+
}
84918540
} else {
84928541
mutex_lock(&ctx->mutex);
84938542
}
@@ -8582,7 +8631,7 @@ SYSCALL_DEFINE5(perf_event_open,
85828631
perf_unpin_context(ctx);
85838632

85848633
if (move_group)
8585-
mutex_unlock(&gctx->mutex);
8634+
perf_event_ctx_unlock(group_leader, gctx);
85868635
mutex_unlock(&ctx->mutex);
85878636

85888637
if (task) {
@@ -8610,7 +8659,7 @@ SYSCALL_DEFINE5(perf_event_open,
86108659

86118660
err_locked:
86128661
if (move_group)
8613-
mutex_unlock(&gctx->mutex);
8662+
perf_event_ctx_unlock(group_leader, gctx);
86148663
mutex_unlock(&ctx->mutex);
86158664
/* err_file: */
86168665
fput(event_file);

0 commit comments

Comments
 (0)