Re: [PATCH v6 12/59] perf evlist: Add reference count checking

sashiko-bot
2026-04-25T18:28:56Z

Sashiko AI review found 3 potential issue(s):
- [Critical] Double-free due to racy cycle-breaking garbage collection in `evlist__put()`.
- [High] Memory leak of `evlist` structures when splicing `evsel` entries.
- [High] Reference leak of `evsel` objects during event delivery.
--

commit 7675a76ec470f3ac5b584c42e3168949ccf7a93e
Author: Ian Rogers <irogers@google.com>

perf evlist: Add reference count checking

This patch adds reference counting and checking for evlist to ensure gets and
puts are paired correctly. It also renames evlist__mmap to evlist__do_mmap
and introduces accessors for evlist variables.

> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index a362f338f104..29588af735e5 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c

[ ... ]

> @@ -193,52 +200,76 @@ static void evlist__purge(struct evlist *evlist)
>
> evlist__for_each_entry_safe(evlist, n, pos) {
> list_del_init(&pos->core.node);
> + if (pos->evlist) {
> + /* Minimal evlist__put. */
> + refcount_dec_and_test(evlist__refcnt(pos->evlist));
> + RC_CHK_PUT(pos->evlist);
> + }
> pos->evlist = NULL;
> evsel__put(pos);
> }

Does this code leak the source evlist when splicing entries?

When evsel entries are moved from one evlist to another via
evlist__splice_list_tail(), their pos->evlist back-pointers are not updated.

When evlist__purge() later cleans up the destination evlist, it does a
minimal put on the old pos->evlist. However, if refcount_dec_and_test()
returns true, the result is ignored, so the cleanup logic and actual freeing
of the old evlist are bypassed.

Additionally, since the old evlist is now empty, a normal evlist__put() on
it would fail to break the reference cycle because it loops over an empty
list, failing to find the outstanding references.

[ ... ]

> void evlist__put(struct evlist *evlist)
> {
> + struct evsel *evsel;
> + unsigned int count;
> +
> if (evlist == NULL)
> return;
>
> - if (!refcount_dec_and_test(&evlist->refcnt))
> - return;
> + if (refcount_dec_and_test(evlist__refcnt(evlist)))
> + goto out_delete;
>
> + count = refcount_read(evlist__refcnt(evlist));
> + evlist__for_each_entry(evlist, evsel) {
> + if (RC_CHK_EQUAL(evsel->evlist, evlist) && count)
> + count--;
> + }
> + if (count != 0) {
> + /*
> + * Not the last reference except for back references from
> + * evsels.
> + */
> + RC_CHK_PUT(evlist);
> + return;
> + }
> +out_delete:
> evlist__free_stats(evlist);
> - evlist__munmap(evlist);
> + evlist__do_munmap(evlist);

Can this sequence lead to a double-free memory corruption?

If two threads concurrently release their external references to an evlist,
both might see refcount_dec_and_test() return false.

If they both proceed to read the refcount, iterate over the list, and
decrement count to 0, wouldn't both threads independently jump to out_delete
and free the evlist twice?

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index deb5b9dfe44c..f9fafbb80a9d 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c

[ ... ]

> @@ -1426,7 +1430,7 @@ static int session__flush_deferred_samples(struct perf_session *session,
> struct deferred_event *de, *tmp;
> int ret = 0;
>
> - list_for_each_entry_safe(de, tmp, &evlist->deferred_samples, list) {
> + list_for_each_entry_safe(de, tmp, evlist__deferred_samples(evlist), list) {
> struct perf_sample sample;
>
> perf_sample__init(&sample, /*all=*/false);
> @@ -1438,6 +1442,8 @@ static int session__flush_deferred_samples(struct perf_session *session,
> }
>
> sample.evsel = evlist__id2evsel(evlist, sample.id);
> + if (sample.evsel)
> + sample.evsel = evsel__get(sample.evsel);
> ret = evlist__deliver_sample(evlist, tool, de->event,
> &sample, sample.evsel, machine);

Does this leak the evsel reference?

We acquire an evsel reference using evsel__get() here, but unlike
evlist__deliver_deferred_callchain() which correctly drops it via
evsel__put() after delivery, the reference doesn't seem to be released
after calling evlist__deliver_sample().

[ ... ]

> @@ -1464,22 +1470,24 @@ static int machines__deliver_event(struct machines *machines,
>
> dump_event(evlist, event, file_offset, sample, file_path);
>
> - if (!sample->evsel)
> + if (!sample->evsel) {
> sample->evsel = evlist__id2evsel(evlist, sample->id);
> - else
> + if (sample->evsel)
> + sample->evsel = evsel__get(sample->evsel);
> + } else {
> assert(sample->evsel == evlist__id2evsel(evlist, sample->id));
> -
> + }
> evsel = sample->evsel;

Could this also leak the evsel reference?

Similar to the regression in session__flush_deferred_samples(), we get
a reference via evsel__get() when populating a missing sample->evsel,
but there appears to be no corresponding evsel__put() after the tool
callbacks are invoked.

--
Sashiko AI review · https://sashiko.dev/#/patchset/20260425174858.3922152-1-irogers@google.com?part=12