Re: [PATCH 5/8] perf machine: Don't abort guest map creation on first inaccessible dir

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] errno is checked for ERANGE after calling strtol() without explicitly clearing it to 0 beforehand, causing a logic error where one invalid directory skips all subsequent valid guest directories.
--

--- Patch [5]: [PATCH 5/8] perf machine: Don't abort guest map creation on first inaccessible dir ---
commit 3942aff28b363eb417be58b368d8c95bd54e8551
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf machine: Don't abort guest map creation on first inaccessible dir

machines__create_guest_kernel_maps() is modified to use 'continue'
instead of jumping to a failure label when a guest directory's
kallsyms file is inaccessible. This allows the loop to process
remaining valid guest directories rather than aborting early.

> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 05724277c2a97..83d38637afe34 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1269,14 +1269,12 @@ int machines__create_guest_kernel_maps(struct machines *machines)

[Severity: High]
This is a pre-existing issue, but does this loop correctly handle errno
after a failed strtol() call?

Looking at machines__create_guest_kernel_maps(), the loop checks errno
for ERANGE after calling strtol(), but there doesn't appear to be an
explicit reset of errno beforehand:

tools/perf/util/machine.c:machines__create_guest_kernel_maps() {
    ...
    pid = (pid_t)strtol(namelist[i]->d_name, &endp, 10);
    if ((*endp != '\0') ||
        (endp == namelist[i]->d_name) ||
        (errno == ERANGE)) {
    ...
}

If a directory name causes an overflow (e.g., exceeding LONG_MAX), strtol()
sets errno to ERANGE. In the next loop iteration, if errno is not cleared
to 0, it will retain the ERANGE value. When strtol() processes a valid PID,
it succeeds, but the (errno == ERANGE) check evaluates to true, causing the
valid directory to be incorrectly skipped.

Since this loop processes directories from guestmount, could an unprivileged
user create a bogus directory to prevent symbol resolution for all subsequent
guests?

>  			snprintf(path, sizeof(path), "%s/%s/proc/kallsyms",
>  				 symbol_conf.guestmount,
>  				 namelist[i]->d_name);
> -			ret = access(path, R_OK);
> -			if (ret) {
> +			if (access(path, R_OK)) {
>  				pr_debug("Can't access file %s\n", path);
> -				goto failure;
> +				continue;
>  			}
>  			machines__create_kernel_maps(machines, pid);
>  		}
> -failure:
>  		free(namelist);
>  	}

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