Re: [PATCH v3 3/6] perf header: Support memory ranges

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The bounds check for `local_region_id` and `remote_region_id` incorrectly rejects the valid value of 255 (UINT8_MAX).
- [Medium] Missing explicit inclusion of `<fcntl.h>` for `open()`, `openat()`, `O_RDONLY`, and `O_DIRECTORY`, causing musl build failures.
- [Medium] Repeated parsing of HEADER_MEMORY_RANGES sections leaks previously allocated memory.
--

--- Patch [3]: [PATCH v3 3/6] perf header: Support memory ranges ---
commit 0b006df2ed3ab47f9865f5a75df7ab210c86b2a2
Author: Thomas Falcon <thomas.falcon@intel.com>

perf header: Support memory ranges

This commit adds support for parsing memory range data from the ACPI MRRM
table exposed via sysfs. It saves this data in the perf data file header
under a new feature bit, HEADER_MEMORY_RANGES, and adds support for
displaying it.

Link: https://lore.kernel.org/lkml/20250505173819.419271-1-tony.luck@intel.com/

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 091d8f7f6bd2c..a9dedbe390243 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
[ ... ]
> @@ -1891,6 +1892,132 @@ static int write_cpu_domain_info(struct feat_fd *ff,
>  	return ret;
>  }
>  
> +static int memory_range__read(struct memory_range *range, const char *path)
> +{
> +	char buf[32];
> +	ssize_t n;
> +	int dfd, fd, tmp, ret = -1;
> +
> +	dfd = open(path, O_RDONLY | O_DIRECTORY);

[Severity: Medium]
Since this function uses open(), openat(), O_RDONLY, and O_DIRECTORY, should
<fcntl.h> be explicitly included at the top of the file to prevent build
failures on musl libc?

> +	if (dfd < 0)
> +		return -1;
> +
> +#define _R(name, dst, conv)			\
> +	fd = openat(dfd, name, O_RDONLY);	\
[ ... ]
> +	_R("local_region_id", tmp, strtol);
> +	if (tmp < 0 || tmp >= UINT8_MAX)
> +		goto out;
> +	range->local_region_id = tmp;
> +	_R("remote_region_id", tmp, strtol);
> +	if (tmp < 0 || tmp >= UINT8_MAX)
> +		goto out;

[Severity: High]
Does this bounds check incorrectly reject the valid value of 255? 

UINT8_MAX evaluates to 255, so using >= will cause the function to reject
255 and jump to the error path. However, the example output in the commit
message shows remote_region_id = 255, which suggests 255 is an expected and
valid value.

> +	range->remote_region_id = tmp;
> +#undef _R
> +
> +	ret = 0;
> +out:
> +	close(dfd);
> +	return ret;
> +}
[ ... ]
> +static int process_memory_ranges(struct feat_fd *ff, void *data __maybe_unused)
> +{
> +	struct perf_env *env = &ff->ph->env;
> +	struct memory_range *ranges, *r;
> +	u32 nr_memory_ranges, i;
[ ... ]
> +	ranges = calloc(nr_memory_ranges, sizeof(*ranges));
> +	if (!ranges)
> +		return -1;
[ ... ]
> +	env->memory_ranges = ranges;
> +	env->nr_memory_ranges = nr_memory_ranges;
> +
> +	return 0;

[Severity: Medium]
If a perf.data file contains multiple HEADER_MEMORY_RANGES sections, could 
this leak the previously allocated memory? 

This assignment blindly overwrites env->memory_ranges with the newly allocated
ranges array without first checking for or freeing any existing allocation.

> +error:
> +	zfree(&ranges);
> +	return -1;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803204540.100348-1-thomas.falcon@intel.com?part=3