Re: [PATCH v3] perf/core: Fix sampling period inconsistency across CPU migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing put_ctx() in perf_get_task_ctxp() causes a permanent reference leak of perf_event_context and task_struct.
- [High] Erroneous perf_task_ctxp sharing clobbers states of independent profiling sessions.
--

commit 8823a0e265a5483fceb1f344697e122c9270c810
Author: Minwoo Ahn <mwahn402@gmail.com>

perf/core: Fix sampling period inconsistency across CPU migration

This commit introduces struct perf_task_context to preserve period_left across
CPU migrations for per-task software sampling events. It adds allocation and
refcounting logic to share this context among related events on the same task.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 6d1f8bad7e1c..f5cb1a273fbf 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
[ ... ]
> @@ -13259,6 +13291,41 @@ static void account_event(struct perf_event *event)
>  	account_pmu_sb_event(event);
>  }
>  
> +static struct perf_task_context *
> +perf_get_task_ctxp(struct perf_event *event, struct task_struct *task,
> +		   bool inherit)
> +{
> +	struct perf_task_context *ctxp = NULL;
> +	struct perf_event_context *ctx;
> +	struct perf_event *iter;
> +	unsigned long flags;
> +
> +	ctx = perf_lock_task_context(task, &flags);
> +	if (ctx) {
> +		list_for_each_entry(iter, &ctx->event_list, event_entry) {
> +			if (iter->perf_task_ctxp &&
> +			    (iter->owner == current ||
> +			     (inherit && !iter->owner)) &&

For inherited child events, both iter->owner and event->owner will be NULL.
If a parent task is being profiled by two separate and independent perf
sessions, will this (inherit && !iter->owner) condition inadvertently merge
both child events into a single perf_task_context?

Would it be safer to verify parent lineage, for example by checking if
iter->parent->perf_task_ctxp equals event->parent->perf_task_ctxp, to
ensure the inherited events actually belong to the same profiling session?

> +			    perf_event_equal_task_ctx(&iter->attr,
> +						     &event->attr)) {

Could this logic mistakenly merge the sampling states of completely
independent perf events in the same task?

If a process opens multiple distinct sampling events for the same task with
identical configurations and sample periods, they appear to pass this
equality check. Since they share the same perf_task_context, won't they
corrupt each other's period_left states when the task schedules out?

> +				ctxp = iter->perf_task_ctxp;
> +				refcount_inc(&ctxp->refcount);
> +				break;
> +			}
> +		}
> +		raw_spin_unlock_irqrestore(&ctx->lock, flags);

The perf_lock_task_context() helper takes a reference on the returned ctx
using refcount_inc_not_zero().

Does this path leak the perf_event_context (and consequently the pinned
task_struct) by returning without calling put_ctx(ctx) to drop that
reference?

> +	}
> +
> +	if (!ctxp) {
> +		ctxp = kzalloc_obj(struct perf_task_context);
> +		if (!ctxp)
> +			return NULL;
> +		refcount_set(&ctxp->refcount, 1);
> +	}
> +
> +	return ctxp;
> +}

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