Re: [PATCH v1 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: AB-BA lock inversion deadlock between dso__data_open_lock and dso->lock during fd eviction
- [High] perf dso: Out-of-bounds list access and crash due to dropped lock desyncing open count
- [Medium] perf vdso: TOCTOU race in machine__findnew_vdso allows duplicate VDSO dso insertion
--

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

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

This commit expands lock annotations for the struct dsos lock and struct
dso data structures, updating locking primitives to ensure proper 
concurrency tracking. It also switches VDSO addition to use the 
locked dsos__add function.

> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index ff5b30a3d4240..87a92238d25ba 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
[ ... ]
> @@ -552,13 +539,13 @@ static void dso__list_add(struct dso *dso) __must_hold(&_dso__data_open_lock)
>  	dso__data_open_cnt++;
>  }
>  
> -static void dso__list_del(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());
> +	mutex_unlock(&dso__data_open_lock);
>  	dso__put(dso__data(dso)->dso);
> -	mutex_lock(dso__data_open_lock());
> +	mutex_lock(&dso__data_open_lock);
>  #endif

[Severity: High]
Does dropping the lock before decrementing the count open a window for an
out-of-bounds access?

In dso__list_del(), the entry is removed from the list via list_del_init()
before dso__data_open_lock is dropped under REFCNT_CHECKING. However,
dso__data_open_cnt is not decremented until after the lock is reacquired.

If the list had exactly one element, it is now empty, but the count remains
positive during the unlocked window.

If another thread calls do_open() and hits EMFILE during this window, it
checks dso__data_open_cnt. Seeing a positive count, it will call
close_first_dso(), which blindly executes list_first_entry() on the empty
dso__data_open list. This results in an invalid pointer and subsequent memory
corruption.

>  	WARN_ONCE(dso__data_open_cnt <= 0,
>  		  "DSO data fd counter out of bounds.");
> @@ -567,7 +554,7 @@ static void dso__list_del(struct dso *dso) __must_hold(&_dso__data_open_lock)
>  
>  static void close_first_dso(void);
>  
> -static int do_open(char *name) __must_hold(&_dso__data_open_lock)
> +static int do_open(char *name) __must_hold(&dso__data_open_lock)
>  {
>  	do {
>  		int fd = open(name, O_RDONLY|O_CLOEXEC);
[ ... ]
> @@ -670,7 +657,7 @@ void dso__put_filename(struct dso *dso __maybe_unused, char *filename, bool deco
>  }
>  
>  static int __open_dso(struct dso *dso, struct machine *machine)
> -	__must_hold(&_dso__data_open_lock)
> +	__must_hold(&dso__data_open_lock)
>  {
>  	int fd = -EINVAL;
>  	char *name;
> @@ -703,7 +690,7 @@ static void check_data_close(void);
>   * list/count of open DSO objects.
>   */
>  static int open_dso(struct dso *dso, struct machine *machine)
> -	__must_hold(&_dso__data_open_lock)
> +	__must_hold(&dso__data_open_lock)

[Severity: High]
Could this sequence lead to an AB-BA lock inversion deadlock between
dso__data_open_lock and dso->lock?

When a thread opens a DSO via __open_dso(), it first acquires dso->lock.
If it calls do_open() and hits the open file limit (EMFILE), do_open() calls
close_first_dso() to evict an older DSO.

This leads to dso__list_del() being called for the evicted DSO. As seen
above, under REFCNT_CHECKING, dso__list_del() temporarily drops the global
dso__data_open_lock and calls dso__put(). At this moment, the current
thread still holds dso->lock for the DSO it was trying to open.

A concurrent thread can now acquire the uncontended dso__data_open_lock,
attempt to open the same DSO the first thread is processing, and block
waiting for dso->lock. When the first thread finishes dso__put(), it will
attempt to reacquire dso__data_open_lock, creating a classic AB-BA deadlock.

[ ... ]
> diff --git a/tools/perf/util/vdso.c b/tools/perf/util/vdso.c
> index c12f5d8c4bf69..862ca172ab59a 100644
> --- a/tools/perf/util/vdso.c
> +++ b/tools/perf/util/vdso.c
> @@ -131,7 +131,7 @@ static struct dso *__machine__addnew_vdso(struct machine *machine, const char *s
>  
>  	dso = dso__new(short_name);
>  	if (dso != NULL) {
> -		__dsos__add(&machine->dsos, dso);
> +		dsos__add(&machine->dsos, dso);
>  		dso__set_long_name(dso, long_name, false);
>  	}
>  

[Severity: Medium]
Does this missing double-check create a TOCTOU race that allows duplicate
VDSO entries to be inserted?

In machine__findnew_vdso(), there is an initial read-locked lookup via
dsos__find(). If the DSO is not found, it calls __machine__addnew_vdso()
which allocates a new DSO and delegates to dsos__add().

Because dsos__add() acquires the write lock and blindly appends the DSO
without verifying if another thread already inserted it, concurrent
threads calling map__new() can race. They can both miss the initial
read-locked lookup, both allocate new DSO objects, and both insert them.

This violates the unique DSO invariant and leaks the memory allocated
for the duplicate VDSO dso.

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