[PATCH v1] perf cgroup: Update metric leader in evlist__expand_cgroup

[PATCH v1] perf cgroup: Update metric leader in evlist__expand_cgroup

[Ian Rogers]

When the evlist is expanded the metric leader wasn't being updated. As
the original evsel is deleted this creates a use-after-free in
stat-shadow's prepare_metric. This was detected running the "perf stat
--bpf-counters --for-each-cgroup test" with sanitizers.

The change itself puts the copied evsel into the priv field (known
unused because of evsel__clone use) and then in a second pass over the
list updates the copied values using the priv pointer.

Fixes: d1c5a0e86a4e ("perf stat: Add --for-each-cgroup option")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/cgroup.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index 040eb75f0804..1e0fef283826 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -417,7 +417,6 @@ static bool has_pattern_string(const char *str)
 int evlist__expand_cgroup(struct evlist *evlist, const char *str, bool open_cgroup)
 {
 	struct evlist *orig_list, *tmp_list;
-	struct evsel *pos, *evsel, *leader;
 	struct rblist orig_metric_events;
 	struct cgroup *cgrp = NULL;
 	struct cgroup_name *cn;
@@ -452,6 +451,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str, bool open_cgro
 		goto out_err;
 
 	list_for_each_entry(cn, &cgroup_list, list) {
+		struct evsel *pos;
 		char *name;
 
 		if (!cn->used)
@@ -467,21 +467,41 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str, bool open_cgro
 		if (cgrp == NULL)
 			continue;
 
-		leader = NULL;
+		/* copy the list and set to the new cgroup. */
 		evlist__for_each_entry(orig_list, pos) {
-			evsel = evsel__clone(/*dest=*/NULL, pos);
+			struct evsel *evsel = evsel__clone(/*dest=*/NULL, pos);
+
 			if (evsel == NULL)
 				goto out_err;
 
+			/* stash the copy during the copying. */
+			pos->priv = &evsel->core;
 			cgroup__put(evsel->cgrp);
 			evsel->cgrp = cgroup__get(cgrp);
 
-			if (evsel__is_group_leader(pos))
-				leader = evsel;
-			evsel__set_leader(evsel, leader);
-
 			evlist__add(tmp_list, evsel);
 		}
+		/* update leader information using stashed pointer to copy. */
+		evlist__for_each_entry(orig_list, pos) {
+			struct evsel *evsel = pos->priv;
+
+			if (evsel__leader(pos))
+				evsel->core.leader = evsel__leader(pos)->priv;
+
+			if (pos->metric_leader) {
+				evsel->metric_leader =
+					container_of(pos->metric_leader->priv, struct evsel, core);
+			}
+			if (pos->first_wildcard_match) {
+				evsel->first_wildcard_match =
+					container_of(pos->first_wildcard_match->priv,
+						     struct evsel, core);
+			}
+		}
+		/* the stashed copy is no longer used. */
+		evlist__for_each_entry(orig_list, pos)
+			pos->priv = NULL;
+
 		/* cgroup__new() has a refcount, release it here */
 		cgroup__put(cgrp);
 		nr_cgroups++;
-- 
2.53.0.1213.gd9a14994de-goog

Re: [PATCH v1] perf cgroup: Update metric leader in evlist__expand_cgroup

[sun jian]

On Fri, Apr 3, 2026 at 5:58 AM Ian Rogers <irogers@google.com> wrote:
>
> When the evlist is expanded the metric leader wasn't being updated. As
> the original evsel is deleted this creates a use-after-free in
> stat-shadow's prepare_metric. This was detected running the "perf stat
> --bpf-counters --for-each-cgroup test" with sanitizers.
>
> The change itself puts the copied evsel into the priv field (known
> unused because of evsel__clone use) and then in a second pass over the
> list updates the copied values using the priv pointer.
>
> Fixes: d1c5a0e86a4e ("perf stat: Add --for-each-cgroup option")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/cgroup.c | 34 +++++++++++++++++++++++++++-------
>  1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
> index 040eb75f0804..1e0fef283826 100644
> --- a/tools/perf/util/cgroup.c
> +++ b/tools/perf/util/cgroup.c
> @@ -417,7 +417,6 @@ static bool has_pattern_string(const char *str)
>  int evlist__expand_cgroup(struct evlist *evlist, const char *str, bool open_cgroup)
>  {
>         struct evlist *orig_list, *tmp_list;
> -       struct evsel *pos, *evsel, *leader;
>         struct rblist orig_metric_events;
>         struct cgroup *cgrp = NULL;
>         struct cgroup_name *cn;
> @@ -452,6 +451,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str, bool open_cgro
>                 goto out_err;
>
>         list_for_each_entry(cn, &cgroup_list, list) {
> +               struct evsel *pos;
>                 char *name;
>
>                 if (!cn->used)
> @@ -467,21 +467,41 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str, bool open_cgro
>                 if (cgrp == NULL)
>                         continue;
>
> -               leader = NULL;
> +               /* copy the list and set to the new cgroup. */
>                 evlist__for_each_entry(orig_list, pos) {
> -                       evsel = evsel__clone(/*dest=*/NULL, pos);
> +                       struct evsel *evsel = evsel__clone(/*dest=*/NULL, pos);
> +
>                         if (evsel == NULL)
>                                 goto out_err;
>
> +                       /* stash the copy during the copying. */
> +                       pos->priv = &evsel->core;
>                         cgroup__put(evsel->cgrp);
>                         evsel->cgrp = cgroup__get(cgrp);
>
> -                       if (evsel__is_group_leader(pos))
> -                               leader = evsel;
> -                       evsel__set_leader(evsel, leader);
> -
>                         evlist__add(tmp_list, evsel);
>                 }
> +               /* update leader information using stashed pointer to copy. */
> +               evlist__for_each_entry(orig_list, pos) {
> +                       struct evsel *evsel = pos->priv;
nit: since pos->priv stores &evsel->core above, could we use container_of() here
as well? Or just stash evsel directly into priv?
> +
> +                       if (evsel__leader(pos))
> +                               evsel->core.leader = evsel__leader(pos)->priv;
> +
> +                       if (pos->metric_leader) {
> +                               evsel->metric_leader =
> +                                       container_of(pos->metric_leader->priv, struct evsel, core);
> +                       }
> +                       if (pos->first_wildcard_match) {
> +                               evsel->first_wildcard_match =
> +                                       container_of(pos->first_wildcard_match->priv,
> +                                                    struct evsel, core);
> +                       }
> +               }
> +               /* the stashed copy is no longer used. */
> +               evlist__for_each_entry(orig_list, pos)
> +                       pos->priv = NULL;
> +
>                 /* cgroup__new() has a refcount, release it here */
>                 cgroup__put(cgrp);
>                 nr_cgroups++;
> --
> 2.53.0.1213.gd9a14994de-goog
>
>
This patch makes sense to me, apart from the nit above.

Acked-by: Sun Jian <sun.jian.kdev@gmail.com>

Re: [PATCH v1] perf cgroup: Update metric leader in evlist__expand_cgroup

[Ian Rogers]

On Thu, Apr 2, 2026 at 11:33 PM sun jian <sun.jian.kdev@gmail.com> wrote:
>
> On Fri, Apr 3, 2026 at 5:58 AM Ian Rogers <irogers@google.com> wrote:
> >
> > When the evlist is expanded the metric leader wasn't being updated. As
> > the original evsel is deleted this creates a use-after-free in
> > stat-shadow's prepare_metric. This was detected running the "perf stat
> > --bpf-counters --for-each-cgroup test" with sanitizers.
> >
> > The change itself puts the copied evsel into the priv field (known
> > unused because of evsel__clone use) and then in a second pass over the
> > list updates the copied values using the priv pointer.
> >
> > Fixes: d1c5a0e86a4e ("perf stat: Add --for-each-cgroup option")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/cgroup.c | 34 +++++++++++++++++++++++++++-------
> >  1 file changed, 27 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
> > index 040eb75f0804..1e0fef283826 100644
> > --- a/tools/perf/util/cgroup.c
> > +++ b/tools/perf/util/cgroup.c
> > @@ -417,7 +417,6 @@ static bool has_pattern_string(const char *str)
> >  int evlist__expand_cgroup(struct evlist *evlist, const char *str, bool open_cgroup)
> >  {
> >         struct evlist *orig_list, *tmp_list;
> > -       struct evsel *pos, *evsel, *leader;
> >         struct rblist orig_metric_events;
> >         struct cgroup *cgrp = NULL;
> >         struct cgroup_name *cn;
> > @@ -452,6 +451,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str, bool open_cgro
> >                 goto out_err;
> >
> >         list_for_each_entry(cn, &cgroup_list, list) {
> > +               struct evsel *pos;
> >                 char *name;
> >
> >                 if (!cn->used)
> > @@ -467,21 +467,41 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str, bool open_cgro
> >                 if (cgrp == NULL)
> >                         continue;
> >
> > -               leader = NULL;
> > +               /* copy the list and set to the new cgroup. */
> >                 evlist__for_each_entry(orig_list, pos) {
> > -                       evsel = evsel__clone(/*dest=*/NULL, pos);
> > +                       struct evsel *evsel = evsel__clone(/*dest=*/NULL, pos);
> > +
> >                         if (evsel == NULL)
> >                                 goto out_err;
> >
> > +                       /* stash the copy during the copying. */
> > +                       pos->priv = &evsel->core;
> >                         cgroup__put(evsel->cgrp);
> >                         evsel->cgrp = cgroup__get(cgrp);
> >
> > -                       if (evsel__is_group_leader(pos))
> > -                               leader = evsel;
> > -                       evsel__set_leader(evsel, leader);
> > -
> >                         evlist__add(tmp_list, evsel);
> >                 }
> > +               /* update leader information using stashed pointer to copy. */
> > +               evlist__for_each_entry(orig_list, pos) {
> > +                       struct evsel *evsel = pos->priv;
> nit: since pos->priv stores &evsel->core above, could we use container_of() here
> as well? Or just stash evsel directly into priv?

You're right, the perf_evsel vs the evsel is out-of-sync here (and
this was missed by Sashiko). I'll fix in v2.

Thanks,
Ian

> > +
> > +                       if (evsel__leader(pos))
> > +                               evsel->core.leader = evsel__leader(pos)->priv;
> > +
> > +                       if (pos->metric_leader) {
> > +                               evsel->metric_leader =
> > +                                       container_of(pos->metric_leader->priv, struct evsel, core);
> > +                       }
> > +                       if (pos->first_wildcard_match) {
> > +                               evsel->first_wildcard_match =
> > +                                       container_of(pos->first_wildcard_match->priv,
> > +                                                    struct evsel, core);
> > +                       }
> > +               }
> > +               /* the stashed copy is no longer used. */
> > +               evlist__for_each_entry(orig_list, pos)
> > +                       pos->priv = NULL;
> > +
> >                 /* cgroup__new() has a refcount, release it here */
> >                 cgroup__put(cgrp);
> >                 nr_cgroups++;
> > --
> > 2.53.0.1213.gd9a14994de-goog
> >
> >
> This patch makes sense to me, apart from the nit above.
>
> Acked-by: Sun Jian <sun.jian.kdev@gmail.com>

[PATCH v2] perf cgroup: Update metric leader in evlist__expand_cgroup

[Ian Rogers]

When the evlist is expanded the metric leader wasn't being updated. As
the original evsel is deleted this creates a use-after-free in
stat-shadow's prepare_metric. This was detected running the "perf stat
--bpf-counters --for-each-cgroup test" with sanitizers.

The change itself puts the copied evsel into the priv field (known
unused because of evsel__clone use) and then in a second pass over the
list updates the copied values using the priv pointer.

Fixes: d1c5a0e86a4e ("perf stat: Add --for-each-cgroup option")
Signed-off-by: Ian Rogers <irogers@google.com>
Acked-by: Sun Jian <sun.jian.kdev@gmail.com>
---
v2: Take better care over evsel vs perf_evsel (Sun Jian)
---
 tools/perf/util/cgroup.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index 040eb75f0804..1b5664d1481f 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -417,7 +417,6 @@ static bool has_pattern_string(const char *str)
 int evlist__expand_cgroup(struct evlist *evlist, const char *str, bool open_cgroup)
 {
 	struct evlist *orig_list, *tmp_list;
-	struct evsel *pos, *evsel, *leader;
 	struct rblist orig_metric_events;
 	struct cgroup *cgrp = NULL;
 	struct cgroup_name *cn;
@@ -452,6 +451,7 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str, bool open_cgro
 		goto out_err;
 
 	list_for_each_entry(cn, &cgroup_list, list) {
+		struct evsel *pos;
 		char *name;
 
 		if (!cn->used)
@@ -467,21 +467,37 @@ int evlist__expand_cgroup(struct evlist *evlist, const char *str, bool open_cgro
 		if (cgrp == NULL)
 			continue;
 
-		leader = NULL;
+		/* copy the list and set to the new cgroup. */
 		evlist__for_each_entry(orig_list, pos) {
-			evsel = evsel__clone(/*dest=*/NULL, pos);
+			struct evsel *evsel = evsel__clone(/*dest=*/NULL, pos);
+
 			if (evsel == NULL)
 				goto out_err;
 
+			/* stash the copy during the copying. */
+			pos->priv = evsel;
 			cgroup__put(evsel->cgrp);
 			evsel->cgrp = cgroup__get(cgrp);
 
-			if (evsel__is_group_leader(pos))
-				leader = evsel;
-			evsel__set_leader(evsel, leader);
-
 			evlist__add(tmp_list, evsel);
 		}
+		/* update leader information using stashed pointer to copy. */
+		evlist__for_each_entry(orig_list, pos) {
+			struct evsel *evsel = pos->priv;
+
+			if (evsel__leader(pos))
+				evsel__set_leader(evsel, evsel__leader(pos)->priv);
+
+			if (pos->metric_leader)
+				evsel->metric_leader = pos->metric_leader->priv;
+
+			if (pos->first_wildcard_match)
+				evsel->first_wildcard_match = pos->first_wildcard_match->priv;
+		}
+		/* the stashed copy is no longer used. */
+		evlist__for_each_entry(orig_list, pos)
+			pos->priv = NULL;
+
 		/* cgroup__new() has a refcount, release it here */
 		cgroup__put(cgrp);
 		nr_cgroups++;
-- 
2.53.0.1213.gd9a14994de-goog

Re: [PATCH v2] perf cgroup: Update metric leader in evlist__expand_cgroup

[Namhyung Kim]

On Fri, 03 Apr 2026 23:05:52 -0700, Ian Rogers wrote:
> When the evlist is expanded the metric leader wasn't being updated. As
> the original evsel is deleted this creates a use-after-free in
> stat-shadow's prepare_metric. This was detected running the "perf stat
> --bpf-counters --for-each-cgroup test" with sanitizers.
> 
> The change itself puts the copied evsel into the priv field (known
> unused because of evsel__clone use) and then in a second pass over the
> list updates the copied values using the priv pointer.
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung

Re: [PATCH v2] perf cgroup: Update metric leader in evlist__expand_cgroup

[Namhyung Kim]

On Fri, Apr 03, 2026 at 11:05:52PM -0700, Ian Rogers wrote:
> When the evlist is expanded the metric leader wasn't being updated. As
> the original evsel is deleted this creates a use-after-free in
> stat-shadow's prepare_metric. This was detected running the "perf stat
> --bpf-counters --for-each-cgroup test" with sanitizers.
> 
> The change itself puts the copied evsel into the priv field (known
> unused because of evsel__clone use) and then in a second pass over the
> list updates the copied values using the priv pointer.
> 
> Fixes: d1c5a0e86a4e ("perf stat: Add --for-each-cgroup option")
> Signed-off-by: Ian Rogers <irogers@google.com>
> Acked-by: Sun Jian <sun.jian.kdev@gmail.com>

Applied to perf-tools-next, thanks!

Best regards,
Namhyung