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.
Unified · Split
| @@ -9529,6 +9529,37 @@ static int perf_event_set_clock(struct perf_event *event, clockid_t clk_id) · 9528 unmodified lines | ||
| 9529 | 9529 | return 0; |
| 9530 | 9530 | } |
| 9531 | 9531 | |
| 9532 | +/* | |
| 9533 | + * Variation on perf_event_ctx_lock_nested(), except we take two context | |
| 9534 | + * mutexes. | |
| 9535 | + */ | |
| 9536 | +static struct perf_event_context * | |
| 9537 | +__perf_event_ctx_lock_double(struct perf_event *group_leader, | |
| 9538 | + struct perf_event_context *ctx) | |
| 9539 | +{ | |
| 9540 | + struct perf_event_context *gctx; | |
| 9541 | + | |
| 9542 | +again: | |
| 9543 | + rcu_read_lock(); | |
| 9544 | + gctx = READ_ONCE(group_leader->ctx); | |
| 9545 | + if (!atomic_inc_not_zero(&gctx->refcount)) { | |
| 9546 | + rcu_read_unlock(); | |
| 9547 | + goto again; | |
| 9548 | + } | |
| 9549 | + rcu_read_unlock(); | |
| 9550 | + | |
| 9551 | + mutex_lock_double(&gctx->mutex, &ctx->mutex); | |
| 9552 | + | |
| 9553 | + if (group_leader->ctx != gctx) { | |
| 9554 | + mutex_unlock(&ctx->mutex); | |
| 9555 | + mutex_unlock(&gctx->mutex); | |
| 9556 | + put_ctx(gctx); | |
| 9557 | + goto again; | |
| 9558 | + } | |
| 9559 | + | |
| 9560 | + return gctx; | |
| 9561 | +} | |
| 9562 | + | |
| 9532 | 9563 | /** |
| 9533 | 9564 | * sys_perf_event_open - open a performance event, associate it to a task/cpu |
| 9534 | 9565 | * |
| @@ -9772,12 +9803,31 @@ SYSCALL_DEFINE5(perf_event_open, · 237 unmodified lines | ||
| 9772 | 9803 | } |
| 9773 | 9804 | |
| 9774 | 9805 | if (move_group) { |
| 9775 | − gctx = group_leader->ctx; | |
| 9776 | − mutex_lock_double(&gctx->mutex, &ctx->mutex); | |
| 9806 | + gctx = __perf_event_ctx_lock_double(group_leader, ctx); | |
| 9807 | + | |
| 9777 | 9808 | if (gctx->task == TASK_TOMBSTONE) { |
| 9778 | 9809 | err = -ESRCH; |
| 9779 | 9810 | goto err_locked; |
| 9780 | 9811 | } |
| 9812 | + | |
| 9813 | + /* | |
| 9814 | + * Check if we raced against another sys_perf_event_open() call | |
| 9815 | + * moving the software group underneath us. | |
| 9816 | + */ | |
| 9817 | + if (!(group_leader->group_caps & PERF_EV_CAP_SOFTWARE)) { | |
| 9818 | + /* | |
| 9819 | + * If someone moved the group out from under us, check | |
| 9820 | + * if this new event wound up on the same ctx, if so | |
| 9821 | + * its the regular !move_group case, otherwise fail. | |
| 9822 | + */ | |
| 9823 | + if (gctx != ctx) { | |
| 9824 | + err = -EINVAL; | |
| 9825 | + goto err_locked; | |
| 9826 | + } else { | |
| 9827 | + perf_event_ctx_unlock(group_leader, gctx); | |
| 9828 | + move_group = 0; | |
| 9829 | + } | |
| 9830 | + } | |
| 9781 | 9831 | } else { |
| 9782 | 9832 | mutex_lock(&ctx->mutex); |
| 9783 | 9833 | } |
| @@ -9879,7 +9929,7 @@ SYSCALL_DEFINE5(perf_event_open, · 95 unmodified lines | ||
| 9879 | 9929 | perf_unpin_context(ctx); |
| 9880 | 9930 | |
| 9881 | 9931 | if (move_group) |
| 9882 | − mutex_unlock(&gctx->mutex); | |
| 9932 | + perf_event_ctx_unlock(group_leader, gctx); | |
| 9883 | 9933 | mutex_unlock(&ctx->mutex); |
| 9884 | 9934 | |
| 9885 | 9935 | if (task) { |
| @@ -9905,7 +9955,7 @@ SYSCALL_DEFINE5(perf_event_open, · 19 unmodified lines | ||
| 9905 | 9955 | |
| 9906 | 9956 | err_locked: |
| 9907 | 9957 | if (move_group) |
| 9908 | − mutex_unlock(&gctx->mutex); | |
| 9958 | + perf_event_ctx_unlock(group_leader, gctx); | |
| 9909 | 9959 | mutex_unlock(&ctx->mutex); |
| 9910 | 9960 | /* err_file: */ |
| 9911 | 9961 | fput(event_file); |