Merge tag 'perf-urgent-2023-10-21' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip

Pull perf events fix from Ingo Molnar: "Fix group event semantics" * tag 'perf-urgent-2023-10-21' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip: perf: Disallow mis-matched inherited group reads

Linus Torvalds committed 3 years ago

Commit 94be1332 parents 023cc83 32671e32 files · +34 −6Browse files

Unified · Split

2 files

include/linux/perf_event.h +1 −0
kernel/events/core.c +33 −6

include/linux/perf_event.h+1−0

@@ -704,6 +704,7 @@ struct perf_event { · 703 unmodified lines
704	/* The cumulative AND of all event_caps for events in this group. */	704	/* The cumulative AND of all event_caps for events in this group. */
705	int group_caps;	705	int group_caps;
706		706
		707	unsigned int group_generation;
707	struct perf_event *group_leader;	708	struct perf_event *group_leader;
708	/*	709	/*
709	* event->pmu will always point to pmu in which this event belongs.	710	* event->pmu will always point to pmu in which this event belongs.

kernel/events/core.c+33−6

 	list_add_tail(&event->sibling_list, &group_leader->sibling_list);
 	group_leader->nr_siblings++;
+	group_leader->group_generation++;
 	perf_event__header_size(group_leader);
 	if (leader != event) {
 		list_del_init(&event->sibling_list);
 		event->group_leader->nr_siblings--;
+		event->group_leader->group_generation++;
 		goto out;
 	}
 					u64 read_format, u64 *values)
 {
 	struct perf_event_context *ctx = leader->ctx;
-	struct perf_event *sub;
+	struct perf_event *sub, *parent;
 	unsigned long flags;
 	int n = 1; /* skip @nr */
 	int ret;
 		return ret;
 	raw_spin_lock_irqsave(&ctx->lock, flags);
+	/*
+	 * Verify the grouping between the parent and child (inherited)
+	 * events is still in tact.
+	 *
+	 * Specifically:
+	 *  - leader->ctx->lock pins leader->sibling_list
+	 *  - parent->child_mutex pins parent->child_list
+	 *  - parent->ctx->mutex pins parent->sibling_list
+	 *
+	 * Because parent->ctx != leader->ctx (and child_list nests inside
+	 * ctx->mutex), group destruction is not atomic between children, also
+	 * see perf_event_release_kernel(). Additionally, parent can grow the
+	 * group.
+	 *
+	 * Therefore it is possible to have parent and child groups in a
+	 * different configuration and summing over such a beast makes no sense
+	 * what so ever.
+	 *
+	 * Reject this.
+	 */
+	parent = leader->parent;
+	if (parent &&
+	    (parent->group_generation != leader->group_generation ||
+	     parent->nr_siblings != leader->nr_siblings)) {
+		ret = -ECHILD;
+		goto unlock;
+	}
 	/*
 	 * Since we co-schedule groups, {enabled,running} times of siblings
 			values[n++] = atomic64_read(&sub->lost_samples);
 	}
+unlock:
 	raw_spin_unlock_irqrestore(&ctx->lock, flags);
-	return 0;
+	return ret;
 }
 static int perf_read_group(struct perf_event *event,
 	values[0] = 1 + leader->nr_siblings;
-	/*
-	 * By locking the child_mutex of the leader we effectively
-	 * lock the child list of all siblings.. XXX explain how.
-	 */
 	mutex_lock(&leader->child_mutex);
 	ret = __perf_read_group_add(leader, read_format, values);
 		    !perf_get_aux_event(child_ctr, leader))
 			return -EINVAL;
 	}
+	leader->group_generation = parent_event->group_generation;
 	return 0;
 }