From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752183AbdANM3X (ORCPT ); Sat, 14 Jan 2017 07:29:23 -0500 Received: from terminus.zytor.com ([198.137.202.10]:50808 "EHLO terminus.zytor.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751919AbdANM3V (ORCPT ); Sat, 14 Jan 2017 07:29:21 -0500 Date: Sat, 14 Jan 2017 04:28:42 -0800 From: tip-bot for Peter Zijlstra Message-ID: Cc: acme@kernel.org, jolsa@redhat.com, linux-kernel@vger.kernel.org, eranian@google.com, mchong@google.com, joaodias@google.com, keescook@chromium.org, mingo@kernel.org, acme@redhat.com, alexander.shishkin@linux.intel.com, hpa@zytor.com, tglx@linutronix.de, peterz@infradead.org, torvalds@linux-foundation.org, vincent.weaver@maine.edu Reply-To: mingo@kernel.org, keescook@chromium.org, joaodias@google.com, mchong@google.com, linux-kernel@vger.kernel.org, jolsa@redhat.com, eranian@google.com, acme@kernel.org, vincent.weaver@maine.edu, torvalds@linux-foundation.org, peterz@infradead.org, tglx@linutronix.de, alexander.shishkin@linux.intel.com, hpa@zytor.com, acme@redhat.com In-Reply-To: <20170106131444.GZ3174@twins.programming.kicks-ass.net> References: <20170106131444.GZ3174@twins.programming.kicks-ass.net> To: linux-tip-commits@vger.kernel.org Subject: [tip:perf/urgent] perf/core: Fix concurrent sys_perf_event_open() vs. 'move_group' race Git-Commit-ID: 321027c1fe77f892f4ea07846aeae08cefbbb290 X-Mailer: tip-git-log-daemon Robot-ID: Robot-Unsubscribe: Contact to get blacklisted from these emails MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=UTF-8 Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Commit-ID: 321027c1fe77f892f4ea07846aeae08cefbbb290 Gitweb: http://git.kernel.org/tip/321027c1fe77f892f4ea07846aeae08cefbbb290 Author: Peter Zijlstra AuthorDate: Wed, 11 Jan 2017 21:09:50 +0100 Committer: Ingo Molnar CommitDate: Sat, 14 Jan 2017 10:56:11 +0100 perf/core: Fix concurrent sys_perf_event_open() vs. 'move_group' race 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: f63a8daa5812 ("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 Signed-off-by: Peter Zijlstra (Intel) Cc: Alexander Shishkin Cc: Arnaldo Carvalho de Melo Cc: Arnaldo Carvalho de Melo Cc: Jiri Olsa Cc: Kees Cook Cc: Linus Torvalds Cc: Min Chong Cc: Peter Zijlstra Cc: Stephane Eranian Cc: Thomas Gleixner Cc: Vince Weaver Fixes: f63a8daa5812 ("perf: Fix event->ctx locking") Link: http://lkml.kernel.org/r/20170106131444.GZ3174@twins.programming.kicks-ass.net Signed-off-by: Ingo Molnar --- kernel/events/core.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 4 deletions(-) diff --git a/kernel/events/core.c b/kernel/events/core.c index 72ce7d6..cbc5937 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -9529,6 +9529,37 @@ static int perf_event_set_clock(struct perf_event *event, clockid_t clk_id) return 0; } +/* + * Variation on perf_event_ctx_lock_nested(), except we take two context + * mutexes. + */ +static struct perf_event_context * +__perf_event_ctx_lock_double(struct perf_event *group_leader, + struct perf_event_context *ctx) +{ + struct perf_event_context *gctx; + +again: + rcu_read_lock(); + gctx = READ_ONCE(group_leader->ctx); + if (!atomic_inc_not_zero(&gctx->refcount)) { + rcu_read_unlock(); + goto again; + } + rcu_read_unlock(); + + mutex_lock_double(&gctx->mutex, &ctx->mutex); + + if (group_leader->ctx != gctx) { + mutex_unlock(&ctx->mutex); + mutex_unlock(&gctx->mutex); + put_ctx(gctx); + goto again; + } + + return gctx; +} + /** * sys_perf_event_open - open a performance event, associate it to a task/cpu * @@ -9772,12 +9803,31 @@ SYSCALL_DEFINE5(perf_event_open, } if (move_group) { - gctx = group_leader->ctx; - mutex_lock_double(&gctx->mutex, &ctx->mutex); + gctx = __perf_event_ctx_lock_double(group_leader, ctx); + if (gctx->task == TASK_TOMBSTONE) { err = -ESRCH; goto err_locked; } + + /* + * Check if we raced against another sys_perf_event_open() call + * moving the software group underneath us. + */ + if (!(group_leader->group_caps & PERF_EV_CAP_SOFTWARE)) { + /* + * If someone moved the group out from under us, check + * if this new event wound up on the same ctx, if so + * its the regular !move_group case, otherwise fail. + */ + if (gctx != ctx) { + err = -EINVAL; + goto err_locked; + } else { + perf_event_ctx_unlock(group_leader, gctx); + move_group = 0; + } + } } else { mutex_lock(&ctx->mutex); } @@ -9879,7 +9929,7 @@ SYSCALL_DEFINE5(perf_event_open, perf_unpin_context(ctx); if (move_group) - mutex_unlock(&gctx->mutex); + perf_event_ctx_unlock(group_leader, gctx); mutex_unlock(&ctx->mutex); if (task) { @@ -9905,7 +9955,7 @@ SYSCALL_DEFINE5(perf_event_open, err_locked: if (move_group) - mutex_unlock(&gctx->mutex); + perf_event_ctx_unlock(group_leader, gctx); mutex_unlock(&ctx->mutex); /* err_file: */ fput(event_file);