linux-kernel - Re: [PATCH v2 24/24] perf: Make perf_pmu

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: <20250213130833.GH28068@noisy.programming.kicks-ass.net>
Date: Thu, 13 Feb 2025 14:08:33 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Ravi Bangoria <ravi.bangoria@....com>
Cc: mingo@...nel.org, lucas.demarchi@...el.com,
	linux-kernel@...r.kernel.org, willy@...radead.org, acme@...nel.org,
	namhyung@...nel.org, mark.rutland@....com,
	alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
	irogers@...gle.com, adrian.hunter@...el.com,
	kan.liang@...ux.intel.com
Subject: Re: [PATCH v2 24/24] perf: Make perf_pmu_unregister() useable

On Thu, Feb 13, 2025 at 01:22:55PM +0530, Ravi Bangoria wrote:
> > Does this work?
> > 
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -2303,6 +2303,7 @@ static void perf_child_detach(struct per
> >  
> >  	sync_child_event(event);
> >  	list_del_init(&event->child_list);
> > +	event->parent = NULL;
> >  }
> >  
> >  static bool is_orphaned_event(struct perf_event *event)
> 
> Apparently not, it ends up with:
> 
>   ------------[ cut here ]------------
>   WARNING: CPU: 145 PID: 5459 at kernel/events/core.c:281 event_function+0xd2/0xf0

Durr, do you have an updated test case?

> Something like below instead? I haven't tested it thoroughly though.
> 
> ---
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index d2b87a425e75..4e131b1c37ad 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -13645,20 +13645,25 @@ perf_event_exit_event(struct perf_event *event,
>  	unsigned long detach_flags = DETACH_EXIT;
>  
>  	if (parent_event) {
> -		/*
> -		 * Do not destroy the 'original' grouping; because of the
> -		 * context switch optimization the original events could've
> -		 * ended up in a random child task.
> -		 *
> -		 * If we were to destroy the original group, all group related
> -		 * operations would cease to function properly after this
> -		 * random child dies.
> -		 *
> -		 * Do destroy all inherited groups, we don't care about those
> -		 * and being thorough is better.
> -		 */
> -		detach_flags |= DETACH_GROUP | DETACH_CHILD;
>  		mutex_lock(&parent_event->child_mutex);
> +		if (event->attach_state & PERF_ATTACH_CHILD) {
> +			/*
> +			 * Do not destroy the 'original' grouping; because of the
> +			 * context switch optimization the original events could've
> +			 * ended up in a random child task.
> +			 *
> +			 * If we were to destroy the original group, all group related
> +			 * operations would cease to function properly after this
> +			 * random child dies.
> +			 *
> +			 * Do destroy all inherited groups, we don't care about those
> +			 * and being thorough is better.
> +			 */
> +			detach_flags |= DETACH_GROUP | DETACH_CHILD;
> +		} else {
> +			mutex_unlock(&parent_event->child_mutex);
> +			parent_event = NULL;
> +		}
>  	}

Yeah, that might do, but not really nice. But perhaps its the best we
can do. I'll give it some thought.