Re: [PATCH v11 07/19] perf evlist: Add reference count checking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [Critical] Double Free and TOCTOU Race in `evlist__put()`
- [Critical] Use-After-Free / Dangling Pointers in `evlist__put` cycle collection
- [High] Memory Leak of `evsel` via missing `evsel__put()` in sample delivery
- [High] Memory leak of spliced `evlist`s due to missing re-parenting

Pre-existing issues:
- [High] Python memory leak in `pyrf_evlist__read_on_cpu`
- [High] Python object memory leak in `pyrf_evlist__add`
--

commit e390129ddc82f7909e4e9e3187e55f4b0eb8b77b
Author: Ian Rogers <irogers@google.com>
perf evlist: Add reference count checking

This patch adds reference count checking for the perf evlist to ensure that 
gets and puts are correctly paired and easy to debug.

> 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
> @@ -183,16 +200,21 @@ 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);
>  	}

[Severity: High]
Does ignoring the return value of refcount_dec_and_test() cause a memory leak
when splicing is used? When evlist__splice_list_tail() moves evsels from a
source evlist to a destination evlist, the spliced evsels still point to the
old source evlist. When this purge logic hits the source evlist, the refcount
drops to 0 and refcount_dec_and_test() returns true, but because the return
value is ignored, the memory for the source evlist is never actually freed.

[ ... ]

> @@ -211,15 +240,43 @@ void evlist__put(struct evlist *evlist)
> +retry:
> +	count = refcount_read(evlist__refcnt(evlist));
> +	ref_cnt = count;
> +	evlist__for_each_entry(evlist, evsel) {
> +		if (RC_CHK_EQUAL(evsel->evlist, evlist) && count)
> +			count--;
> +	}
> +	if (refcount_read(evlist__refcnt(evlist)) != ref_cnt)
> +		goto retry;
>  
> +	if (count != 0) {
> +		/*
> +		 * Not the last reference except for back references from
> +		 * evsels.
> +		 */
> +		RC_CHK_PUT(evlist);
> +		return;
> +	}

[Severity: Critical]
Can this TOCTOU race lead to a double free? If two threads concurrently drop
an external reference to the evlist, they both might execute
refcount_dec_and_test(). If the refcount drops to the number of evsels with
back-references, both threads could enter the retry block simultaneously, read
the count, and decrement it to 0. Since the refcount hasn't changed during
the loop, the verification evaluates to false for both. Both threads would
see count == 0 and fall through to out_delete, causing a double free.

[Severity: Critical]
Does this cycle collection logic prematurely free the evlist by ignoring the
refcounts of the evsel objects themselves? If an external user holds a strong
reference to an evsel (where its refcount > 1), it relies on the evsel's
strong reference to the evlist to keep the evlist alive. By proceeding to
destroy the evlist here without checking evsel refcounts, evlist__purge()
will set pos->evlist = NULL, leaving the external user with a dangling or
NULL evlist pointer.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -1804,27 +1823,32 @@ static PyObject *pyrf_evlist__add(struct pyrf_evlist *pevlist,
>  				  PyObject *args,
>  				  PyObject *kwargs __maybe_unused)
>  {
> -	struct evlist *evlist = pevlist->evlist;
> +	struct evlist *evlist;
>  	PyObject *pevsel;
>  	struct evsel *evsel;
>  
> +	CHECK_INITIALIZED(pevlist->evlist, "evlist");
> +
> +	evlist = pevlist->evlist;
>  	if (!PyArg_ParseTuple(args, "O!", &pyrf_evsel__type, &pevsel))
>  		return NULL;
>  
> -	Py_INCREF(pevsel);
>  	evsel = ((struct pyrf_evsel *)pevsel)->evsel;
> -	evsel->core.idx = evlist->core.nr_entries;
> +	CHECK_INITIALIZED(evsel, "evsel");
> +
> +	Py_INCREF(pevsel);
> +	evsel->core.idx = evlist__nr_entries(evlist);
>  	evlist__add(evlist, evsel__get(evsel));

[Severity: High]
This isn't a bug introduced by this patch, but does calling Py_INCREF(pevsel)
here cause a memory leak? The Python wrapper object is incremented but never
stored in any Python collection or decremented later. The underlying C evsel
is added to the C evlist (which takes its own reference via evsel__get).
Because the Python reference is incremented but never decremented, the wrapper
is permanently leaked, preventing its destructor from running and leaking the
initial C reference it holds.

[ ... ]

> @@ -1836,13 +1860,16 @@ static struct mmap *get_md(struct evlist *evlist, int cpu)
>  static PyObject *pyrf_evlist__read_on_cpu(struct pyrf_evlist *pevlist,
>  					  PyObject *args, PyObject *kwargs)
>  {
> -	struct evlist *evlist = pevlist->evlist;
> +	struct evlist *evlist;
>  	union perf_event *event;
>  	int sample_id_all = 1, cpu;
>  	static char *kwlist[] = { "cpu", "sample_id_all", NULL };
>  	struct mmap *md;
>  	int err;
>  
> +	CHECK_INITIALIZED(pevlist->evlist, "evlist");
> +
> +	evlist = pevlist->evlist;
>  	if (!PyArg_ParseTupleAndKeywords(args, kwargs, "i|i", kwlist,
>  					 &cpu, &sample_id_all))
>  		return NULL;

[Severity: High]
This is a pre-existing issue, but does this function leak the python object
on failure? If evlist__parse_sample() fails, the code returns an exception
via PyErr_Format() but fails to Py_DECREF() the allocated pyevent object,
permanently leaking it. Additionally, if it succeeds, it calls Py_INCREF()
before returning it, bumping the refcount to 2 while the caller only expects
1, preventing the object from ever being garbage collected.

> 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
> @@ -1959,21 +1963,23 @@ 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));
> -
> +	}
>  	machine = machines__find_for_cpumode(machines, event, sample);

[Severity: High]
Does adding evsel__get() here introduce a memory leak? The sample delivery
path populates sample->evsel and takes a reference, but the cleanup paths
like perf_sample__exit() do not appear to have a matching evsel__put(). This
permanently leaks the evsel object and its reference wrapper for events
processed this way.

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