Re: [PATCH v2 07/10] perf dso: Fix unlocked __dsos__add() in vdso and expand lock annotations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] perf dso: Use-after-free of dso handle in dso__list_del
- [Medium] perf dso: TOCTOU race in dso__put crashes process via BUG_ON
- [Medium] perf: TOCTOU race in dsos__findnew_id() returns DSO with conflicting ID
--

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

perf dso: Fix unlocked __dsos__add() in vdso and expand lock annotations

This patch addresses a missing lock acquisition in __machine__addnew_vdso()
and introduces a read/write lock upgrade pattern for DSO ID improvement. It
also tightens locking for dso__list_del() to avoid lock inversions during
EMFILE eviction and expands lock annotations.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index ff5b30a3d4240..21491625ec821 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -539,13 +539,13 @@ static void dso__list_add(struct dso *dso) __must_hold(&_dso__data_open_lock)
>  static void dso__list_del(struct dso *dso) __must_hold(&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());
> +	BUG_ON(refcount_dec_and_test(&RC_CHK_ACCESS(dso)->refcnt));

[Severity: Medium]
If two threads concurrently call dso__put() on a DSO with 3 references, won't
both threads read the refcount as 3 and bypass the refcount_read() == 2
check? 

If both drop the refcount to 1 without calling dso__data_close(), could a
later EMFILE eviction calling dso__list_del() hit this BUG_ON when the
refcount unexpectedly drops to 0?

> +	RC_CHK_PUT(dso__data(dso)->dso);
> +	dso__data(dso)->dso = NULL;

[Severity: High]
Does RC_CHK_PUT() free the handle that is immediately dereferenced on the
next line when evaluating the dso__data(dso) macro, resulting in a heap
use-after-free during EMFILE eviction?

>  #endif
>  	WARN_ONCE(dso__data_open_cnt <= 0,
>  		  "DSO data fd counter out of bounds.");

[ ... ]

> @@ -1804,7 +1804,7 @@ struct dso *dso__get(struct dso *dso)
>  void dso__put(struct dso *dso)
>  {
>  #ifdef REFCNT_CHECKING
> -	if (dso && dso__data(dso) && refcount_read(&RC_CHK_ACCESS(dso)->refcnt) == 2)
> +	if (dso && dso__data(dso)->dso && refcount_read(&RC_CHK_ACCESS(dso)->refcnt) == 2)
>  		dso__data_close(dso);
>  #endif
>  	if (dso && refcount_dec_and_test(&RC_CHK_ACCESS(dso)->refcnt))

[ ... ]

> diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
> index a2cd37bd16602..0e4307e357689 100644
> --- a/tools/perf/util/dsos.c
> +++ b/tools/perf/util/dsos.c
[ ... ]
> @@ -343,8 +354,20 @@ static struct dso *__dsos__findnew_id(struct dsos *dsos, const char *name, const
>  struct dso *dsos__findnew_id(struct dsos *dsos, const char *name, const struct dso_id *id)
>  {
>  	struct dso *dso;
> +
> +	down_read(&dsos->lock);
> +	dso = __dsos__find_id(dsos, name, id, false, /*write_locked=*/false);
> +	if (dso && !dso__needs_improve_id(dso, id)) {
> +		up_read(&dsos->lock);
> +		return dso;
> +	}
> +	up_read(&dsos->lock);
> +
>  	down_write(&dsos->lock);
> -	dso = __dsos__findnew_id(dsos, name, id);
> +	if (dso)
> +		__dso__improve_id(dso, id);

[Severity: Medium]
If two threads concurrently call dsos__findnew_id() for the same DSO name but
with different valid IDs, couldn't both find the same empty DSO under the
read lock?

When they subsequently acquire the write lock, since the cached dso pointer
is used unconditionally without re-validation, could one thread incorrectly
receive a DSO that was just updated with the other thread's ID, leading to
incorrect symbol resolution and corrupted tracking structures?

> +	else
> +		dso = __dsos__findnew_id(dsos, name, id);
>  	up_write(&dsos->lock);
>  	return dso;
>  }

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