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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1ec56e83-be22-480c-be60-5b898fc5f5b7@linux.intel.com>
Date: Fri, 14 Mar 2025 10:38:29 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Peter Zijlstra <peterz@...radead.org>
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 2025-03-14 9:50 a.m., Liang, Kan wrote:
> 
> 
> On 2025-03-14 8:59 a.m., Peter Zijlstra wrote:
>> On Thu, Mar 13, 2025 at 05:02:38PM -0700, kan.liang@...ux.intel.com wrote:
>>
>>> @@ -12551,6 +12813,14 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>>>  	if (err)
>>>  		return ERR_PTR(err);
>>>  
>>> +	if (event->attach_state & PERF_ATTACH_TASK_DATA) {
>>> +		err = attach_perf_ctx_data(event);
>>> +		if (err) {
>>> +			security_perf_event_free(event);
>>> +			return ERR_PTR(err);
>>> +		}
>>> +	}
>>> +
>>>  	/* symmetric to unaccount_event() in _free_event() */
>>>  	account_event(event);
>>>  
>>
>> This seems weird. We just pushed all the error handling into
>> __free_event(), and now you're adding one special case back.
>>
>> Also, you've placed it after security_perf_event_alloc(), which I
>> _think_ wants to be last.
> 
> Right, I will send a new series to fix it.
> 

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);
+	}
+
 	/*
 	 * Disallow uncore-task events. Similarly, disallow uncore-cgroup
 	 * events (they don't make sense as the cgroup will be different


Thanks,
Kan