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@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxx>