[PATCH v1] perf test: Fix maps use after put

[PATCH v1] perf test: Fix maps use after put

[Ian Rogers]

Fix a use after put reference count issue. maps is copied from leader,
but the leader is put on line 79 and then maps is used to read the
reference count below - so a use after put, with the put of maps
happening within thread__put. Fix by reversing the order of puts so
that the leader is put last.

To explain the reference count checker, I wrote this up as a little
example here:
https://perf.wiki.kernel.org/index.php/Reference_Count_Checking

Note, the bug was introduced by the committer and wasn't present in
the original reference count patch set.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/thread-maps-share.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/thread-maps-share.c b/tools/perf/tests/thread-maps-share.c
index 75ce8aedfc78..858e725318a9 100644
--- a/tools/perf/tests/thread-maps-share.c
+++ b/tools/perf/tests/thread-maps-share.c
@@ -76,16 +76,16 @@ static int test__thread_maps_share(struct test_suite *test __maybe_unused, int s
 	TEST_ASSERT_VAL("maps don't match", RC_CHK_ACCESS(other_maps) == RC_CHK_ACCESS(other_leader->maps));
 
 	/* release thread group */
-	thread__put(leader);
+	thread__put(t3);
 	TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 3);
 
-	thread__put(t1);
+	thread__put(t2);
 	TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 2);
 
-	thread__put(t2);
+	thread__put(t1);
 	TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 1);
 
-	thread__put(t3);
+	thread__put(leader);
 
 	/* release other group  */
 	thread__put(other_leader);
-- 
2.40.0.634.g4ca3ef3211-goog

Re: [PATCH v1] perf test: Fix maps use after put

[Arnaldo Carvalho de Melo]

Em Wed, Apr 19, 2023 at 08:04:30PM -0700, Ian Rogers escreveu:
> Fix a use after put reference count issue. maps is copied from leader,
> but the leader is put on line 79 and then maps is used to read the
> reference count below - so a use after put, with the put of maps
> happening within thread__put. Fix by reversing the order of puts so
> that the leader is put last.
> 
> To explain the reference count checker, I wrote this up as a little
> example here:
> https://perf.wiki.kernel.org/index.php/Reference_Count_Checking
> 
> Note, the bug was introduced by the committer and wasn't present in
> the original reference count patch set.

Yes, the bug predated your patch and is detected by the reference count
checking you contributed. 

This was just part of splitting up your series into smaller chunks, in
this case either we fix the problem detected while developing this
reference counting infrastructure or fix it later after the merged
infrastructure, when built with EXTRA_CFLAGS="-DREFCNT_CHECKING=1"
detects it when running 'perf test'.

Thanks for providing the separate patch fixing it.

Applied.

- Arnaldo
 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/thread-maps-share.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/tests/thread-maps-share.c b/tools/perf/tests/thread-maps-share.c
> index 75ce8aedfc78..858e725318a9 100644
> --- a/tools/perf/tests/thread-maps-share.c
> +++ b/tools/perf/tests/thread-maps-share.c
> @@ -76,16 +76,16 @@ static int test__thread_maps_share(struct test_suite *test __maybe_unused, int s
>  	TEST_ASSERT_VAL("maps don't match", RC_CHK_ACCESS(other_maps) == RC_CHK_ACCESS(other_leader->maps));
>  
>  	/* release thread group */
> -	thread__put(leader);
> +	thread__put(t3);
>  	TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 3);
>  
> -	thread__put(t1);
> +	thread__put(t2);
>  	TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 2);
>  
> -	thread__put(t2);
> +	thread__put(t1);
>  	TEST_ASSERT_EQUAL("wrong refcnt", refcount_read(maps__refcnt(maps)), 1);
>  
> -	thread__put(t3);
> +	thread__put(leader);
>  
>  	/* release other group  */
>  	thread__put(other_leader);
> -- 
> 2.40.0.634.g4ca3ef3211-goog
> 

-- 

- Arnaldo