Re: [PATCH v2 12/58] perf evlist: Add reference count checking

sashiko-bot
2026-04-23T04:34:41Z

Sashiko AI review found 4 potential issue(s):
- [High] Deterministic Use-After-Free in `evlist__remove` due to incorrect order of operations
- [High] Thread-safety violation in `evlist__put` cyclic reference detection leading to double-free race conditions
- [Medium] Refcount leak of `evsel` objects during sample delivery
- [Low] Missing NULL check on `from_list_start()` result in `perf_evlist__mmap_cb_idx()`
--

commit 3bea95f5803afd016cc89a08d42f43d3d2c999dd
Author: Ian Rogers <irogers@google.com>

perf evlist: Add reference count checking

Now the evlist is reference counted, add reference count checking so
that gets and puts are paired and easy to debug. Reference count
checking is documented here:
https://perfwiki.github.io/main/reference-count-checking/

> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
[ ... ]
> @@ -211,25 +225,44 @@ static void evlist__exit(struct evlist *evlist)
> 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:

Is this new cyclic reference detection thread-safe?

It appears to perform a non-atomic two-step check. If refcount_dec_and_test()
returns false, it proceeds to read the decremented refcount and iterate over
the entries without holding any locks.

If two threads drop their external references concurrently, could both threads
compute count == 0 and concurrently fall through to the out_delete path? This
would lead to list corruption and a double free.

[ ... ]
> @@ -238,15 +271,16 @@ void evlist__add(struct evlist *evlist, struct evsel *entry)
>
> void evlist__remove(struct evlist *evlist, struct evsel *evsel)
> {
> + evlist__put(evsel->evlist);
> evsel->evlist = NULL;
> - perf_evlist__remove(&evlist->core, &evsel->core);
> + perf_evlist__remove(evlist__core(evlist), &evsel->core);
> }

Can this order of operations trigger a use-after-free?

Because evlist__put() is called before perf_evlist__remove(), the evsel
is still linked in the list. When evlist__put() iterates over the list to
detect cyclic references, it might count this evsel as an internal reference.

This could artificially reduce the external reference count to 0 and
prematurely free the evlist. Immediately after evlist__put() returns,
perf_evlist__remove() is called, which would dereference the freshly freed
evlist pointer.

[ ... ]
> @@ -881,14 +910,24 @@ static void
> perf_evlist__mmap_cb_idx(struct perf_evlist *_evlist,
> struct perf_evsel *_evsel,
> struct perf_mmap_param *_mp,
> int idx)
> {
> - struct evlist *evlist = container_of(_evlist, struct evlist, core);
> + struct evlist *evlist = from_list_start(_evlist);
> struct mmap_params *mp = container_of(_mp, struct mmap_params, core);
> struct evsel *evsel = container_of(_evsel, struct evsel, core);
>
> auxtrace_mmap_params__set_idx(&mp->auxtrace_mp, evlist, evsel, idx);
> +
> + from_list_end(evlist);
> }

Should we verify if evlist is NULL before using it?

When REFCNT_CHECKING is enabled, from_list_start() allocates memory for a
new wrapper and can return NULL on failure. In this case, passing evlist to
auxtrace_mmap_params__set_idx() will dereference it, which might cause a
segmentation fault.

> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
[ ... ]
> @@ -1371,13 +1371,15 @@ static int evlist__deliver_deferred_callchain(struct evlist *evlist,
> struct evsel *saved_evsel = sample->evsel;
>
> sample->evsel = evlist__id2evsel(evlist, sample->id);
> + if (sample->evsel)
> + sample->evsel = evsel__get(sample->evsel);
> ret = tool->callchain_deferred(tool, event, sample,
> sample->evsel, machine);
> sample->evsel = saved_evsel;
> return ret;
> }

Is it possible that we are leaking an evsel reference here?

The code acquires a reference with evsel__get(), but then sample->evsel is
overwritten with saved_evsel without a corresponding call to evsel__put().

Since Perf processes millions of events, could this leak a reference for every
processed sample?

There are similar additions of evsel__get() without evsel__put() in
session__flush_deferred_samples() and machines__deliver_event() in this file.

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