linux-kernel - Re: [PATCH V9 3/7] perf: attach/detach PMU specific data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250314210358.GA21786@noisy.programming.kicks-ass.net>
Date: Fri, 14 Mar 2025 22:03:58 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: "Liang, Kan" <kan.liang@...ux.intel.com>
Cc: mingo@...hat.com, tglx@...utronix.de, bp@...en8.de, acme@...nel.org,
	namhyung@...nel.org, irogers@...gle.com,
	linux-kernel@...r.kernel.org, ak@...ux.intel.com,
	eranian@...gle.com
Subject: Re: [PATCH V9 3/7] perf: attach/detach PMU specific data

On Fri, Mar 14, 2025 at 10:38:29AM -0400, Liang, Kan wrote:
> I think the attach_perf_ctx_data() should be moved even earlier, right
> after the perf_init_event(). Because the PERF_ATTACH_TASK_DATA is set in
> the event_init()->hw_config(), rather than after perf attach the data.
> 
> So any errors between perf_init_event() and attach_perf_ctx_data(event)
> would cause the detach_perf_ctx_data() mistakenly invoked.
> 
> @@ -5393,6 +5607,9 @@ static void __free_event(struct perf_event *event)
>  	if (is_cgroup_event(event))
>  		perf_detach_cgroup(event);
> 
> +	if (event->attach_state & PERF_ATTACH_TASK_DATA)
> +		detach_perf_ctx_data(event);
> +
>  	if (event->destroy)
>  		event->destroy(event);
> @@ -12481,6 +12746,18 @@ perf_event_alloc(struct perf_event_attr *attr,
> int cpu,
>  	if (IS_ERR(pmu))
>  		return (void*)pmu;
> 
> +	/*
> +	 * The PERF_ATTACH_TASK_DATA is set in the event_init()->hw_config().
> +	 * The attach should be right after the perf_init_event().
> +	 * Otherwise, the __free_event() would mistakenly detach the non-exist
> +	 * perf_ctx_data because of the other errors between them.
> +	 */
> +	if (event->attach_state & PERF_ATTACH_TASK_DATA) {
> +		err = attach_perf_ctx_data(event);
> + 		if (err)
> +			return ERR_PTR(err);
> +	}
> +

AFAICT it is possible for perF_init_event() to return an error *and* set
ATTACH_TASK_DATA, at which point we're up a creek with __free_event().

I'm afraid we need more state to track this.

(additionally, I'll need to rebase the pmu_unregister patch on top of
this -- that needs more changes still).

Anyway, let me go poke at this a bit, see what I can do with v10.

I'll reply there.