Re: [PATCH] perf unwind-libdw: Fix unwinding of multi-threaded processes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] Missing explicit POSIX standard header inclusion for `<assert.h>`, breaking compatibility with musl libc.

Pre-existing issues:
- [High] Missing NULL check on `zalloc` allocation for `dwfl_ui_ti` leads to a NULL pointer dereference, and unchecked `dwfl_begin()` failure leads to a memory leak.
- [High] The local pointer `ui` is declared as a static variable but is dynamically allocated and freed within the function, creating a severe data race.
--

--- Patch [1]: [PATCH] perf unwind-libdw: Fix unwinding of multi-threaded processes ---
commit cd2051f3de0d807edd3e4fcc7ed115a6364be27d
Author: Alessio Podda <aleph.pi.gh@gmail.com>

perf unwind-libdw: Fix unwinding of multi-threaded processes

This commit fixes unwinding of multi-threaded processes in perf. It updates
next_thread() to return the current sample's TID and provides get_thread()
so libdw can find the thread directly.

> diff --git a/tools/perf/util/unwind-libdw.c b/tools/perf/util/unwind-libdw.c
> index 7f35042be5677..3735ef83c8c48 100644
> --- a/tools/perf/util/unwind-libdw.c
> +++ b/tools/perf/util/unwind-libdw.c
> @@ -173,14 +173,30 @@ static int entry(u64 ip, struct unwind_info *ui)
>  	return 0;
>  }
>  
> -static pid_t next_thread(Dwfl *dwfl, void *arg, void **thread_argp)
> +static pid_t next_thread(Dwfl *dwfl __maybe_unused, void *arg, void **thread_argp)
>  {
> +	struct dwfl_ui_thread_info *dwfl_ui_ti = arg;
> +
>  	/* We want only single thread to be processed. */
>  	if (*thread_argp != NULL)
>  		return 0;
>  
> +	assert(dwfl_ui_ti->ui != NULL);

[Severity: Low]
Does this file need an explicit include for <assert.h>? The perf subsystem
guidelines require direct POSIX header inclusions for compatibility with
musl libc, and it appears the assert macro is being introduced here without
it.

> +	*thread_argp = arg;
> -	return dwfl_pid(dwfl);
> +	return thread__tid(dwfl_ui_ti->ui->thread);
>  }

[ ... ]

> @@ -400,7 +417,7 @@ int libdw__get_entries(unwind_entry_cb_t cb, void *arg,
>  	if (err)
>  		goto out;
>  
> -	dwfl_attach_state(dwfl, /*elf=*/NULL, thread__tid(thread), &callbacks,
> +	dwfl_attach_state(dwfl, /*elf=*/NULL, thread__pid(thread), &callbacks,
>  			  /* Dwfl thread function argument*/dwfl_ui_ti);
>  	// Ignore thread already attached error.

[Severity: High]
This is a pre-existing issue, but can the allocation for dwfl_ui_ti fail
and lead to a NULL pointer dereference?

Looking at earlier code in libdw__get_entries():

	dwfl_ui_ti = zalloc(sizeof(*dwfl_ui_ti));
	dwfl = dwfl_begin(&offline_callbacks);
	if (!dwfl)
		goto out;

	dwfl_ui_ti->dwfl = dwfl;

If zalloc fails, it seems we would dereference the NULL dwfl_ui_ti right
away. Also, if dwfl_begin fails, we jump to out:

out:
...
	dwfl_ui_ti->ui = NULL;

which would also dereference NULL, or leak the allocated dwfl_ui_ti if
zalloc had succeeded. Does this error path need a NULL check and proper
cleanup?

[Severity: High]
This is a pre-existing issue, but does the static declaration of the ui
pointer cause data races?

In libdw__get_entries(), ui is declared as static but dynamically
allocated:

	static struct unwind_info *ui;
...
	ui = zalloc(sizeof(*ui) + sizeof(ui->entries[0]) * max_stack);

And then freed at the end of the same function:

	free(ui);

If multiple threads process events concurrently, could this static pointer
be overwritten, leading to memory leaks, use-after-free, or double-free
issues?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722102407.3755154-1-aleph.pi.gh@gmail.com?part=1