linux-kernel - Re: [tip: perf/urgent] perf/core: Fix WARN in perf_cgroup

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: <3dfe387c-d1db-4cbe-b41d-8f9f27fe3a1f@huaweicloud.com>
Date: Thu, 26 Jun 2025 21:08:38 +0800
From: Luo Gengkun <luogengkun@...weicloud.com>
To: linux-kernel@...r.kernel.org, linux-tip-commits@...r.kernel.org
Cc: "Peter Zijlstra (Intel)" <peterz@...radead.org>, x86@...nel.org
Subject: Re: [tip: perf/urgent] perf/core: Fix WARN in perf_cgroup_switch()


On 2025/6/11 17:29, tip-bot2 for Luo Gengkun wrote:
> The following commit has been merged into the perf/urgent branch of tip:
>
> Commit-ID:     3172fb986666dfb71bf483b6d3539e1e587fa197
> Gitweb:        https://git.kernel.org/tip/3172fb986666dfb71bf483b6d3539e1e587fa197
> Author:        Luo Gengkun <luogengkun@...weicloud.com>
> AuthorDate:    Wed, 04 Jun 2025 03:39:24
> Committer:     Peter Zijlstra <peterz@...radead.org>
> CommitterDate: Thu, 05 Jun 2025 14:37:52 +02:00
>
> perf/core: Fix WARN in perf_cgroup_switch()
>
> There may be concurrency between perf_cgroup_switch and
> perf_cgroup_event_disable. Consider the following scenario: after a new
> perf cgroup event is created on CPU0, the new event may not trigger
> a reprogramming, causing ctx->is_active to be 0. In this case, when CPU1
> disables this perf event, it executes __perf_remove_from_context->
> list _del_event->perf_cgroup_event_disable on CPU1, which causes a race
> with perf_cgroup_switch running on CPU0.
>
> The following describes the details of this concurrency scenario:
>
> CPU0						CPU1
>
> perf_cgroup_switch:
>     ...
>     # cpuctx->cgrp is not NULL here
>     if (READ_ONCE(cpuctx->cgrp) == NULL)
>     	return;
>
> 						perf_remove_from_context:
> 						   ...
> 						   raw_spin_lock_irq(&ctx->lock);
> 						   ...
> 						   # ctx->is_active == 0 because reprogramm is not
> 						   # tigger, so CPU1 can do __perf_remove_from_context
> 						   # for CPU0
> 						   __perf_remove_from_context:
> 						         perf_cgroup_event_disable:
> 							    ...
> 							    if (--ctx->nr_cgroups)
> 							    ...
>
>     # this warning will happened because CPU1 changed
>     # ctx.nr_cgroups to 0.
>     WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0);
>
> [peterz: use guard instead of goto unlock]
> Fixes: db4a835601b7 ("perf/core: Set cgroup in CPU contexts for new cgroup events")
> Signed-off-by: Luo Gengkun <luogengkun@...weicloud.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> Link: https://lkml.kernel.org/r/20250604033924.3914647-3-luogengkun@huaweicloud.com

Sorry for the late reply, I found that the link is v2 instead of v3.
This v3 link is:

https://lore.kernel.org/all/20250609035316.250557-1-luogengkun@huaweicloud.com/

v2 attempts to fix a concurrency problem between perf_cgroup_switch
and perf_cgroup_event_disable. But it does not to move WARN_ON_ONCE
into lock-protected region, so the warning is still triggered.

The following patches have been tested and fix this issue.

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 1f746469fda5..a35784d42c66 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -951,8 +951,6 @@ static void perf_cgroup_switch(struct task_struct *task)
         if (READ_ONCE(cpuctx->cgrp) == NULL)
                 return;

-       WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0);
-
         cgrp = perf_cgroup_from_task(task, NULL);
         if (READ_ONCE(cpuctx->cgrp) == cgrp)
                 return;
@@ -964,6 +962,8 @@ static void perf_cgroup_switch(struct task_struct *task)
         if (READ_ONCE(cpuctx->cgrp) == NULL)
                 return;

+       WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0);
+
         perf_ctx_disable(&cpuctx->ctx, true);

         ctx_sched_out(&cpuctx->ctx, NULL, EVENT_ALL|EVENT_CGROUP);

> ---
>   kernel/events/core.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index d786083..d7cf008 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -207,6 +207,19 @@ static void perf_ctx_unlock(struct perf_cpu_context *cpuctx,
>   	__perf_ctx_unlock(&cpuctx->ctx);
>   }
>   
> +typedef struct {
> +	struct perf_cpu_context *cpuctx;
> +	struct perf_event_context *ctx;
> +} class_perf_ctx_lock_t;
> +
> +static inline void class_perf_ctx_lock_destructor(class_perf_ctx_lock_t *_T)
> +{ perf_ctx_unlock(_T->cpuctx, _T->ctx); }
> +
> +static inline class_perf_ctx_lock_t
> +class_perf_ctx_lock_constructor(struct perf_cpu_context *cpuctx,
> +				struct perf_event_context *ctx)
> +{ perf_ctx_lock(cpuctx, ctx); return (class_perf_ctx_lock_t){ cpuctx, ctx }; }
> +
>   #define TASK_TOMBSTONE ((void *)-1L)
>   
>   static bool is_kernel_event(struct perf_event *event)
> @@ -944,7 +957,13 @@ static void perf_cgroup_switch(struct task_struct *task)
>   	if (READ_ONCE(cpuctx->cgrp) == cgrp)
>   		return;
>   
> -	perf_ctx_lock(cpuctx, cpuctx->task_ctx);
> +	guard(perf_ctx_lock)(cpuctx, cpuctx->task_ctx);
> +	/*
> +	 * Re-check, could've raced vs perf_remove_from_context().
> +	 */
> +	if (READ_ONCE(cpuctx->cgrp) == NULL)
> +		return;
> +
>   	perf_ctx_disable(&cpuctx->ctx, true);
>   
>   	ctx_sched_out(&cpuctx->ctx, NULL, EVENT_ALL|EVENT_CGROUP);
> @@ -962,7 +981,6 @@ static void perf_cgroup_switch(struct task_struct *task)
>   	ctx_sched_in(&cpuctx->ctx, NULL, EVENT_ALL|EVENT_CGROUP);
>   
>   	perf_ctx_enable(&cpuctx->ctx, true);
> -	perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
>   }
>   
>   static int perf_cgroup_ensure_storage(struct perf_event *event,