[PATCH] perf/core: Remove group siblings before the leader in move_group

[PATCH] perf/core: Remove group siblings before the leader in move_group

[Jun Yang]

From: Jun Yang <junvyyang@tencent.com>

During group migration, perf_event_open() removes the group leader before
its siblings. The context's active lists contain only group leaders, so a
concurrent context switch can no longer reach the remaining active
siblings after the leader has been removed.

The context can then become inactive while those siblings are still
active. Removing a sibling in this state takes the inactive-context path
and calls pmu->del() on the calling CPU rather than the CPU where the event
is running.

Remove the siblings first so that a concurrent context switch can still
schedule them out through the leader.

Fixes: bd2756811766 ("perf: Rewrite core context handling")
Cc: stable@kernel.org
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Assisted-by: tencentos-corvus-ai:kimi-k3
Signed-off-by: Jun Yang <junvyyang@tencent.com>
---
A KASAN reproducer for this issue is available if requested.

 kernel/events/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index fe33fe1..5bf8053 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -14230,14 +14230,15 @@ SYSCALL_DEFINE5(perf_event_open,
 	 */
 
 	if (move_group) {
-		perf_remove_from_context(group_leader, 0);
-		put_pmu_ctx(group_leader->pmu_ctx);
-
+		/* Keep siblings reachable by group_sched_out(). */
 		for_each_sibling_event(sibling, group_leader) {
 			perf_remove_from_context(sibling, 0);
 			put_pmu_ctx(sibling->pmu_ctx);
 		}
 
+		perf_remove_from_context(group_leader, 0);
+		put_pmu_ctx(group_leader->pmu_ctx);
+
 		/*
 		 * Install the group siblings before the group leader.
 		 *
-- 
2.54.0 (Apple Git-157)

Re: [PATCH] perf/core: Remove group siblings before the leader in move_group

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] perf/core: use-after-free of sibling->pmu_ctx in perf_event_open()
--

commit dae4cdd7da7aed3ce65185b981d41f47553d78fb
Author: Jun Yang <junvyyang@tencent.com>

perf/core: Remove group siblings before the leader in move_group

This commit changes the group migration path in perf_event_open() to remove
sibling events from the context before removing the group leader, so that a
concurrent context switch can still schedule active siblings out through the
leader.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index c180b174bac7c..d16178f3f53e6 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -14335,14 +14335,15 @@ SYSCALL_DEFINE5(perf_event_open,
>  	 */
>  
>  	if (move_group) {
> -		perf_remove_from_context(group_leader, 0);
> -		put_pmu_ctx(group_leader->pmu_ctx);
> -
> +		/* Keep siblings reachable by group_sched_out(). */
>  		for_each_sibling_event(sibling, group_leader) {
>  			perf_remove_from_context(sibling, 0);
>  			put_pmu_ctx(sibling->pmu_ctx);
>  		}
>  
> +		perf_remove_from_context(group_leader, 0);
> +		put_pmu_ctx(group_leader->pmu_ctx);

[Severity: Critical]
Can calling put_pmu_ctx(sibling->pmu_ctx) before removing group_leader cause
a use-after-free of sibling->pmu_ctx during a concurrent context switch?

In a pure software group, a software sibling backed by a different software
PMU from group_leader (for example, perf_task_clock or perf_tracepoint paired
with a perf_swevent leader) keeps its own PMU because
in_software_context(group_leader) is true, and gets a separate
perf_event_pmu_context via find_get_pmu_context():

kernel/events/core.c:SYSCALL_DEFINE5(perf_event_open) {
    ...
    if (is_software_event(event) &&
        !in_software_context(group_leader)) {
        pmu = group_leader->pmu_ctx->pmu;
    }
    ...
    pmu_ctx = find_get_pmu_context(pmu, ctx, event);
    ...
    event->pmu_ctx = pmu_ctx;
}

When migrating that group to a hardware context (move_group == 1),
perf_remove_from_context(sibling, 0) leaves sibling on
group_leader->sibling_list, and put_pmu_ctx(sibling->pmu_ctx) is now called
while group_leader is still active in ctx and before sibling->pmu_ctx is
reassigned to the new pmu_ctx.

If sibling holds the sole reference to its perf_event_pmu_context,
put_pmu_ctx() drops epc->refcount to zero and queues free_epc_rcu() via
call_rcu(). Because perf_event_open() runs in preemptible process context
holding only pmus_srcu and ctx->mutex, and subsequent
perf_remove_from_context() calls invoke event_function_call() ->
task_function_call(), an RCU grace period can elapse and free
sibling->pmu_ctx before perf_remove_from_context(group_leader, 0) completes.

If a concurrent context switch or reschedule on the target task's CPU invokes
group_sched_out() -> event_sched_out() or group_sched_in() ->
event_sched_in() for group_leader, both functions walk
group_leader->sibling_list and dereference event->pmu_ctx->pmu via
this_cpc(epc->pmu) before checking event->state:

kernel/events/core.c:event_sched_out() {
    struct perf_event_pmu_context *epc = event->pmu_ctx;
    struct perf_cpu_pmu_context *cpc = this_cpc(epc->pmu);
    enum perf_event_state state = PERF_EVENT_STATE_INACTIVE;
    ...
    if (event->state != PERF_EVENT_STATE_ACTIVE)
        return;
}

kernel/events/core.c:event_sched_in() {
    struct perf_event_pmu_context *epc = event->pmu_ctx;
    struct perf_cpu_pmu_context *cpc = this_cpc(epc->pmu);
    int ret = 0;
    ...
    if (event->state <= PERF_EVENT_STATE_OFF)
        return 0;
}

Should put_pmu_ctx(sibling->pmu_ctx) be deferred until after group_leader has
been removed from ctx?

> +
>  		/*
>  		 * Install the group siblings before the group leader.
>  		 *

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022083849.90948-1-juny24602@gmail.com?part=1