Re: [PATCH 1/9] perf machine: Propagate machine__init() error to callers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Error paths introduced by checking machines__init() result in cleanup routines destroying uninitialized mutexes and rwlocks.
- [High] Missing error handling for the newly propagated return value of machines__init() in aslr_tool__init().
--

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

perf machine: Propagate machine__init() error to callers

machine__init() always returns 0 even when memory allocation fails,
because commit 81f981d7ec43ed93 ("perf machine: Free root_dir in
machine__init() error path") introduced 'int err = -ENOMEM' and an
error cleanup path but left the final 'return 0' instead of
'return err'.

[ ... ]
> diff --git a/tools/perf/tests/hists_cumulate.c b/tools/perf/tests/hists_cumulate.c
> index 267cbc24691ac..9f4d4e281e11e 100644
> --- a/tools/perf/tests/hists_cumulate.c
> +++ b/tools/perf/tests/hists_cumulate.c
> @@ -723,7 +723,8 @@ static int test__hists_cumulate(struct test_suite *test __maybe_unused, int subt
>  		goto out;
>  	err = TEST_FAIL;
>  
> -	machines__init(&machines);
> +	if (machines__init(&machines))
> +		goto out;

[Severity: High]
If machines__init() fails here, we jump to out:

out:
	/* tear down everything */
	evlist__delete(evlist);
	machines__exit(&machines);
	put_fake_samples();

Since machines is allocated on the stack and machines__init() can fail before
fully initializing it, will machines__exit() try to clean up uninitialized
state, such as calling down_write() on an uninitialized rwlock?

[ ... ]
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 1a9a008ddda35..26bacb6c1a572 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -160,7 +160,9 @@ struct perf_session *__perf_session__new(struct perf_data *data,
>  	session->decomp_data.zstd_decomp = &session->zstd_data;
>  	session->active_decomp = &session->decomp_data;
>  	INIT_LIST_HEAD(&session->auxtrace_index);
> -	machines__init(&session->machines);
> +	if (machines__init(&session->machines))
> +		goto out_delete;
> +

[Severity: High]
If we jump to out_delete here, perf_session__delete(session) is called, which
unconditionally calls perf_env__exit(&session->header.env).

Because this goto happens before perf_env__init(&session->header.env) is called,
could perf_env__exit() end up destroying uninitialized rwlocks and mutexes in
the zero-allocated session->header.env?

[ ... ]

[Severity: High]
Although not modified in this patch, the newly propagated return value of
machines__init() seems to be ignored in tools/perf/util/aslr.c in the
aslr_tool__init() function.

If machines__init() fails there, could aslr_tool__init() silently proceed
with an uninitialized machines structure, leading to crashes when it is
subsequently used or destroyed?

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