Re: [PATCH 3.2 114/126] perf/core: Fix concurrent sys_perf_event_open() vs. 'move_group' race

Ben Hutchings
2017-02-21T00:46:42Z

[-- Attachment #1: Type: text/plain, Size: 2428 bytes --]

On Wed, 2017-02-15 at 22:41 +0000, Ben Hutchings wrote:
> 3.2.85-rc1 review patch.  If anyone has any objections, please let me know.
>
> ------------------
>
> From: Peter Zijlstra <peterz@infradead.org>
>
> 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:
>
>   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.
[...]
>   /*
>    * See perf_event_ctx_lock() for comments on the details
>    * of swizzling perf_event::ctx.
>    */
> - mutex_lock_double(&gctx->mutex, &ctx->mutex);
> -
>   perf_remove_from_context(group_leader, false);
>
>   /*
> @@ -6709,10 +6757,8 @@ SYSCALL_DEFINE5(perf_event_open,
>   ++ctx->generation;
>   perf_unpin_context(ctx);
>
> - if (move_group) {
> - mutex_unlock(&gctx->mutex);
> - put_ctx(gctx);
> - }
> + if (move_group)
> + perf_event_ctx_unlock(group_leader, gctx);
>   mutex_unlock(&ctx->mutex);
>
>   event->owner = current;
[...]

Peter has clarified that the last call to put_ctx(gctx) corresponds to
the reference cleared by perf_remove_from_context(group_leader, false)
above. So although perf_event_ctx_unlock() also calls put_ctx(gctx),
we really do want to drop two references here now and should keep the
direct call.

I made the same error when backporting to 3.16, and will fix that as
well.

Ben.

--
Ben Hutchings
73.46% of all statistics are made up.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]