| 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
| ||
|
Message-ID: <CAM9d7chsbpmVhhtzBgqD29Agb6xTMpxfwvqUdo6RKx2K1WN-DQ@mail.gmail.com>
Date: Thu, 10 Mar 2022 01:25:40 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Chengming Zhou <zhouchengming@...edance.com>
Cc: Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Stephane Eranian <eranian@...gle.com>,
linux-perf-users <linux-perf-users@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
songmuchun@...edance.com
Subject: Re: [RFC PATCH] perf/core: fix cpuctx cgrp warning
Hello,
On Tue, Mar 8, 2022 at 6:00 AM Chengming Zhou
<zhouchengming@...edance.com> wrote:
>
> There is a race problem that can trigger WARN_ON_ONCE(cpuctx->cgrp)
> in perf_cgroup_switch().
>
> CPU1 CPU2
> (in context_switch) (attach running task)
> perf_cgroup_sched_out(task, next)
> if (cgrp1 != cgrp2) True
> task->cgroups = xxx
> perf_cgroup_attach()
> perf_cgroup_sched_in(prev, task)
> if (cgrp1 != cgrp2) False
But perf_cgroup_switch will be synchronized as the context switch
disables the interrupt. And right, it still can see the task->cgroups
is changing in the middle.
>
> The commit a8d757ef076f ("perf events: Fix slow and broken cgroup
> context switch code") would save cpuctx switch in/out when the
> perf_cgroup of "prev" and "next" are the same.
>
> But perf_cgroup of task can change in concurrent with context_switch.
> If cgrp1 == cgrp2 in sched_out(), cpuctx won't do switch out, then
> task perf_cgroup changed cause cgrp1 != cgrp2 in sched_in(), cpuctx
> will do switch in, and trigger WARN_ON_ONCE(cpuctx->cgrp).
>
> The perf_cgroup of "prev" and "next" can be changed at any time, so we
> first have to combine perf_cgroup_sched_in() into perf_cgroup_sched_out(),
> so we can get a consistent value of condition (cgrp1 == cgrp2).
>
> And we introduce a percpu "cpu_perf_cgroups" to track the current used
> perf_cgroup, instead of using the unstable perf_cgroup of "prev", which
> maybe not the cpuctx->cgrp we used to schedule cgroup events on cpu.
Is this really needed? I think the warning comes because the two
cgroups were the same when in sched-out, but they became
different when in sched-in. So just combining sched-in/out should
be ok, isn't it?
>
> Fixes: a8d757ef076f ("perf events: Fix slow and broken cgroup context
> switch code")
> Signed-off-by: Chengming Zhou <zhouchengming@...edance.com>
> ---
> kernel/events/core.c | 95 +++++++++++---------------------------------
> 1 file changed, 23 insertions(+), 72 deletions(-)
>
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 6859229497b1..f3bc2841141f 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -826,6 +826,7 @@ perf_cgroup_set_timestamp(struct task_struct *task,
> }
> }
>
> +static DEFINE_PER_CPU(struct perf_cgroup *, cpu_perf_cgroups);
> static DEFINE_PER_CPU(struct list_head, cgrp_cpuctx_list);
>
> #define PERF_CGROUP_SWOUT 0x1 /* cgroup switch out every event */
> @@ -837,8 +838,9 @@ static DEFINE_PER_CPU(struct list_head, cgrp_cpuctx_list);
> * mode SWOUT : schedule out everything
> * mode SWIN : schedule in based on cgroup for next
You can remove this comment now.
> */
> -static void perf_cgroup_switch(struct task_struct *task, int mode)
> +static void perf_cgroup_switch(struct task_struct *task)
> {
> + struct perf_cgroup *cgrp;
> struct perf_cpu_context *cpuctx, *tmp;
> struct list_head *list;
> unsigned long flags;
> @@ -849,6 +851,9 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
> */
> local_irq_save(flags);
>
> + cgrp = perf_cgroup_from_task(task, NULL);
> + __this_cpu_write(cpu_perf_cgroups, cgrp);
> +
> list = this_cpu_ptr(&cgrp_cpuctx_list);
> list_for_each_entry_safe(cpuctx, tmp, list, cgrp_cpuctx_entry) {
> WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0);
> @@ -856,28 +861,15 @@ static void perf_cgroup_switch(struct task_struct *task, int mode)
> perf_ctx_lock(cpuctx, cpuctx->task_ctx);
> perf_pmu_disable(cpuctx->ctx.pmu);
>
> - if (mode & PERF_CGROUP_SWOUT) {
> - cpu_ctx_sched_out(cpuctx, EVENT_ALL);
> - /*
> - * must not be done before ctxswout due
> - * to event_filter_match() in event_sched_out()
Unrelated, but I don't see the event_filter_match() in
event_sched_out() anymore. Does it sched-out all
non-cgroup cpu events here?
> - */
> - cpuctx->cgrp = NULL;
> - }
> + cpu_ctx_sched_out(cpuctx, EVENT_ALL);
> + /*
> + * must not be done before ctxswout due
> + * to event_filter_match() in event_sched_out()
> + */
> + cpuctx->cgrp = cgrp;
Maybe we can check cpuctx->cgrp is the same as task's
cgroup before accessing the pmu. As in the commit message
it can call perf_cgroup_switch() after the context switch so
the cgroup events might be scheduled already.
Thanks,
Namhyung
> +
> + cpu_ctx_sched_in(cpuctx, EVENT_ALL, task);
>
> - if (mode & PERF_CGROUP_SWIN) {
> - WARN_ON_ONCE(cpuctx->cgrp);
> - /*
> - * set cgrp before ctxsw in to allow
> - * event_filter_match() to not have to pass
> - * task around
> - * we pass the cpuctx->ctx to perf_cgroup_from_task()
> - * because cgorup events are only per-cpu
> - */
> - cpuctx->cgrp = perf_cgroup_from_task(task,
> - &cpuctx->ctx);
> - cpu_ctx_sched_in(cpuctx, EVENT_ALL, task);
> - }
> perf_pmu_enable(cpuctx->ctx.pmu);
> perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
> }
Powered by blists - more mailing lists