Re: [PATCH 5/5] perf dso: Defer dropping the open list reference until after the lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Failing to allocate a deferred put node leaks the reference handle and causes a memory leak of the DSO object.
--

commit a29267f86fa45a10095cc91a7c0350e522de6c4a
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf dso: Defer dropping the open list reference until after the lock

This patch addresses a deadlock and resource imbalance issue by deferring
the dropping of the DSO open list reference. It transfers the reference to a
deferred node during dso__list_del(), which is later drained by
dso__put_deferred() outside of the dso__data_open_lock.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index 42bfe30a3b518..a4b2361bc7420 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
[ ... ]
> @@ -531,18 +531,79 @@ static void dso__list_add(struct dso *dso) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_o
>  	dso__data_open_cnt++;
>  }
>  
> +#ifdef REFCNT_CHECKING
> +/*
> + * A deferred put: carries the reference taken by dso__list_add() for an
> + * entry removed from dso__data_open.  Dedicated nodes are used so that
> + * the dso_data's own open_entry node can be relinked by a concurrent
> + * dso__list_add() without corrupting this list or its reference.
> + */
> +struct dso_data_put {
> +	struct list_head entry;
> +	struct dso *dso;
> +};
> +static LIST_HEAD(dso__data_open_put);
> +#endif
> +
>  static void dso__list_del(struct dso *dso) EXCLUSIVE_LOCKS_REQUIRED(_dso__data_open_lock)
>  {
> -	list_del_init(&dso__data(dso)->open_entry);
>  #ifdef REFCNT_CHECKING
> -	mutex_unlock(dso__data_open_lock());
> -	dso__put(dso__data(dso)->dso);
> -	mutex_lock(dso__data_open_lock());
> +	struct dso_data_put *put;
>  #endif
> +
> +	list_del_init(&dso__data(dso)->open_entry);
>  	WARN_ONCE(dso__data_open_cnt <= 0,
>  		  "DSO data fd counter out of bounds.");
>  	dso__data_open_cnt--;
> +#ifdef REFCNT_CHECKING
> +	/*
> +	 * The reference taken in dso__list_add() cannot be dropped while
> +	 * holding the open lock: dso__put() may call dso__data_close(),
> +	 * which takes dso__data_open_lock itself, deadlocking and leaving
> +	 * the list/counter state inconsistent for concurrent threads.
> +	 * Transfer the reference to a deferred node drained by
> +	 * dso__put_deferred() once the lock is released.
> +	 */
> +	put = zalloc(sizeof(*put));
> +
> +	if (put == NULL)
> +		return;

[Severity: Medium]
Does this early return cause a memory leak of the DSO object if zalloc fails?

During dso__list_del(), returning early without placing the deferred put node
onto dso__data_open_put abandons the reference in dso__data(dso)->dso. If the
DSO is subsequently added to the open list again, dso__list_add() could
overwrite this pointer with a new reference, permanently leaking the prior one
and preventing the DSO reference count from ever reaching 0.

> +
> +	put->dso = dso__data(dso)->dso;
> +	dso__data(dso)->dso = NULL;
> +	list_add_tail(&put->entry, &dso__data_open_put);
> +#endif
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903132251.237029-1-acme@kernel.org?part=5