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@xxxxxxxxxx>
> 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@xxxxxxxxx>
> Reported-by: Naresh Kamboju <naresh.kamboju@xxxxxxxxxx>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
> 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();
> }
>
> /*
|