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

[sashiko-bot@kernel.org]

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