Re: [PATCH v1 01/26] perf threads: Fix uninitialized pointer put when thread removal misses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] perf tools: TOCTOU race in threads__find() [1]
--

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

perf threads: Fix uninitialized pointer put when thread removal misses

This commit initializes old_value to NULL and checks the return value of
hashmap__delete() before calling thread__put() in threads__remove() and
threads__remove_all_threads().

> diff --git a/tools/perf/util/threads.c b/tools/perf/util/threads.c
> index 6ca0b178fb6c3..98cf7edaf7480 100644
> --- a/tools/perf/util/threads.c
> +++ b/tools/perf/util/threads.c

[ ... ]

> @@ -154,14 +154,14 @@ void threads__remove_all_threads(struct threads *threads)
>  void threads__remove(struct threads *threads, struct thread *thread)
>  {
>  	struct threads_table_entry *table  = threads__table(threads, thread__tid(thread));
> -	struct thread *old_value;
> +	struct thread *old_value = NULL;
>  
>  	down_write(&table->lock);
>  	if (table->last_match && RC_CHK_EQUAL(table->last_match, thread))
>  		__threads_table_entry__set_last_match(table, NULL);
>  
> -	hashmap__delete(&table->shard, thread__tid(thread), /*old_key=*/NULL, &old_value);
> -	thread__put(old_value);
> +	if (hashmap__delete(&table->shard, thread__tid(thread), /*old_key=*/NULL, &old_value))
> +		thread__put(old_value);
>  	up_write(&table->lock);
>  }

[Severity: Medium]
This problem wasn't introduced by this patch, but can a concurrent
threads__find() race with threads__remove() [1] and resurrect a
removed thread in table->last_match?

threads__find() looks up the thread under down_read(&table->lock), drops
the read lock, and then acquires down_write(&table->lock) inside
threads_table_entry__set_last_match() to update the cache:

tools/perf/util/threads.c:threads__find() {
    ...
	down_read(&table->lock);
	res = __threads_table_entry__get_last_match(table, tid);
	if (!res) {
		if (hashmap__find(&table->shard, tid, &res))
			res = thread__get(res);
	}
	up_read(&table->lock);
	if (res)
		threads_table_entry__set_last_match(table, res);
	return res;
}

If threads__remove() runs in the window after up_read(&table->lock),
threads__find() overwrites table->last_match with the deleted thread:

CPU0                                  CPU1
threads__find()
  down_read(&table->lock)
  hashmap__find(..., &res)
  up_read(&table->lock)
                                      threads__remove()
                                        down_write(&table->lock)
                                        __threads_table_entry__set_last_match(
                                          table, NULL)
                                        hashmap__delete(...)
                                        up_write(&table->lock)
  threads_table_entry__set_last_match()
    down_write(&table->lock)
    __threads_table_entry__set_last_match(
      table, res)  <-- resurrects removed thread in last_match

Subsequent lookups for that tid then return the removed thread from
table->last_match, causing stale event attribution and leaking the
cached reference until another lookup evicts it.

[1] https://sashiko.dev/#/bug/linux-4e27d490-a049-4f85-8e0b-b969e5b6a6bc

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