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

sashiko-bot
2026-04-24T17:37:46Z

Sashiko AI review found 3 potential issue(s):
- [High] Time-Of-Check to Time-Of-Use (TOCTOU) race condition in `evlist__put()` breaks atomic reference counting, leading to Double Free or Use-After-Free.
- [High] Reassigning the local parameter `evlist` in `iostat_prepare()` causes a Use-After-Free crash in `cmd_stat()`. (Pre-existing issue)
- [Medium] Leaked `evlist` reference when removing `probe:vfs_getname` events due to bypassing `evlist__remove()`.
--

commit 277a4b5e346eb6e74fad04ad6b9e88d0d32aa103
Author: Ian Rogers <irogers@google.com>

perf evlist: Add reference count checking

This patch adds reference count checking to the perf evlist object so that
gets and puts are paired and easy to debug. It also introduces accessors to
evlist functions and updates the codebase to use them.

> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index a362f338f1044a..29588af735e5af 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
[ ... ]
> @@ -215,16 +225,32 @@ 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:

Can this sequence result in a race condition that breaks atomic reference
counting?

Because the decrement via refcount_dec_and_test() and the subsequent read
are not atomic, concurrent calls to evlist__put() could both decrement the
refcount before either reads it. Both threads would then read the same count,
subtract the back-references, see a count of zero, and proceed to out_delete,
causing a double free.

Similarly, a concurrent evlist__get() could increment the refcount after
the count is read, leaving the other thread with a dangling pointer since
the first thread proceeds to free the evlist.

[ ... ]
> @@ -235,10 +257,11 @@ void evlist__put(struct evlist *evlist)
>
> void evlist__add(struct evlist *evlist, struct evsel *entry)
> {
> - perf_evlist__add(&evlist->core, &entry->core);
> - entry->evlist = evlist;
> + perf_evlist__add(evlist__core(evlist), &entry->core);
> + evlist__put(entry->evlist);
> + entry->evlist = evlist__get(evlist);
> entry->tracking = !entry->core.idx;

Does this change leak the evlist reference when probe:vfs_getname events
are removed in evlist__add_vfs_getname()?

With the new reference counting semantics, adding an evsel to an evlist
acquires a reference to the evlist. In evlist__add_vfs_getname(), unmatched
evsels are manually removed from the list by explicitly setting
evsel->evlist = NULL and calling evsel__put(evsel).

By bypassing evlist__remove() and setting evsel->evlist = NULL without
calling evlist__put(), the reference to the evlist acquired during insertion
is permanently leaked.

Because the event is removed from the linked list, the cycle-breaking loop
in evlist__put() will not traverse it. The evlist reference count will never
reach 0, leaking the entire list and its resources upon exit.

> diff --git a/tools/perf/arch/x86/util/iostat.c b/tools/perf/arch/x86/util/iostat.c
> index e0417552b0cbd1..a0baa6cdefd891 100644
> --- a/tools/perf/arch/x86/util/iostat.c
> +++ b/tools/perf/arch/x86/util/iostat.c
[ ... ]
> @@ -334,7 +334,7 @@ static int iostat_event_group(struct evlist *evl,
>
> int iostat_prepare(struct evlist *evlist, struct perf_stat_config *config)
> {
> - if (evlist->core.nr_entries > 0) {
> + if (evlist__nr_entries(evlist) > 0) {
> pr_warning("The -e and -M options are not supported."
> "All chosen events/metrics will be dropped\n");
> evlist__put(evlist);
> evlist = evlist__new();

This isn't a bug introduced by this patch, but does this code cause a
use-after-free when reassigning the evlist?

Since evlist is passed by value, reassigning evlist = evlist__new() only
updates the local parameter. If the user passes unsupported arguments,
evlist__nr_entries(evlist) > 0 evaluates to true, and evlist__put(evlist)
frees the original list.

The caller retains its original pointer, which now points to freed memory,
potentially causing a crash when it iterates over it later.

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