linux-kernel - Re: [tip: perf/core] perf: Fix event_function

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: <ZrwYCBazVKjBni/+@xpf.sh.intel.com>
Date: Wed, 14 Aug 2024 10:35:52 +0800
From: Pengfei Xu <pengfei.xu@...el.com>
To: Peter Zijlstra <peterz@...radead.org>
CC: Namhyung Kim <namhyung@...nel.org>, Naresh Kamboju
	<naresh.kamboju@...aro.org>, <kan.liang@...ux.intel.com>,
	<linux-kernel@...r.kernel.org>, <linux-tip-commits@...r.kernel.org>,
	<syzkaller-bugs@...glegroups.com>, <x86@...nel.org>,
	<lkft-triage@...ts.linaro.org>, <dan.carpenter@...aro.org>,
	<anders.roxell@...aro.org>, <arnd@...db.de>, Linux Kernel Functional Testing
	<lkft@...aro.org>, Andrii Nakryiko <andrii.nakryiko@...il.com>
Subject: Re: [tip: perf/core] perf: Fix event_function_call() locking

Hi Peter and Kim,

I tested this patch on top of 6.11.0-rc3-next-20240812.
This issue can not be reproduced in syzkaller reproducer.

Best Regards,
Thanks!

On 2024-08-13 at 23:02:09 +0200, Peter Zijlstra wrote:
> On Tue, Aug 13, 2024 at 11:28:54AM -0700, Namhyung Kim wrote:
> 
> Duh, yeah.
> 
> > ---
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 9893ba5e98aa..85204c2376fa 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -298,13 +298,14 @@ static int event_function(void *info)
> >  static void event_function_call(struct perf_event *event, event_f func, void *data)
> >  {
> >  	struct perf_event_context *ctx = event->ctx;
> > -	struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context);
> > +	struct perf_cpu_context *cpuctx;
> >  	struct task_struct *task = READ_ONCE(ctx->task); /* verified in event_function */
> >  	struct event_function_struct efs = {
> >  		.event = event,
> >  		.func = func,
> >  		.data = data,
> >  	};
> > +	unsigned long flags;
> >  
> >  	if (!event->parent) {
> >  		/*
> > @@ -327,22 +328,27 @@ static void event_function_call(struct perf_event *event, event_f func, void *da
> >  	if (!task_function_call(task, event_function, &efs))
> >  		return;
> >  
> > +	local_irq_save(flags);
> 
> This can just be local_irq_disable() though, seeing how the fingered
> commit replaced raw_spin_lock_irq().
> 
> I'll queue the below...
> 
> ---
> Subject: perf: Really fix event_function_call() locking
> From: Namhyung Kim <namhyung@...nel.org>
> Date: Tue Aug 13 22:55:11 CEST 2024
> 
> Commit 558abc7e3f89 ("perf: Fix event_function_call() locking") lost
> IRQ disabling by mistake.
> 
> Fixes: 558abc7e3f89 ("perf: Fix event_function_call() locking")
> Reported-by: Pengfei Xu <pengfei.xu@...el.com>
> Reported-by: Naresh Kamboju <naresh.kamboju@...aro.org>
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>  kernel/events/core.c |   13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -298,8 +298,8 @@ static int event_function(void *info)
>  static void event_function_call(struct perf_event *event, event_f func, void *data)
>  {
>  	struct perf_event_context *ctx = event->ctx;
> -	struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context);
>  	struct task_struct *task = READ_ONCE(ctx->task); /* verified in event_function */
> +	struct perf_cpu_context *cpuctx;
>  	struct event_function_struct efs = {
>  		.event = event,
>  		.func = func,
> @@ -327,22 +327,25 @@ static void event_function_call(struct p
>  	if (!task_function_call(task, event_function, &efs))
>  		return;
>  
> +	local_irq_disable();
> +	cpuctx = this_cpu_ptr(&perf_cpu_context);
>  	perf_ctx_lock(cpuctx, ctx);
>  	/*
>  	 * Reload the task pointer, it might have been changed by
>  	 * a concurrent perf_event_context_sched_out().
>  	 */
>  	task = ctx->task;
> -	if (task == TASK_TOMBSTONE) {
> -		perf_ctx_unlock(cpuctx, ctx);
> -		return;
> -	}
> +	if (task == TASK_TOMBSTONE)
> +		goto unlock;
>  	if (ctx->is_active) {
>  		perf_ctx_unlock(cpuctx, ctx);
> +		local_irq_enable();
>  		goto again;
>  	}
>  	func(event, NULL, ctx, data);
> +unlock:
>  	perf_ctx_unlock(cpuctx, ctx);
> +	local_irq_enable();
>  }
>  
>  /*