Re: [PATCH 06/12] perf symbol: Fall back to fetching the vmlinux by build ID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf symbol: NULL pointer dereference in trampoline map adjustment due to lock dropping
--

commit 54cc3e4d87729d86b17ea862346d56f11117f916
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

    perf symbol: Fall back to fetching the vmlinux by build ID

    A profile recorded on a kernel that is no longer installed, be it
    because the machine was rebooted into a new kernel or because the
    profile is being processed on another machine, can't have its kernel
    symbols resolved. As a last resort, when the kernel symbols can't be
    found locally and the user didn't specify a kallsyms file, fetch the
    vmlinux keyed by the kernel build ID recorded in the perf.data file
    using the debuginfod client and use it for symbols.

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 1d70c43bb24ee..ccb6217119416 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
[ ... ]
> @@ -2257,7 +2280,53 @@ static int dso__load_kernel_sym(struct dso *dso, struct map *map)
>  		pr_debug("Using %s for symbols\n", kallsyms_filename);
>  	free(kallsyms_allocated_filename);
>  
> -	if (err > 0 && !dso__is_kcore(dso)) {
> +	/*
> +	 * The kallsyms may be unavailable or restricted, e.g.
> +	 * /proc/kallsyms with kernel.perf_event_paranoid > 1, try to fetch
> +	 * the vmlinux keyed by the build ID using debuginfod as a last
> +	 * resort, honoring --ignore-vmlinux and --ignore-vmlinux_buildid
> +	 * like the other vmlinux sources above.
> +	 */
> +	if (err <= 0 && !user_kallsyms &&
> +	    !symbol_conf.ignore_vmlinux &&
> +	    !symbol_conf.ignore_vmlinux_buildid) {
> +		char *fetched_path = NULL;
> +
> +		/*
> +		 * dso__load() holds dso->lock while it calls us, and the
> +		 * fetch below can take a long time, blocked on the network
> +		 * or on the terminal, waiting for the user: do it with the
> +		 * lock dropped, as dso__debuginfo() does for the debuginfo
> +		 * of a DSO, so that the threads that need this dso don't get
> +		 * stuck behind a server round trip.  Nothing of the dso is
> +		 * touched by the fetch, the symbols are loaded with the lock
> +		 * held again, and only if the fetch brought a file back.
> +		 */
> +		mutex_unlock(dso__lock(dso));
> +		err = dso__fetch_vmlinux_build_id(dso, &fetched_path);
> +		mutex_lock(dso__lock(dso));

[Severity: Medium]
Could dropping the lock here lead to a race condition that causes a NULL
pointer dereference?

If a second thread enters dso__load() while the lock is dropped here, it
would see dso__loaded(dso) is still false and also enter
dso__load_kernel_sym(). Both threads would eventually return and
consecutively call machine__map_x86_64_entry_trampolines().

Since machine__map_x86_64_entry_trampolines_cb() translates map->pgoff
in place without idempotency guarantees:

tools/perf/util/machine.c:machine__map_x86_64_entry_trampolines_cb()
    dest_map = maps__find(args->kmaps, map__pgoff(map));
    if (RC_CHK_ACCESS(dest_map) != RC_CHK_ACCESS(map))
        map__set_pgoff(map, map__map_ip(dest_map, map__pgoff(map)));

Would the second execution fail to find dest_map due to the modified
pgoff, and subsequently pass NULL to map__map_ip(), resulting in a
fatal NULL pointer dereference?

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