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

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

Sashiko AI review found 2 potential issue(s):
- [High] Python API bisectability breakage causing heap corruption upon evlist destruction.
- [High] Python API bisectability breakage causing `BUG_ON` crashes and broken mutability for `evlist[i]`.
--

commit 1fe320874850c1396e51c8aec7499414d09675c4
Author: Ian Rogers <irogers@google.com>
Subject: perf evlist: Add reference count

This commit introduces a reference count to the evlist structure. It ensures
that evlists are properly managed with evlist__get() and evlist__put() calls,
changing the Python bindings to handle evlists dynamically instead of cloning
them.

> The combination is needed for the python code to operate correctly (not hit
> asserts in the evsel clone), but the changes are broken apart for the sake
> of smaller patches.

Does this break bisectability?

Although the commit message states that the python code breakage is fixed in
a later commit in the series, knowingly introducing a broken state in the
middle of a patch series violates strict kernel bisectability rules.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 1e6c99efff90..3f0758d5bd90 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
[ ... ]
> @@ -1551,7 +1558,7 @@ static PyObject *pyrf_evlist__add(struct pyrf_evlist *pevlist,
> PyObject *args,
> PyObject *kwargs __maybe_unused)
> {
> - struct evlist *evlist = &pevlist->evlist;
> + struct evlist *evlist = pevlist->evlist;
> PyObject *pevsel;
> struct evsel *evsel;

Can this cause heap corruption upon evlist destruction?

Looking at the full context of pyrf_evlist__add(), it appends an interior
pointer to the C evlist:

Py_INCREF(pevsel);
evsel = &((struct pyrf_evsel *)pevsel)->evsel;
...
evlist__add(evlist, evsel);

When the newly allocated evlist is destroyed via the introduced evlist__put()
call, it invokes evlist__purge(), which calls evsel__delete() on all contained
events. This would trigger a native free() on the interior pointer
&pevsel->evsel from the Python-allocated object, leading to an abort in glibc.

While the commit message notes this is fixed later in the series, it leaves
the tree broken at this specific commit.

[ ... ]
> @@ -1794,17 +1817,16 @@ static PyObject *pyrf_evlist__item(PyObject *obj, Py_ssize_t i)
> struct pyrf_evlist *pevlist = (void *)obj;
> struct evsel *pos;
>
> - if (i >= pevlist->evlist.core.nr_entries) {
> + if (i >= pevlist->evlist->core.nr_entries) {
> PyErr_SetString(PyExc_IndexError, "Index out of range");
> return NULL;
> }
>
> - evlist__for_each_entry(&pevlist->evlist, pos) {
> + evlist__for_each_entry(pevlist->evlist, pos) {
> if (i-- == 0)
> break;
> }
> -
> - return Py_BuildValue("O", container_of(pos, struct pyrf_evsel, evsel));
> + return pyrf_evsel__from_evsel(pos);
> }

Does this result in lost mutations and BUG_ON crashes during script execution?

The new pyrf_evsel__from_evsel() call creates a shallow clone of the internal
evsel via evsel__clone(). This means modifications made from Python, such as
evlist[0].sample_period = 100, will only affect the ephemeral clone and are
silently lost for the actual event.

Additionally, evsel__clone() strictly asserts that the event is not open by
checking BUG_ON(orig->core.fd). If a Python script accesses an item after
calling evlist.open(), it will hit this assertion and crash the host process.

Again, while the commit message notes this is fixed later in the series,
it introduces bisectability breakage here.

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